Search Linux Wireless

Re: [PATCH v2] wifi: mac80211: integrate PS buffering into iTXQ

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22.11.22 22:25, Johannes Berg wrote:
On Wed, 2022-11-02 at 19:40 +0100, Alexander Wetzel wrote:

+++ b/net/mac80211/sta_info.c
@@ -114,6 +114,28 @@ static int link_sta_info_hash_del(struct ieee80211_local *local,
  			       link_sta_rht_params);
  }
+static void num_sta_ps_dec_for_mc_txq(struct ieee80211_sub_if_data *sdata,
+				      atomic_t *num_sta_ps)

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


Makes sense, I'll change that.

+		/* 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?


ieee80211_sta_ps_deliver_wakeup() has the same comment/call just without local_bh_disable()

Having the comment for the function which may call wake_tx_queue() makes more sense for me. But I'll move the comment some lines up.

@@ -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;
- indicate_tim |= !skb_queue_empty(&sta->tx_filtered[ac]) ||
-				!skb_queue_empty(&sta->ps_tx_buf[ac]);
+		indicate_tim |= !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?


Yes, looks odd in the patch. But this is only the "early" indicate_tim set.
If there are no filtered frame the code continues.
And the existing code is already checking if we have anything in the iTXQ and then sets the the variable.

Here a "commented" sniped of the relevant code:

        for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) {
                unsigned long tids;

                if (ignore_for_tim & ieee80211_ac_to_qos_mask[ac])
                        continue;

                indicate_tim |= !skb_queue_empty(&sta->tx_filtered[ac]);

                if (indicate_tim)	
			//if true we don't have to check (more) iTXQs
                        break;

                tids = ieee80211_tids_for_ac(ac);

                indicate_tim |=
                        sta->driver_buffered_tids & tids;

		// This will set indicate_tim if anything is in the iTXQ
                indicate_tim |=
                        sta->txq_buffered_tids & tids;
        }

@@ -1553,23 +1547,11 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
BUILD_BUG_ON(BITS_TO_LONGS(IEEE80211_NUM_TIDS) > 1);
  	sta->driver_buffered_tids = 0;
-	sta->txq_buffered_tids = 0;
if (!ieee80211_hw_check(&local->hw, AP_LINK_PS))
  		drv_sta_notify(local, sdata, STA_NOTIFY_AWAKE, &sta->sta);
- for (i = 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 = 0; ac < IEEE80211_NUM_ACS; ac++) {
  		int count = skb_queue_len(&pending), tmp;
@@ -1579,16 +1561,39 @@ void ieee80211_sta_ps_deliver_wakeup(struct sta_info *sta)
  		tmp = skb_queue_len(&pending);
  		filtered += tmp - count;
  		count = 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 = skb_queue_len(&pending);
-		buffered += tmp - count;
  	}
+ 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?


Agree. That has somehow slipped to a wrong position. Will move that back.


+	for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
+		if (!sta->sta.txq[i])
+			continue;
+
+		txqi = 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?


That is a complicate question:-)

Short answer is, that this is already broken and this patch is not fixing it. The transmit logic for pending frames is not changed.
And yes, pending tx path is not (yet) using iTXQs.

The patch has a - probably incomplete - drive-by fix and reorders the actions to first try to send out pending/filtered frames. (I commented on that shortly in the v1 patch.)

Old logic was: Send out iTXQ, send filtered, send buffered.
New logic is: Send out filtered, send iTXQ.

When taking back a step and looking at how pending frames are handled in more detail there is no difference to the current code: ieee80211_add_pending_skbs() is still simply adding the skbs to local->pending. (The original code is doing that for filtered and buffered frames, the patched one only for filtered.)

These frames are then send via the tx_pending_tasklet. Which gets scheduled in __ieee80211_wake_queue() and thus directly with the ieee80211_add_pending_skbs() call.

In other words:
Calling ieee80211_add_pending_skbs() will schedule a run for tx_pending_tasklet to send out the frames via ieee80211_tx_pending_skb() and ultimately using drv_tx(). Not using iTXQs.

But I do not see how the code makes sure that the tasklet run happens prior to dequeuing frames from the iTXQs. So that's probably another bug.

I can also look into that. Either by starting to use iTXQs also for filtered frames or a more simple stop gap solution for now.

All assuming I read understand the code right and don't miss a clever way the current code still ends up doing the right thing...

Alexander

johannes




[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux