Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@kernel.org>
To:     Alexander Wetzel <alexander@wetzel-home.de>,
        linux-wireless@vger.kernel.org
Cc:     Johannes Berg <johannes@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers
In-Reply-To: <20220926161303.13035-1-alexander@wetzel-home.de>
References: <20220926161303.13035-1-alexander@wetzel-home.de>
Date:   Thu, 29 Sep 2022 16:23:39 +0200
Message-ID: <87wn9ml0no.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Alexander Wetzel <alexander@wetzel-home.de> 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 <alexander@wetzel-home.de>

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