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: <20200505174947.GB2079@sakura>
References: <20200504193959.GC26805@sakura> <878si6oabp.fsf@toke.dk> <20200505131531.GA32619@sakura> <87368eo5dn.fsf@toke.dk> <20200505152010.GA33304@sakura> <87pnbimil6.fsf@toke.dk> <20200505174947.GB2079@sakura>
Date:   Tue, 05 May 2020 22:49:11 +0200
Message-ID: <87d07im7js.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 18:50:45 (+0200), Toke H=C3=B8iland-J=C3=B8=
rgensen wrote:
>
>> This seems like a bad idea; we want the TXQ mechanism to decide which
>> frame to send on wakeup.
>
> .release_buffered_frames() is only needed/used if STA went into
> powersave while packets were already sitting inside txqi, that's an
> edge case.
>
> In the other much more common case (STA went into sleep without any
> traffic pending in txqi), then the "classic" ps delivery code is used:
> frames gets pulled from ps_tx_buf queue (1 by 1 for ps poll, more for
> uapsd), and those frames ends up being sent through drv_tx(), since
> they have the flag IEEE80211_TX_CTRL_PS_RESPONSE so they bypass txqi.

Ah, I see, and if there are a lot of outstanding frames the client is
supposed to wake up and resume regular operation? As I said, I really
don't know much about how PS works; but I'm enjoying learning about it,
so thanks! :)

> so I was just looking at removing that edge case, sending those frames
> back to ps_tx_buf() from the driver.

Right, that makes sense. But I guess this is only something you can do
if you never buffer frames in the driver, no? E.g., ath9k has its own
internal retry queue, so it needs the callback to train that; and once
the callback is there, extending it to pull from the TXQs is quite
straight forward... So it's not necessarily a generally-applicable
optimisation, is what I mean.

>> 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 just want to give those back to mac80211, those frames were already
> in push mode anyway.

Gotcha.

>> 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?
>
> maybe with the current firmware interface, but with the new one
> aggregation is done on host side, so tx path is no more that simple.

Right, OK. Is this just a different firmware that's generally available,
or is it a new thing? I am generally a fan of moving logic out of the
firmware like this...

-Toke