LinuxLists.cc - [PATCH] wifi: mac80211: Use internal TX queues for all drivers

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

Alexander Wetzel <[email protected]> writes:

> Align TX handling and use mac80211 internal TX queues (iTXQs) for
> drivers not implementing the optional .wake_tx_queue operation.
>
> mac80211 will handle the iTXQs and push frames via the drv_tx operation
> when a driver is not implementing .wake_tx_queue.
>
> As a side effect this converts all netdev interfaces created by mac80211
> to be non-queuing (using qdisc noqueue).
>
> Signed-off-by: Alexander Wetzel <[email protected]>

Great to see this! I'm actually surprised it doesn't take more code than
this. A few minor-ish comments below:

[...]

> diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
> index 9f4377566c42..cde169038429 100644
> --- a/net/mac80211/trace.h
> +++ b/net/mac80211/trace.h
> @@ -2301,37 +2301,6 @@ TRACE_EVENT(drv_tdls_recv_channel_switch,
> )
> );
>
> -TRACE_EVENT(drv_wake_tx_queue,
> - TP_PROTO(struct ieee80211_local *local,
> - struct ieee80211_sub_if_data *sdata,
> - struct txq_info *txq),
> -
> - TP_ARGS(local, sdata, txq),
> -
> - TP_STRUCT__entry(
> - LOCAL_ENTRY
> - VIF_ENTRY
> - STA_ENTRY
> - __field(u8, ac)
> - __field(u8, tid)
> - ),
> -
> - TP_fast_assign(
> - struct ieee80211_sta *sta = txq->txq.sta;
> -
> - LOCAL_ASSIGN;
> - VIF_ASSIGN;
> - STA_ASSIGN;
> - __entry->ac = txq->txq.ac;
> - __entry->tid = txq->txq.tid;
> - ),
> -
> - TP_printk(
> - LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " ac:%d tid:%d",
> - LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->ac, __entry->tid
> - )
> -);
> -
> TRACE_EVENT(drv_get_ftm_responder_stats,
> TP_PROTO(struct ieee80211_local *local,
> struct ieee80211_sub_if_data *sdata,
> @@ -3026,6 +2995,37 @@ TRACE_EVENT(stop_queue,
> )
> );
>
> +TRACE_EVENT(wake_tx_queue,

I know tracepoints are technically not considered API, but that doesn't
mean we *have* to break them if there's no reason to. And since the
actual contents is the same, how about keeping the old name as an alias
for this?

> + TP_PROTO(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + struct txq_info *txq),
> +
> + TP_ARGS(local, sdata, txq),
> +
> + TP_STRUCT__entry(
> + LOCAL_ENTRY
> + VIF_ENTRY
> + STA_ENTRY
> + __field(u8, ac)
> + __field(u8, tid)
> + ),
> +
> + TP_fast_assign(
> + struct ieee80211_sta *sta = txq->txq.sta;
> +
> + LOCAL_ASSIGN;
> + VIF_ASSIGN;
> + STA_ASSIGN;
> + __entry->ac = txq->txq.ac;
> + __entry->tid = txq->txq.tid;
> + ),
> +
> + TP_printk(
> + LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " ac:%d tid:%d",
> + LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->ac, __entry->tid
> + )
> +);
> +
> #endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */
>
> #undef TRACE_INCLUDE_PATH
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 48deda3570a7..f4dc1ddbe2ea 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1599,9 +1599,6 @@ int ieee80211_txq_setup_flows(struct ieee80211_local *local)
> bool supp_vht = false;
> enum nl80211_band band;
>
> - if (!local->ops->wake_tx_queue)
> - return 0;
> -
> ret = fq_init(fq, 4096);
> if (ret)
> return ret;
> @@ -1649,9 +1646,6 @@ void ieee80211_txq_teardown_flows(struct ieee80211_local *local)
> {
> struct fq *fq = &local->fq;
>
> - if (!local->ops->wake_tx_queue)
> - return;
> -
> kfree(local->cvars);
> local->cvars = NULL;
>
> @@ -1668,8 +1662,7 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
> struct ieee80211_vif *vif;
> struct txq_info *txqi;
>
> - if (!local->ops->wake_tx_queue ||
> - sdata->vif.type == NL80211_IFTYPE_MONITOR)
> + if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
> return false;
>
> if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> @@ -4184,12 +4177,7 @@ void __ieee80211_subif_start_xmit(struct sk_buff *skb,
> if (IS_ERR(sta))
> sta = NULL;
>
> - if (local->ops->wake_tx_queue) {
> - u16 queue = __ieee80211_select_queue(sdata, sta, skb);
> - skb_set_queue_mapping(skb, queue);
> - skb_get_hash(skb);

This skb_get_hash is there to ensure that the hash is calculated early
(in particular, before packets are encrypted). This improves the
behaviour of the FQ, since that will just use the skb->hash value if
it's set. In most cases the operation here will be a noop because the
packet was already hashed somewhere in the ingress path, but I think we
should probably keep this call intact anyway...

> - }
> -
> + skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, sta, skb));
> ieee80211_aggr_check(sdata, sta, skb);
>
> sk_pacing_shift_update(skb->sk, sdata->local->hw.tx_sk_pacing_shift);
> @@ -4495,11 +4483,7 @@ static void ieee80211_8023_xmit(struct ieee80211_sub_if_data *sdata,
> struct tid_ampdu_tx *tid_tx;
> u8 tid;
>
> - if (local->ops->wake_tx_queue) {
> - u16 queue = __ieee80211_select_queue(sdata, sta, skb);
> - skb_set_queue_mapping(skb, queue);
> - skb_get_hash(skb);

As above.

-Toke

2022-09-29 21:09:08

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

Thanks for doing this!

It's a bit bad timing right now, so I'll only have a chance to look at
this in the next couple of weeks, but that also doesn't matter that much
since 6.0 release is imminent (will likely be Sunday).

Just wanted to comment on this one thing for now:

On Mon, 2022-09-26 at 18:13 +0200, Alexander Wetzel wrote:
> A short look at the in-tree drivers seems to confirm, that mac80211
> drivers are indeed not directly calling netif_stop_queue(). Which is no
> longer undesired but outright wrong after this patch.
> So I *think* we should be fine on that front, too.
>

They really couldn't, they don't have (easy) access to the netdev. There
are ways of getting to the netdev, but they're (intentionally) difficult
and doing that would've been wrong even previously since mac80211 has
its own reasons for stopping netdev queues sometimes.

So unless a driver is already broken, this can't be an issue.

> I did not try very hard to identify now obsolete code fragments and
> kept this as simple as possible.

That's nice enough for now, do you want to do follow-ups to clean up
more?

> I've also have a rough draft to move PS (multicast and unicast) to iTXQ
> we should look at later. That seems to be way more invasive than what we
> do here. But once PS also has been sorted out moving everything to iTXQ
> should be straight forward.

Ohhh! I'm excited about this :-)

johannes

2022-09-30 07:27:28

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

<resend to all, sorry for duplicates >

On 29.09.22 16:23, Toke Høiland-Jørgensen wrote:

[...]
>> diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
>> index 9f4377566c42..cde169038429 100644
>> --- a/net/mac80211/trace.h
>> +++ b/net/mac80211/trace.h
>> @@ -2301,37 +2301,6 @@ TRACE_EVENT(drv_tdls_recv_channel_switch,
>> )
>> );
>>
>> -TRACE_EVENT(drv_wake_tx_queue,
>> - TP_PROTO(struct ieee80211_local *local,
>> - struct ieee80211_sub_if_data *sdata,
>> - struct txq_info *txq),
>> -
>> - TP_ARGS(local, sdata, txq),
>> -
>> - TP_STRUCT__entry(
>> - LOCAL_ENTRY
>> - VIF_ENTRY
>> - STA_ENTRY
>> - __field(u8, ac)
>> - __field(u8, tid)
>> - ),
>> -
>> - TP_fast_assign(
>> - struct ieee80211_sta *sta = txq->txq.sta;
>> -
>> - LOCAL_ASSIGN;
>> - VIF_ASSIGN;
>> - STA_ASSIGN;
>> - __entry->ac = txq->txq.ac;
>> - __entry->tid = txq->txq.tid;
>> - ),
>> -
>> - TP_printk(
>> - LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " ac:%d tid:%d",
>> - LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->ac, __entry->tid
>> - )
>> -);
>> -
>> TRACE_EVENT(drv_get_ftm_responder_stats,
>> TP_PROTO(struct ieee80211_local *local,
>> struct ieee80211_sub_if_data *sdata,
>> @@ -3026,6 +2995,37 @@ TRACE_EVENT(stop_queue,
>> )
>> );
>>
>> +TRACE_EVENT(wake_tx_queue,
>
> I know tracepoints are technically not considered API, but that doesn't
> mean we *have* to break them if there's no reason to. And since the
> actual contents is the same, how about keeping the old name as an alias
> for this?
>

I don't understand what we would gain by an alias.
For me it looks like an alias would just be confusing and never be used...

Or why would anyone want to make additional calls to
trace[_drv]_wake_tx_queue() on top what we have in the patch?

I initially also considered to simply keep trace_drv_wake_tx_queue().
(But that looked to be misleading.) If there is some reason to keep the
old name I would just switch back to trace_drv_wake_tx_queue().

But when we are discussing that code anyhow, there is another related issue:
I was not sure if it's ok to keep the renamed wake_tx_queue trace call
in the old position. (In the section otherwise only having driver
calls.) Having a better structured file did seem more desirable than a
shorter patch, so I moved it.

[...]
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 48deda3570a7..f4dc1ddbe2ea 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1599,9 +1599,6 @@ int ieee80211_txq_setup_flows(struct ieee80211_local *local)
>> bool supp_vht = false;
>> enum nl80211_band band;
>>
>> - if (!local->ops->wake_tx_queue)
>> - return 0;
>> -
>> ret = fq_init(fq, 4096);
>> if (ret)
>> return ret;
>> @@ -1649,9 +1646,6 @@ void ieee80211_txq_teardown_flows(struct ieee80211_local *local)
>> {
>> struct fq *fq = &local->fq;
>>
>> - if (!local->ops->wake_tx_queue)
>> - return;
>> -
>> kfree(local->cvars);
>> local->cvars = NULL;
>>
>> @@ -1668,8 +1662,7 @@ static bool ieee80211_queue_skb(struct ieee80211_local *local,
>> struct ieee80211_vif *vif;
>> struct txq_info *txqi;
>>
>> - if (!local->ops->wake_tx_queue ||
>> - sdata->vif.type == NL80211_IFTYPE_MONITOR)
>> + if (sdata->vif.type == NL80211_IFTYPE_MONITOR)
>> return false;
>>
>> if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
>> @@ -4184,12 +4177,7 @@ void __ieee80211_subif_start_xmit(struct sk_buff *skb,
>> if (IS_ERR(sta))
>> sta = NULL;
>>
>> - if (local->ops->wake_tx_queue) {
>> - u16 queue = __ieee80211_select_queue(sdata, sta, skb);
>> - skb_set_queue_mapping(skb, queue);
>> - skb_get_hash(skb);
>
> This skb_get_hash is there to ensure that the hash is calculated early
> (in particular, before packets are encrypted). This improves the
> behaviour of the FQ, since that will just use the skb->hash value if
> it's set. In most cases the operation here will be a noop because the
> packet was already hashed somewhere in the ingress path, but I think we
> should probably keep this call intact anyway...
>

Got it and I'll change that in v2. I'll wait for more feedback some days
- or at least till we have sorted out the issue with
trace_drv_wake_tx_queue alias above.

[...]

Thanks,
Alexander

2022-09-30 09:10:17

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

On 29.09.22 22:40, Johannes Berg wrote:
> Thanks for doing this!
>
> It's a bit bad timing right now, so I'll only have a chance to look at
> this in the next couple of weeks, but that also doesn't matter that much
> since 6.0 release is imminent (will likely be Sunday).
>

Thanks for the warning. I'll then send a v2 to correct skb_get_hash()
handling in the next days.

> Just wanted to comment on this one thing for now:
>
> On Mon, 2022-09-26 at 18:13 +0200, Alexander Wetzel wrote:
>> A short look at the in-tree drivers seems to confirm, that mac80211
>> drivers are indeed not directly calling netif_stop_queue(). Which is no
>> longer undesired but outright wrong after this patch.
>> So I *think* we should be fine on that front, too.
>>
>
> They really couldn't, they don't have (easy) access to the netdev. There
> are ways of getting to the netdev, but they're (intentionally) difficult
> and doing that would've been wrong even previously since mac80211 has
> its own reasons for stopping netdev queues sometimes.
>
> So unless a driver is already broken, this can't be an issue.
>
>> I did not try very hard to identify now obsolete code fragments and
>> kept this as simple as possible.
>
> That's nice enough for now, do you want to do follow-ups to clean up
> more?
>

The general plan so far is, to move everything to iTXQ first and then
see what we an throw out. I'll prepare a patch cleaning up that a bit
more. Don't think that will be much at this stage, so maybe I just add
that into the v2 of the patch...

I'm basically working with the plan you outlined some years ago:
https://lore.kernel.org/linux-wireless/[email protected]/

It just turned out to be simpler to start with unifying the TX paths.

>> I've also have a rough draft to move PS (multicast and unicast) to iTXQ
>> we should look at later. That seems to be way more invasive than what we
>> do here. But once PS also has been sorted out moving everything to iTXQ
>> should be straight forward.
>
> Ohhh! I'm excited about this :-)
> > johannes

Alexander

2022-09-30 09:46:40

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

On Fri, 2022-09-30 at 11:08 +0200, Alexander Wetzel wrote:
> On 29.09.22 22:40, Johannes Berg wrote:
> > Thanks for doing this!
> >
> > It's a bit bad timing right now, so I'll only have a chance to look at
> > this in the next couple of weeks, but that also doesn't matter that much
> > since 6.0 release is imminent (will likely be Sunday).
> >
>
> Thanks for the warning. I'll then send a v2 to correct skb_get_hash()
> handling in the next days.

I probably won't merge it much before ~2 weeks from now, so take your
time :)

> The general plan so far is, to move everything to iTXQ first and then
> see what we an throw out. I'll prepare a patch cleaning up that a bit
> more. Don't think that will be much at this stage, so maybe I just add
> that into the v2 of the patch...

No, separate patches are fine - was more wondering if you were planning
to do it, or if I should think about finding someone else ;-)

> I'm basically working with the plan you outlined some years ago:
> https://lore.kernel.org/linux-wireless/[email protected]/

:)
I can't even remember it that much, to be honest.

> It just turned out to be simpler to start with unifying the TX paths.

Makes sense.

Thanks!

johannes

2022-09-30 11:21:21

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

Alexander Wetzel <[email protected]> writes:

> <resend to all, sorry for duplicates >
>
> On 29.09.22 16:23, Toke Høiland-Jørgensen wrote:
>
> [...]
>>> diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
>>> index 9f4377566c42..cde169038429 100644
>>> --- a/net/mac80211/trace.h
>>> +++ b/net/mac80211/trace.h
>>> @@ -2301,37 +2301,6 @@ TRACE_EVENT(drv_tdls_recv_channel_switch,
>>> )
>>> );
>>>
>>> -TRACE_EVENT(drv_wake_tx_queue,
>>> - TP_PROTO(struct ieee80211_local *local,
>>> - struct ieee80211_sub_if_data *sdata,
>>> - struct txq_info *txq),
>>> -
>>> - TP_ARGS(local, sdata, txq),
>>> -
>>> - TP_STRUCT__entry(
>>> - LOCAL_ENTRY
>>> - VIF_ENTRY
>>> - STA_ENTRY
>>> - __field(u8, ac)
>>> - __field(u8, tid)
>>> - ),
>>> -
>>> - TP_fast_assign(
>>> - struct ieee80211_sta *sta = txq->txq.sta;
>>> -
>>> - LOCAL_ASSIGN;
>>> - VIF_ASSIGN;
>>> - STA_ASSIGN;
>>> - __entry->ac = txq->txq.ac;
>>> - __entry->tid = txq->txq.tid;
>>> - ),
>>> -
>>> - TP_printk(
>>> - LOCAL_PR_FMT VIF_PR_FMT STA_PR_FMT " ac:%d tid:%d",
>>> - LOCAL_PR_ARG, VIF_PR_ARG, STA_PR_ARG, __entry->ac, __entry->tid
>>> - )
>>> -);
>>> -
>>> TRACE_EVENT(drv_get_ftm_responder_stats,
>>> TP_PROTO(struct ieee80211_local *local,
>>> struct ieee80211_sub_if_data *sdata,
>>> @@ -3026,6 +2995,37 @@ TRACE_EVENT(stop_queue,
>>> )
>>> );
>>>
>>> +TRACE_EVENT(wake_tx_queue,
>>
>> I know tracepoints are technically not considered API, but that doesn't
>> mean we *have* to break them if there's no reason to. And since the
>> actual contents is the same, how about keeping the old name as an alias
>> for this?
>>
>
> I don't understand what we would gain by an alias.
> For me it looks like an alias would just be confusing and never be used...
>
> Or why would anyone want to make additional calls to
> trace[_drv]_wake_tx_queue() on top what we have in the patch?
>
> I initially also considered to simply keep trace_drv_wake_tx_queue().
> (But that looked to be misleading.) If there is some reason to keep the
> old name I would just switch back to trace_drv_wake_tx_queue().

Well, the tracepoint is something that is read from userspace by
programs that want to trace the stack. The current one lives in
/sys/kernel/tracing/events/mac80211/drv_wake_tx_queue/

So if you rename it, any debug/tracing tooling that uses this will have
to be updated to use the new name. Which is not an ABI problem per se
(we exempt tracepoints from that), but it's also annoying if anyone *is*
actually using that tracepoint, so no reason to break things unless
there's really a compelling reason to...

I also thought the tracing subsystem had an alias mechanism built-in to
handle this sort of thing, but I think I may be misremembering this part.

> But when we are discussing that code anyhow, there is another related issue:
> I was not sure if it's ok to keep the renamed wake_tx_queue trace call
> in the old position. (In the section otherwise only having driver
> calls.) Having a better structured file did seem more desirable than a
> shorter patch, so I moved it.

I think I would lean towards just keeping it in the old position with
the old name for the reasons outlined above... This is not an incredibly
strong opinion, though, so if anyone has stronger opinions in the other
direction I can probably be convinced ;)

-Toke

2022-10-05 12:30:11

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

On Mon, 2022-09-26 at 18:13 +0200, Alexander Wetzel wrote:

> - trace_drv_wake_tx_queue(local, sdata, txq);

Technically, I guess we could keep both tracepoints, but it'd be kind of
pointless since we know statically which driver does which...

> @@ -596,21 +598,18 @@ __sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>
> sta->last_connected = ktime_get_seconds();
>
> - if (local->ops->wake_tx_queue) {
> - void *txq_data;
> - int size = sizeof(struct txq_info) +
> - ALIGN(hw->txq_data_size, sizeof(void *));
> + size = sizeof(struct txq_info) +
> + ALIGN(hw->txq_data_size, sizeof(void *));
>
> - txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> - if (!txq_data)
> - goto free;
> + txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> + if (!txq_data)
> + goto free;
>
> - for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> - struct txq_info *txq = txq_data + i * size;
> + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> + struct txq_info *txq = txq_data + i * size;
>
> - /* might not do anything for the bufferable MMPDU TXQ */
> - ieee80211_txq_init(sdata, sta, txq, i);
> - }
> + /* might not do anything for the bufferable MMPDU TXQ */
> + ieee80211_txq_init(sdata, sta, txq, i);

Is that comment still true?

> +++ b/net/mac80211/util.c
> @@ -288,6 +288,64 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw,
> }
> EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>
> +static void wake_tx_push_queue(struct ieee80211_local *local,
> + struct ieee80211_sub_if_data *sdata,
> + struct ieee80211_txq *queue)
> +{
> + int q = sdata->vif.hw_queue[queue->ac];
> + struct ieee80211_tx_control control = {};
> + struct sk_buff *skb;
> + unsigned long flags;
> + bool q_stopped;
> +
> + control.sta = queue->sta;
> +
> + while (1) {
> + spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> + q_stopped = local->queue_stop_reasons[q];
> + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> +
> + if (q_stopped)
> + break;
> +
> + skb = ieee80211_tx_dequeue(&local->hw, queue);
> + if (!skb)
> + break;
> +
> + drv_tx(local, &control, skb);
> + }
> +}
> +
> +void wake_tx_queue(struct ieee80211_local *local, struct txq_info *txq)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(txq->txq.vif);
> + struct ieee80211_txq *queue;
> +
> + /* In reconfig don't transmit now, but mark for waking later */
> + if (local->in_reconfig) {
> + set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txq->flags);
> + return;
> + }
> +
> + if (!check_sdata_in_driver(sdata))
> + return;
> +
> + trace_wake_tx_queue(local, sdata, txq);
> +
> + if (local->ops->wake_tx_queue) {
> + drv_wake_tx_queue(local, txq);
> + return;
> + }
> +
> + /* Driver has no native support for iTXQ, handle the queues */
> + ieee80211_txq_schedule_start(&local->hw, txq->txq.ac);
> + while ((queue = ieee80211_next_txq(&local->hw, txq->txq.ac))) {
> + wake_tx_push_queue(local, sdata, queue);
> + ieee80211_return_txq(&local->hw, queue, false);
> + }
> + ieee80211_txq_schedule_end(&local->hw, txq->txq.ac);
> +}

Here's another thought:

Since this code is basically all moved from the original
drv_wake_tx_queue(), except for the "else" portion (after the if/return)
of it, another thing we could do is to just have an exported function
that does this:

void ieee80211_handle_wake_tx_queue(struct ieee80211_hw *hw,
struct ieee80211_txq *txq)
{
... *local = from_hw(hw);
... *sdata = from_vif(txq->vif);

wake_tx_push_queue(local, sdata, txq);
}

Actually ... I wonder why you'd here - in waking a single TXQ - use
ieee80211_next_txq() at all, Toke, what do you think?

Anyway, then we could require drivers set wake_txq to
ieee80211_handle_wake_tx_queue and make sure in main.c that
wake_tx_queue is non-NULL.

That's a bit more churn in drivers, but:
* it's not really that hard to do
* it avoids an extra function call to then jump to the op
* it avoids the tracing changes since now it does look like a driver
wake_tx_queue callback

What do you think?

johannes

2022-10-05 12:37:26

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

Johannes Berg <[email protected]> writes:

> On Mon, 2022-09-26 at 18:13 +0200, Alexander Wetzel wrote:
>
>> - trace_drv_wake_tx_queue(local, sdata, txq);
>
> Technically, I guess we could keep both tracepoints, but it'd be kind of
> pointless since we know statically which driver does which...
>
>> @@ -596,21 +598,18 @@ __sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>>
>> sta->last_connected = ktime_get_seconds();
>>
>> - if (local->ops->wake_tx_queue) {
>> - void *txq_data;
>> - int size = sizeof(struct txq_info) +
>> - ALIGN(hw->txq_data_size, sizeof(void *));
>> + size = sizeof(struct txq_info) +
>> + ALIGN(hw->txq_data_size, sizeof(void *));
>>
>> - txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
>> - if (!txq_data)
>> - goto free;
>> + txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
>> + if (!txq_data)
>> + goto free;
>>
>> - for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
>> - struct txq_info *txq = txq_data + i * size;
>> + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
>> + struct txq_info *txq = txq_data + i * size;
>>
>> - /* might not do anything for the bufferable MMPDU TXQ */
>> - ieee80211_txq_init(sdata, sta, txq, i);
>> - }
>> + /* might not do anything for the bufferable MMPDU TXQ */
>> + ieee80211_txq_init(sdata, sta, txq, i);
>
> Is that comment still true?
>
>> +++ b/net/mac80211/util.c
>> @@ -288,6 +288,64 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw,
>> }
>> EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>>
>> +static void wake_tx_push_queue(struct ieee80211_local *local,
>> + struct ieee80211_sub_if_data *sdata,
>> + struct ieee80211_txq *queue)
>> +{
>> + int q = sdata->vif.hw_queue[queue->ac];
>> + struct ieee80211_tx_control control = {};
>> + struct sk_buff *skb;
>> + unsigned long flags;
>> + bool q_stopped;
>> +
>> + control.sta = queue->sta;
>> +
>> + while (1) {
>> + spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
>> + q_stopped = local->queue_stop_reasons[q];
>> + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>> +
>> + if (q_stopped)
>> + break;
>> +
>> + skb = ieee80211_tx_dequeue(&local->hw, queue);
>> + if (!skb)
>> + break;
>> +
>> + drv_tx(local, &control, skb);
>> + }
>> +}
>> +
>> +void wake_tx_queue(struct ieee80211_local *local, struct txq_info *txq)
>> +{
>> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(txq->txq.vif);
>> + struct ieee80211_txq *queue;
>> +
>> + /* In reconfig don't transmit now, but mark for waking later */
>> + if (local->in_reconfig) {
>> + set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txq->flags);
>> + return;
>> + }
>> +
>> + if (!check_sdata_in_driver(sdata))
>> + return;
>> +
>> + trace_wake_tx_queue(local, sdata, txq);
>> +
>> + if (local->ops->wake_tx_queue) {
>> + drv_wake_tx_queue(local, txq);
>> + return;
>> + }
>> +
>> + /* Driver has no native support for iTXQ, handle the queues */
>> + ieee80211_txq_schedule_start(&local->hw, txq->txq.ac);
>> + while ((queue = ieee80211_next_txq(&local->hw, txq->txq.ac))) {
>> + wake_tx_push_queue(local, sdata, queue);
>> + ieee80211_return_txq(&local->hw, queue, false);
>> + }
>> + ieee80211_txq_schedule_end(&local->hw, txq->txq.ac);
>> +}
>
> Here's another thought:
>
> Since this code is basically all moved from the original
> drv_wake_tx_queue(), except for the "else" portion (after the if/return)
> of it, another thing we could do is to just have an exported function
> that does this:
>
> void ieee80211_handle_wake_tx_queue(struct ieee80211_hw *hw,
> struct ieee80211_txq *txq)
> {
> ... *local = from_hw(hw);
> ... *sdata = from_vif(txq->vif);
>
> wake_tx_push_queue(local, sdata, txq);
> }
>
> Actually ... I wonder why you'd here - in waking a single TXQ - use
> ieee80211_next_txq() at all, Toke, what do you think?

