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;
Message-ID: <96e9ad692842853cfe92a7e5de18136baf20a492.camel@sipsolutions.net>
Subject: Re: [PATCH] wifi: mac80211: Use internal TX queues for all drivers
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Alexander Wetzel <alexander@wetzel-home.de>,
        linux-wireless@vger.kernel.org
Cc:     Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= <toke@kernel.org>
Date:   Wed, 05 Oct 2022 13:39:43 +0200
In-Reply-To: <20220926161303.13035-1-alexander@wetzel-home.de>
References: <20220926161303.13035-1-alexander@wetzel-home.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

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 *sdat=
a,
> =20
>  	sta->last_connected =3D ktime_get_seconds();
> =20
> -	if (local->ops->wake_tx_queue) {
> -		void *txq_data;
> -		int size =3D sizeof(struct txq_info) +
> -			   ALIGN(hw->txq_data_size, sizeof(void *));
> +	size =3D sizeof(struct txq_info) +
> +	       ALIGN(hw->txq_data_size, sizeof(void *));
> =20
> -		txq_data =3D kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> -		if (!txq_data)
> -			goto free;
> +	txq_data =3D kcalloc(ARRAY_SIZE(sta->sta.txq), size, gfp);
> +	if (!txq_data)
> +		goto free;
> =20
> -		for (i =3D 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> -			struct txq_info *txq =3D txq_data + i * size;
> +	for (i =3D 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> +		struct txq_info *txq =3D txq_data + i * size;
> =20
> -			/* 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);
> =20
> +static void wake_tx_push_queue(struct ieee80211_local *local,
> +			       struct ieee80211_sub_if_data *sdata,
> +			       struct ieee80211_txq *queue)
> +{
> +	int q =3D sdata->vif.hw_queue[queue->ac];
> +	struct ieee80211_tx_control control =3D {};
> +	struct sk_buff *skb;
> +	unsigned long flags;
> +	bool q_stopped;
> +
> +	control.sta =3D queue->sta;
> +
> +	while (1) {
> +		spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> +		q_stopped =3D local->queue_stop_reasons[q];
> +		spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> +
> +		if (q_stopped)
> +			break;
> +
> +		skb =3D 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 =3D 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 =3D 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 =3D from_hw(hw);
	... *sdata =3D 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