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: <19418837778881777379446e65938b455fbb523a.camel@sipsolutions.net>
Subject: Re: [PATCH v2] wifi: mac80211: integrate PS buffering into iTXQ
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Alexander Wetzel <alexander@wetzel-home.de>,
        linux-wireless@vger.kernel.org
Date:   Tue, 22 Nov 2022 22:25:38 +0100
In-Reply-To: <20221102184005.10649-1-alexander@wetzel-home.de>
References: <20221102184005.10649-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 Wed, 2022-11-02 at 19:40 +0100, Alexander Wetzel wrote:
>=20
> +++ b/net/mac80211/sta_info.c
> @@ -114,6 +114,28 @@ static int link_sta_info_hash_del(struct ieee80211_l=
ocal *local,
>  			       link_sta_rht_params);
>  }
> =20
> +static void num_sta_ps_dec_for_mc_txq(struct ieee80211_sub_if_data *sdat=
a,
> +				      atomic_t *num_sta_ps)

passing the pointer to the atomic_t variable seems a bit odd here, maybe
pass the 'ps' pointer here:

> +		/* can call wake_tx_queue for vif.txq */
> +		num_sta_ps_dec_for_mc_txq(sdata, &ps->num_sta_ps);

instead?

Also maybe that comment should be a couple of lines above on the
local_bh_disable() since that's the context requirement for that?

> @@ -1046,8 +1070,8 @@ static void __sta_info_recalc_tim(struct sta_info *=
sta, bool ignore_pending)
>  		if (ignore_for_tim & ieee80211_ac_to_qos_mask[ac])
>  			continue;
> =20
> -		indicate_tim |=3D !skb_queue_empty(&sta->tx_filtered[ac]) ||
> -				!skb_queue_empty(&sta->ps_tx_buf[ac]);
> +		indicate_tim |=3D !skb_queue_empty(&sta->tx_filtered[ac]);

This seems odd, we must indicate the TIM whenever we have *any* traffic
pending to that STA? IOW also if there's anything on the TXQs?

> @@ -1553,23 +1547,11 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_i=
nfo *sta)
> =20
>  	BUILD_BUG_ON(BITS_TO_LONGS(IEEE80211_NUM_TIDS) > 1);
>  	sta->driver_buffered_tids =3D 0;
> -	sta->txq_buffered_tids =3D 0;
> =20
>  	if (!ieee80211_hw_check(&local->hw, AP_LINK_PS))
>  		drv_sta_notify(local, sdata, STA_NOTIFY_AWAKE, &sta->sta);
> =20
> -	for (i =3D 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> -		if (!sta->sta.txq[i] || !txq_has_queue(sta->sta.txq[i]))
> -			continue;
> -
> -		schedule_and_wake_txq(local, to_txq_info(sta->sta.txq[i]));
> -	}
> -
> -	skb_queue_head_init(&pending);
> -
> -	/* sync with ieee80211_tx_h_unicast_ps_buf */
> -	spin_lock(&sta->ps_lock);
> -	/* Send all buffered frames to the station */
> +	/* Send all filtered frames to the station */
>  	for (ac =3D 0; ac < IEEE80211_NUM_ACS; ac++) {
>  		int count =3D skb_queue_len(&pending), tmp;
> =20
> @@ -1579,16 +1561,39 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_i=
nfo *sta)
>  		tmp =3D skb_queue_len(&pending);
>  		filtered +=3D tmp - count;
>  		count =3D tmp;
> -
> -		spin_lock_irqsave(&sta->ps_tx_buf[ac].lock, flags);
> -		skb_queue_splice_tail_init(&sta->ps_tx_buf[ac], &pending);
> -		spin_unlock_irqrestore(&sta->ps_tx_buf[ac].lock, flags);
> -		tmp =3D skb_queue_len(&pending);
> -		buffered +=3D tmp - count;
>  	}
> =20
> +	skb_queue_head_init(&pending);
>  	ieee80211_add_pending_skbs(local, &pending);

It seems to me you moved the skb_queue_head_init() erroneously, it's
still needed for the loop for the filtered frames?

If you reinit here you probably leak, and if it's not inited before you
probably crash?

> +	for (i =3D 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
> +		if (!sta->sta.txq[i])
> +			continue;
> +
> +		txqi =3D to_txq_info(sta->sta.txq[i]);
> +		clear_bit(IEEE80211_TXQ_STOP_PS, &txqi->flags);
> +	}

Should we really do this before pending are transmitted? Actually I'm a
bit confused about ieee80211_add_pending_skbs() now - does that mean
those frames don't go to the driver via TXQs?

johannes