Well, this patch does almost exactly the same as the ath9k driver does,
for instance. Really, the wake_tx_queue() is a signal to the driver to
start transmitting on the *hardware* queue that the txq points to. For
some drivers (like Intel, right?) that's a 1-to-1 mapping, for others
there are multiple TXQs being scheduled on the same HW-TXQ. So I think
it's probably the right thing to do to just call next_txq(); if there's
only a single TXQ scheduled it should be pretty cheap to do so.

This logic has implications for putting "urgent" frames (like PS(?)) on
TXQs as well, of course, but that needs to be handled somehow anyway...

> Anyway, then we could require drivers set wake_txq to
> ieee80211_handle_wake_tx_queue and make sure in main.c that
> wake_tx_queue is non-NULL.
>
> That's a bit more churn in drivers, but:
> * it's not really that hard to do
> * it avoids an extra function call to then jump to the op
> * it avoids the tracing changes since now it does look like a driver
> wake_tx_queue callback
>
> What do you think?

Sounds reasonable :)

-Toke

2022-10-05 12:44:52

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

On Wed, 2022-10-05 at 14:26 +0200, Toke Høiland-Jørgensen wrote:

> > void ieee80211_handle_wake_tx_queue(struct ieee80211_hw *hw,
> > struct ieee80211_txq *txq)
> > {
> > ... *local = from_hw(hw);
> > ... *sdata = from_vif(txq->vif);
> >
> > wake_tx_push_queue(local, sdata, txq);
> > }
> >
> > Actually ... I wonder why you'd here - in waking a single TXQ - use
> > ieee80211_next_txq() at all, Toke, what do you think?
>
> Well, this patch does almost exactly the same as the ath9k driver does,
> for instance. Really, the wake_tx_queue() is a signal to the driver to
> start transmitting on the *hardware* queue that the txq points to. For
> some drivers (like Intel, right?) that's a 1-to-1 mapping, for others
> there are multiple TXQs being scheduled on the same HW-TXQ. So I think
> it's probably the right thing to do to just call next_txq(); if there's
> only a single TXQ scheduled it should be pretty cheap to do so.

