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
Subject: Re: [PATCH 1/6] mt76: use mac80211 txq scheduling
In-Reply-To: <2cc1f1d4-f07e-faa0-ede7-95dd2917a64a@nbd.name>
References: <20190316204242.73560-1-nbd@nbd.name> <87muluv4hd.fsf@toke.dk> <7b9a0a24-14b0-9b96-0010-ce9f96308ad0@nbd.name> <875zshviqg.fsf@toke.dk> <2cc1f1d4-f07e-faa0-ede7-95dd2917a64a@nbd.name>
Date:   Sun, 17 Mar 2019 22:59:09 +0100
Message-ID: <87r2b5tb6a.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-17 12:32, Toke Høiland-Jørgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>> 
>>> On 2019-03-16 23:28, Toke Høiland-Jørgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>> 
>>>>> Performance improvement and preparation for adding airtime fairness
>>>>> support
>>>> 
>>>> Great to see this! Do you have a plan for the airtime fairness part?
>>>> I.e., how to get the airtime information?
>>> Not yet. Still need to investigate what kind of information the hardware
>>> can provide. On a first glance it seems rather limited, so we may have
>>> to approximate based on tx status rates/retry and average packet size.
>> 
>> OK, cool. A byte-based estimator can also be useful for preventing dumb
>> firmware from buffering too much. The Chromium guys did that for ath10k:
>> 
>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/588190/13/drivers/net/wireless-4.2/ath/ath10k/mac.c#3826
> Interesting, thanks. I can probably use some ideas from that.
>
>>>> The call to ieee80211_return_txq() is really meant to be unconditional.
>>>> The TXQ will only actually be scheduled if it still has packets queued.
>>>> I know it's slightly more expensive to have the check in mac80211, but
>>>> this is what makes it possible to change the implementation without
>>>> touching the drivers (such as in the RFC patch I sent earlier that
>>>> switches the scheduling algorithm)...
>>> I think this API needs to be extended to allow the driver to specify
>>> that it has buffered packets for a txq. Otherwise there's a small window
>>> where the driver has packets for a txq but mac80211 doesn't, and
>>> mac80211 won't schedule the queue in that case.
>>> I'll send a patch for this soon.
>> 
>> Right, makes sense. As long as mac80211 is in control over how it will
>> react to that information (thus allowing to e.g., invert the logic if
>> needed), I have no objections to extending the API... :)I'm thinking of changing the code to make ieee80211_schedule_txq add the
> txq to the list, even if mac80211 does not have any frames buffered for it.
>
> I've looked at ath9k (the only user at the moment), and it seems to call
> the function in that way already: at PS wake or tx status time if it has
> frames in its internal retry queue.
> While it does not match the current documented behavior for that
> function, it nicely fits ath9k's currently unfulfilled expectations ;)

Heh, fair point :)

-Toke