Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Maxime Bizon <mbizon@freebox.fr>
Cc:     linux-wireless@vger.kernel.org
Subject: Re: Regarding .wake_tx_queue() model
In-Reply-To: <20200505152010.GA33304@sakura>
References: <20200504193959.GC26805@sakura> <878si6oabp.fsf@toke.dk> <20200505131531.GA32619@sakura> <87368eo5dn.fsf@toke.dk> <20200505152010.GA33304@sakura>
Date:   Tue, 05 May 2020 18:50:45 +0200
Message-ID: <87pnbimil6.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

Maxime Bizon <mbizon@freebox.fr> writes:

> On Tuesday 05 May 2020 =C3=A0 15:53:08 (+0200), Toke H=C3=B8iland-J=C3=B8=
rgensen wrote:
>
>> Well, I think that should be fine? Having a longer HW queue is fine, as
>> long as you have some other logic to not fill it all the time (like the
>> "max two aggregates" logic I mentioned before). I think this is what
>> ath9k does too. At least it looks like both drv_tx() and
>> release_buffered_frames() will just abort (and drop in the former case)
>> if there is no HW buffer space left.
>
> Ok
>
> BTW, the "max two aggregates" rule, why is it based on number of
> frames and not duration ? if you are queing 1500 bytes @1Mbit/s, even
> one frame is enough, but not so for faster rates.

It's the minimum amount that works - assuming you get a TX completion
when one is done, the CPU has time to build the next one before the
second one is done, and you avoid starvation. Note this only works well
for aggregates, since their size tend to vary with rate; if you're
queueing individual packets to the HWQ you need something that takes
rate into account, which is what AQL (Airtime Queue Limits) does for
ath10k.

> It would be even better if minstrel could limit the total duration
> when computing number of hardware retries, and then mac80211 could
> handle software retries for those really slow packets, no hardware
> FIFO "pollution"

Minstrel will compute the max aggregate size based on the rate, which is
why the "two aggregates" scheme works. It likely could be smarter about
limiting the number of retries, as you say, but we never did get around
to doing anything about that :)

>> > Also .release_buffered_frames() codepath is difficult to test, how do
>> > you trigger that reliably ? assuming VIF is an AP, then you need the
>> > remote STA to go to sleep even though you have traffic waiting for it
>> > in the txqi. For now I patch the stack to introduce artificial delay.
>> >
>> > Since my hardware has a sticky per-STA PS filter with good status
>> > reporting, I'm considering using ieee80211_sta_block_awake() and only
>> > unblock STA when all its txqi are empty to get rid of
>> > .release_buffered_frames() complexity.
>>=20
>> I'm probably not the right person to answer that; never did have a good
>> grip on the details of PS support.
>
> Hopefully Felix or Johannes will see this.
>
> Just wondering if there is anything I'm missing, this alternative way
> of doing looks easier to me because it removes "knowledge" of frame
> delivery under service period from the driver:
>
>
> 1) first get rid of buffered txq traffic when entering PS:
>
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1593,6 +1593,15 @@ static void sta_ps_start(struct sta_info *sta)
>                         list_del_init(&txqi->schedule_order);
>                 spin_unlock(&local->active_txq_lock[txq->ac]);
>=20=20
> -               if (txq_has_queue(txq))
> -                       set_bit(tid, &sta->txq_buffered_tids);
> -               else
> -                       clear_bit(tid, &sta->txq_buffered_tids);
> +               /* transfer txq into tx_filtered frames */
> +               spin_lock(&fq->lock);
> +               while ((skb =3D skb_dequeue(&txq->frags)))
> +                       skb_queue_tail(&sta->tx_filtered[txq->ac], skb);
> +               /* use something more efficient like fq_tin_reset  */
> +               while ((skb =3D fq_tin_dequeue(fq, tin, fq_tin_dequeue_fu=
nc)))
> +                       skb_queue_tail(&sta->tx_filtered[txq->ac], skb);
> +               spin_unlock_bh(&fq->lock);

This seems like a bad idea; we want the TXQ mechanism to decide which
frame to send on wakeup.

> 2) driver register for STA_NOTIFY_SLEEP
>
> 3) driver count tx frames pending in the hardware per STA and sets
> ieee80211_sta_block_awake(sta, 1) when > 0
>
> 4) on tx completion, if STA is sleeping and number of pending tx frames i=
n hardware for a
> given STA reaches 0:
>  - if driver buffers other frames for this STA, release them with TX_FILT=
ERED in reverse order
>  - calls ieee80211_sta_block_awake(false)
>
> what do you think ?

As I said, I'm not an expert on the PS code, so I may be missing
something. But it seems to me that in a model where the driver pulls the
frames from mac80211 (i.e., for drivers using wake_tx_queue), there
really is no way around having a way to instruct the driver "please use
these flags for the next N frames you send" - which is what
release_buffered_frames() does. What you're suggesting is basically
turning off this 'pull mode' for the frames buffered during PS and have
mac80211 revert to push mode for those, right? But then you lose the
benefits of pull mode (the TXQs) for those frames.

I remember Johannes talking about a 'shim layer' between the mac80211
TXQs and the 'drv_tx()' hook as a way to bring the benefits of the TXQs
to the 'long tail' of simple drivers that don't do any internal
buffering anyway, without having to change the drivers to use 'pull
mode'. Am I wrong in thinking that mwl8k may be a good candidate for
such a layer? From glancing through the existing driver it looks like
it's mostly just taking each frame, wrapping it in a HW descriptor, and
sticking it on a TX ring?

>> What hardware is it you're writing a driver for, BTW, and are you
>> planning to upstream it? :)
>
> that's a rewrite of the mwl8k driver targeting the same hardware, but
> with a different firmware interface.
>
> if I can bring it on par with the existing set of supported hardware
> and features, I could try to upstream it yes.

That would be awesome! :)

-Toke