Oh OK. So then the logic Alexander had makes sense.

>
> This logic has implications for putting "urgent" frames (like PS(?)) on
> TXQs as well, of course, but that needs to be handled somehow anyway...

But that probably then anyway needs to be handled in next_txq()?

johannes

2022-10-05 14:50:48

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

Johannes Berg <[email protected]> writes:

> On Wed, 2022-10-05 at 14:26 +0200, Toke Høiland-Jørgensen wrote:
>
>> > void ieee80211_handle_wake_tx_queue(struct ieee80211_hw *hw,
>> > struct ieee80211_txq *txq)
>> > {
>> > ... *local = from_hw(hw);
>> > ... *sdata = from_vif(txq->vif);
>> >
>> > wake_tx_push_queue(local, sdata, txq);
>> > }
>> >
>> > Actually ... I wonder why you'd here - in waking a single TXQ - use
>> > ieee80211_next_txq() at all, Toke, what do you think?
>>
>> Well, this patch does almost exactly the same as the ath9k driver does,
>> for instance. Really, the wake_tx_queue() is a signal to the driver to
>> start transmitting on the *hardware* queue that the txq points to. For
>> some drivers (like Intel, right?) that's a 1-to-1 mapping, for others
>> there are multiple TXQs being scheduled on the same HW-TXQ. So I think
>> it's probably the right thing to do to just call next_txq(); if there's
>> only a single TXQ scheduled it should be pretty cheap to do so.
>
> Oh OK. So then the logic Alexander had makes sense.

