Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Felix Fietkau <nbd@nbd.name>, Kan Yan <kyan@google.com>,
        johannes@sipsolutions.net
Cc:     linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net, yiboz@codeaurora.org,
        john@phrozen.org, lorenzo@kernel.org, rmanohar@codeaurora.org,
        kevinhayes@google.com
Subject: Re: [PATCH v11 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
In-Reply-To: <04284fb6-4a68-8449-4105-dcf90f128ef8@nbd.name>
References: <20191119060610.76681-1-kyan@google.com> <20191119060610.76681-5-kyan@google.com> <789d592c-5b1b-b785-6d9c-86b7cc7d57f4@nbd.name> <87k149xbb4.fsf@toke.dk> <829b6b28-99cd-ea9d-fea3-603a10eae401@nbd.name> <875zfsxs0u.fsf@toke.dk> <04284fb6-4a68-8449-4105-dcf90f128ef8@nbd.name>
Date:   Thu, 27 Feb 2020 12:59:19 +0100
Message-ID: <874kvcw8a0.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Felix Fietkau <nbd@nbd.name> writes:

> On 2020-02-27 11:07, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>=20
>>> On 2020-02-26 22:56, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>> - We need an API that allows the driver to change the pending airtime
>>>>> values, e.g. subtract estimated tx time for a packet.
>>>>> mt76 an ath9k can queue packets inside the driver that are not curren=
tly
>>>>> in the hardware queues. Typically if the txqs have more data than what
>>>>> gets put into the hardware queue, both drivers will pull an extra fra=
me
>>>>> and queue it in its private txq struct. This frame will get used on t=
he
>>>>> next txq scheduling round for that particular station.
>>>>> If you have lots of stations doing traffic (or having driver buffered
>>>>> frames in powersave mode), this could use up a sizable chunk of the A=
QL
>>>>> budget.
>>>>=20
>>>> I'm a bit more skeptical about this. If the driver buffers a bunch of
>>>> packets that are not accounted that will hurt that station due to extra
>>>> latency when it wakes up. For ath9k, this is the retry_q you're talking
>>>> about, right? The number of packets queued on that is fairly limited,
>>>> isn't it? What kind of powersave buffering is the driver doing, and why
>>>> can't it leave the packets on the TXQ? That would allow them to be
>>>> scheduled along with any new ones that might have arrived in the
>>>> meantime, which would be a benefit for latency.
>>> For mt76 there should be max. 1 frame in the retry queue, it's just a
>>> frame that was pulled from the txq in a transmission attempt but that it
>>> couldn't put in the hw queue because it didn't fit in the current
>>> aggregate batch.
>>=20
>> Wait, if it's only a single frame that is queued in the driver, how is
>> this causing problems? We deliberately set the limit so there was a bit
>> of slack above the size of an aggregate for things like this. Could you
>> please describe in a bit more detail what symptoms you are seeing of
>> this problem? :)
> It would be a single frame per sta/txq. I don't know if it will cause
> problems in practice, it's just a potential corner case that I found
> during review. I'd imagine this would probably show up in some
> benchmarks at least.
> I'm not seeing any symptoms myself, but I also haven't run any intricate
> tests yet.

See my other reply; I'm not convinced this behaviour is wrong :)

-Toke