From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Felix Fietkau <nbd@nbd.name>, linux-wireless@vger.kernel.org
Cc:     johannes@sipsolutions.net
Subject: Re: [RFC] mac80211: rework locking for txq scheduling / airtime fairness
In-Reply-To: <50e3ab54-4e8c-171c-28f0-7e3ad5a02c99@nbd.name>
References: <20190313181531.62539-1-nbd@nbd.name> <87va0mz8nu.fsf@toke.dk> <50e3ab54-4e8c-171c-28f0-7e3ad5a02c99@nbd.name>
Date:   Thu, 14 Mar 2019 23:17:12 +0100
Message-ID: <87bm2dxfrr.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2019-03-13 23:55, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> Holding the lock around the entire duration of tx scheduling can
>>> create some nasty lock contention, especially when processing airtime
>>> information from the tx status or the rx path.
>> 
>> Right, I can see how that could become an issue at higher loads than
>> what I tested with :)
> I stumbled onto this when I was doing perf runs with mt76 (before even
> adding support for this API) and I noticed that a visible amount of lock
> contention was caused by mac80211 tx calls being blocked by mt76 tx
> scheduling.
> I wanted to fix these issues by switching to the mac80211 txq scheduling
> API, but when reading the code I noticed that using this API had the
> very same issue I was trying to fix. That's why I made this patch :)

Ah, I see. Well I applaud you making fixing this and switching over mt76
your solution :)

>>> Improve locking by only holding the active_txq_lock for lookups /
>>> scheduling list modifications.
>> 
>> So the (potential) issue I see with this modification is that it
>> requires the driver to ensure that it will not interleave two different
>> scheduling rounds for the same acno. I.e., another call to
>> schedule_start() will reset the round counter and something needs to
>> guarantee that this doesn't happen until the driver has actually
>> finished the previous round.
>> 
>> I am not sure to what extent this would *actually* be a problem. For
>> ath9k, for instance, there's already the in-driver chan_lock (although
>> the call to ieee80211_txq_schedule_start() would then have to be moved
>> into the section covered by that lock). But it does introduce an
>> implicit dependency in the API, which should at least be documented.
> With ath9k it's also protected by the per-txq lock.

Ah, right, that was just moved not removed entirely. Great!

> With ath10k it's protected by having scheduling calls come from the NAPI
> poll function.

Cool.

>> If memory serves, avoiding this implicit dependency was the original
>> reason I had for adding the full lock around everything. I can't think
>> of any other reason right now, but if I do think of something I'll be
>> sure to let you know :)
> I'll change the patch to make this more explicit and resubmit.
> Thanks for your comments.

Sounds good!

-Toke