Yup, I think so :)

>>
>> This logic has implications for putting "urgent" frames (like PS(?)) on
>> TXQs as well, of course, but that needs to be handled somehow anyway...
>
> But that probably then anyway needs to be handled in next_txq()?

Yeah, just meant that comment as an "for future reference", it doesn't
impact this patch series (I think?)

-Toke

2022-10-05 18:17:47

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

On Wed, 2022-10-05 at 16:43 +0200, Toke Høiland-Jørgensen wrote:
> > >
> > > This logic has implications for putting "urgent" frames (like PS(?)) on
> > > TXQs as well, of course, but that needs to be handled somehow anyway...
> >
> > But that probably then anyway needs to be handled in next_txq()?
>
> Yeah, just meant that comment as an "for future reference", it doesn't
> impact this patch series (I think?)
>

Right now, no, but with the promised patch to make powersave use TXQs it
would, I think :)

johannes

2022-10-06 11:47:57

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

Johannes Berg <[email protected]> writes:

> On Wed, 2022-10-05 at 16:43 +0200, Toke Høiland-Jørgensen wrote:
>> > >
>> > > This logic has implications for putting "urgent" frames (like PS(?)) on
>> > > TXQs as well, of course, but that needs to be handled somehow anyway...
>> >
>> > But that probably then anyway needs to be handled in next_txq()?
>>
>> Yeah, just meant that comment as an "for future reference", it doesn't
>> impact this patch series (I think?)
>>
>
> Right now, no, but with the promised patch to make powersave use TXQs it
> would, I think :)

