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: <20200505131531.GA32619@sakura>
References: <20200504193959.GC26805@sakura> <878si6oabp.fsf@toke.dk> <20200505131531.GA32619@sakura>
Date:   Tue, 05 May 2020 15:53:08 +0200
Message-ID: <87368eo5dn.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 14:06:18 (+0200), Toke H=C3=B8iland-J=C3=B8=
rgensen wrote:
>
> Hello Toke,
>
> thanks for your comments
>
>> I don't think ieee80211_stop_queue() is meant to be used this way at all
>> in the wake_tx_queue case. Rather, when you get a wake_tx_queue()
>> callback, you just queue as many frames as you feel like (see '2
>> aggregate' limit above), and then return.  Then, on a TX completion you
>> just call your internal driver function that tries to pull more frames
>> from the mac80211 TXQs.
>
> alright, indeed .wake_tx_queue() does not have to result in any actual
> frame sent.
>
> but what about .release_buffered_frames then ?
>
> .release_buffered_frames() and .drv_tx() are both "push" oriented
> interface. When they are called, you have to push a frame to the
> hardware, so if they are called when hardware FIFO is full, you need
> to drop the frame (or add yet another intermediate level of queuing)
>
> from what I can see, .release_buffered_frames() will happily be called
> even if queue is stopped, and you have to at least send one frame.
>
> I'm just trying to avoid any additionnal intermediate queing in the
> driver between what mac80211 gives me and the HW fifo which has a
> fixed size (well actually you can choose the size, but only at init
> time).
>
> my current workaround is to pre-size the hw fifo queue to handle what
> I think is the worst case

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.

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

I'm probably not the right person to answer that; never did have a good
grip on the details of PS support.


What hardware is it you're writing a driver for, BTW, and are you
planning to upstream it? :)

-Toke