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