Yup, hence why I wanted to give this "head's up" in advance so everyone
is aware :)

-Toke

2022-10-06 16:12:15

[permalink] [raw]

Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers

On 05.10.22 13:39, Johannes Berg wrote:
> On Mon, 2022-09-26 at 18:13 +0200, Alexander Wetzel wrote:
>
>> - trace_drv_wake_tx_queue(local, sdata, txq);
>
> Technically, I guess we could keep both tracepoints, but it'd be kind of
> pointless since we know statically which driver does which...
>
>> @@ -596,21 +598,18 @@ __sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>>
>> sta->last_connected = ktime_get_seconds();
>>
>> - if (local->ops->wake_tx_queue) {
>> - void *txq_data;
>> - int size = sizeof(struct txq_info) +
>> - ALIGN(hw->txq_data_size, sizeof(void *));
>> + size = sizeof(struct txq_info) +
>> + ALIGN(hw->txq_data_size, sizeof(void *));
>>
>> - txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
>> - if (!txq_data)
>> - goto free;
>> + txq_data = kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
>> + if (!txq_data)
>> + goto free;
>>
>> - for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
>> - struct txq_info *txq = txq_data + i * size;
>> + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
>> + struct txq_info *txq = txq_data + i * size;
>>
>> - /* might not do anything for the bufferable MMPDU TXQ */
>> - ieee80211_txq_init(sdata, sta, txq, i);
>> - }
>> + /* might not do anything for the bufferable MMPDU TXQ */
>> + ieee80211_txq_init(sdata, sta, txq, i);
>
> Is that comment still true?
>

