Received-SPF: pass (google.com: domain of linux-wireless+bounces-3440-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@kernel.org>
To: Johannes Berg <johannes@sipsolutions.net>, Felix Fietkau <nbd@nbd.name>,
 linux-wireless@vger.kernel.org
Subject: Re: [RFC] mac80211: add AQL support for broadcast packets
In-Reply-To: <66bddf2f6362c9f39f06e06c0c35b6900917b9bf.camel@sipsolutions.net>
References: <20240209184730.69589-1-nbd@nbd.name> <87plx4s71y.fsf@toke.dk>
 <960efcac-0995-4a42-b90c-3e66c0f56762@nbd.name>
 <66bddf2f6362c9f39f06e06c0c35b6900917b9bf.camel@sipsolutions.net>
Date: Mon, 12 Feb 2024 11:56:17 +0100
Message-ID: <87sf1yymr2.fsf@toke.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Johannes Berg <johannes@sipsolutions.net> writes:

> On Sat, 2024-02-10 at 17:18 +0100, Felix Fietkau wrote:
>> 
>> > > +++ b/include/net/cfg80211.h
>> > > @@ -3385,6 +3385,7 @@ enum wiphy_params_flags {
>> > >  /* The per TXQ device queue limit in airtime */
>> > >  #define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L	5000
>> > >  #define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H	12000
>> > > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_BC	50000
>> > 
>> > How did you arrive at the 50 ms figure for the limit on broadcast
>> > traffic? Seems like quite a lot? Did you experiment with different
>> > values?
>> 
>> Whenever a client is connected and in powersave mode, all multicast 
>> packets are buffered and sent after the beacon. Because of that I 
>> decided to use half of a default beacon interval.
>
> That makes some sense, I guess.

This implies that we will allow enough data to be queued up in the
hardware to spend half the next beacon interval just sending that
broadcast data? Isn't that a bit much if the goal is to prevent
broadcast from killing the network? What effect did you measure of this
patch? :)

Also, as soon as something is actually transmitted, the kernel will
start pushing more data into the HW from the queue in the host. So the
HW queue limit shouldn't be set as "this is the maximum that should be
transmitted in one go", but rather "this is the minimum time we need for
the software stack to catch up and refill the queue before it runs
empty". So from that perspective 50ms also seems a bit high?

> It does have me wondering though if we should also consider multicast
> for airtime fairness in some way?

Yeah, that would make sense. The virtual time-based scheduler that we
ended up reverting actually included airtime accounting for the
multicast queue as well. I don't recall if there was any problem with
that particular part of the change, or if it's just incidental that we
got rid of it as part of the revert. But it may be worth revisiting and
adding a similar mechanism to the round-robin scheduler...

-Toke