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: <20190313181531.62539-1-nbd@nbd.name>
References: <20190313181531.62539-1-nbd@nbd.name>
Date:   Wed, 13 Mar 2019 23:55:33 +0100
Message-ID: <87va0mz8nu.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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 :)

> 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.

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 :)

-Toke