Pretty sure, yes. This patch is not changing anything related to that.
It could change when we are able to handle PS with iTXQ, though. (So far
only mvm has this queue, no other driver)

Since that is/was a tricky construct for me:
For my understanding this is the non-data queue for stations and the
comment is a bit misleading:
The queue is theoretical also accepting some non-bufferable frames when
the interface is in NL80211_IFTYPE_STATION mode. But since an AP never
sleeps we accept these frames here, too.

Or in other words:
We can't put a MPDU in any iTXQ which may have to be send out to a
sleeping station. But when our interface is a manged BSS member we know
that the remote sta (AP) never sleeps, allowing us to use this queue for
more frame types.

>> +++ b/net/mac80211/util.c
>> @@ -288,6 +288,64 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw,
>> }
>> EXPORT_SYMBOL(ieee80211_ctstoself_duration);
>>
>> +static void wake_tx_push_queue(struct ieee80211_local *local,
>> + struct ieee80211_sub_if_data *sdata,
>> + struct ieee80211_txq *queue)
>> +{
>> + int q = sdata->vif.hw_queue[queue->ac];
>> + struct ieee80211_tx_control control = {};
>> + struct sk_buff *skb;
>> + unsigned long flags;
>> + bool q_stopped;
>> +
>> + control.sta = queue->sta;
>> +
>> + while (1) {
>> + spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
>> + q_stopped = local->queue_stop_reasons[q];
>> + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>> +
>> + if (q_stopped)
>> + break;
>> +
>> + skb = ieee80211_tx_dequeue(&local->hw, queue);
>> + if (!skb)
>> + break;
>> +
>> + drv_tx(local, &control, skb);
>> + }
>> +}
>> +
>> +void wake_tx_queue(struct ieee80211_local *local, struct txq_info *txq)
>> +{
>> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(txq->txq.vif);
>> + struct ieee80211_txq *queue;
>> +
>> + /* In reconfig don't transmit now, but mark for waking later */
>> + if (local->in_reconfig) {
>> + set_bit(IEEE80211_TXQ_STOP_NETIF_TX, &txq->flags);
>> + return;
>> + }
>> +
>> + if (!check_sdata_in_driver(sdata))
>> + return;
>> +
>> + trace_wake_tx_queue(local, sdata, txq);
>> +
>> + if (local->ops->wake_tx_queue) {
>> + drv_wake_tx_queue(local, txq);
>> + return;
>> + }
>> +
>> + /* Driver has no native support for iTXQ, handle the queues */
>> + ieee80211_txq_schedule_start(&local->hw, txq->txq.ac);
>> + while ((queue = ieee80211_next_txq(&local->hw, txq->txq.ac))) {
>> + wake_tx_push_queue(local, sdata, queue);
>> + ieee80211_return_txq(&local->hw, queue, false);
>> + }
>> + ieee80211_txq_schedule_end(&local->hw, txq->txq.ac);
>> +}
>
> Here's another thought:
>
> Since this code is basically all moved from the original
> drv_wake_tx_queue(), except for the "else" portion (after the if/return)
> of it, another thing we could do is to just have an exported function
> that does this:
>
> void ieee80211_handle_wake_tx_queue(struct ieee80211_hw *hw,
> struct ieee80211_txq *txq)
> {
> ... *local = from_hw(hw);
> ... *sdata = from_vif(txq->vif);
>
> wake_tx_push_queue(local, sdata, txq);
> }
>
> Actually ... I wonder why you'd here - in waking a single TXQ - use
> ieee80211_next_txq() at all, Toke, what do you think?
>
>
> Anyway, then we could require drivers set wake_txq to
> ieee80211_handle_wake_tx_queue and make sure in main.c that
> wake_tx_queue is non-NULL.
>
> That's a bit more churn in drivers, but:
> * it's not really that hard to do
> * it avoids an extra function call to then jump to the op
> * it avoids the tracing changes since now it does look like a driver
> wake_tx_queue callback
>
> What do you think?
>

Makes sense: V2 will then have three patches:
1) adding ieee80211_handle_wake_tx_queue to mac80211
2) switch the drivers without iTXQ support to it
3) drop driver support for the old push path

> johannes

Alexander

2022-10-06 16:53:46