Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4040f674-8430-69ba-1b6f-f9fd92da413d@nbd.name>
Date:   Wed, 21 Sep 2022 10:05:22 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.13.0
Content-Language: en-US
To:     Venkat Ch <venkatch@gmail.com>
Cc:     johannes@sipsolutions.net, linux-wireless@vger.kernel.org,
        Venkat Chimata <venkata@shasta.cloud>
References: <20220915043527.737133-1-venkatch@gmail.com>
 <238a21a8-c97b-5a38-6c08-9057055bf73f@nbd.name>
 <CABHL29H75U-0dBP14d-6ds-xMPt1eqrvQPuMp3oY8qQS8Y3Dbw@mail.gmail.com>
 <9acc4159-8223-bbca-a83f-d075660ac6db@nbd.name>
 <CABHL29GB6DCrdu4FzRfBDSj9A7pwqnWE+wgH2xfkBMLjT3bAxA@mail.gmail.com>
 <f75ad975-8b76-f3e6-151b-6eda552c0431@nbd.name>
 <CABHL29GUgta4Ke3sxbongMSK__cruTaDYCXe1-yEDL3WA3mO9Q@mail.gmail.com>
From:   Felix Fietkau <nbd@nbd.name>
Subject: Re: [PATCH v2] wifi: mac80211: Fix performance issue with mutex_lock
In-Reply-To: <CABHL29GUgta4Ke3sxbongMSK__cruTaDYCXe1-yEDL3WA3mO9Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 20.09.22 21:23, Venkat Ch wrote:
> Hi Felix,
> 
>   Following is the background of the problem, how I traced to
> mutex_lock and why I propose rcu locks.
> 
> Issue:
>   On a 10Mbps upload / 50 Mbps download connection, the following issue reported.
> 
> Video periodically freezes and/or appears delayed when on Zoom or Teams.
> 1. Video will freeze for 10 or 15 seconds periodically when on a call,
> but audio continues and the session doesn't drop.
> 2. The video  still works but it appears delay (I move, but the video
> of my movement is noticeably delay by a second or so)
> 
> Tracing to mutex_lock(sta_mutex):
> 
>   When I investigated, I found that the ucentral agent in openwifi
> fetches the station list periodically.  Without the station list
> fetch, the video quality is just fine. I investigated the station list
> path and found this mutex_lock. I also see that earlier it was
> rcu_lock which protected the station list in this path. In this
> commit, https://github.com/torvalds/linux/commit/66572cfc30a4b764150c83ee5d842a3ce17991c9,
> rcu lock was changed to mutex lock without providing any reason.
The reason seems clear to me, even though it was not explicitly stated 
in the commit message: in sta_set_sinfo it introduces a call to a driver 
op that is allowed to sleep.

> I also saw this comment just above the sta_mutex declaration.
>          /* Station data */
>          /*
>           * The mutex only protects the list, hash table and
>           * counter, reads are done with RCU.
>           */
>          struct mutex sta_mtx;
> 
> So I reverted back the mutex_lock with rcu_lock and it just worked
> fine. We tested for more than 2 weeks before concluding this analysis.
> 
> I think the usage of mutex_lock is impacting the tx / rx path
> somewhere and hence the issue. It is a challenge to trace the exact
> function though.

I don't see any critical part in the tx/rx path which depends on the 
sta_mtx lock. My guess is that for some reason your change is simply 
accidentally papering over the real bug, which may be somewhere else 
entirely, maybe even in the driver. A freeze for 10-15 seconds 
definitely does not sound like simple lock contention on the mutex, 
since a single station dump will be much faster than that.

- Felix