[RFC 0/3] some AP PS buffering rework

[RFC 0/3] some AP PS buffering rework

[Johannes Berg]

I was looking at AP side PS buffering for uAPSD
and found a number of things that need to be
improved first. This is what I've come up with
so far, comments would be appreciated.

Thanks everyone who already looked at this when
I asked them on IRC :-)

So far, it's pretty much untested.

johannes

[RFC 1/3] mac80211: let drivers inform it about per TID buffered frames

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

For uAPSD implementation, it is necessary to know on
which ACs frames are buffered. mac80211 obviously
knows about the frames it has buffered itself, but
with aggregation many drivers buffer frames. Thus,
mac80211 needs to be informed about this.

For now, since we don't have APSD in any form, this
will unconditionally set the TIM bit for the station
but later with uAPSD only some ACs might cause the
TIM bit to be set.

ath9k is the only driver using this API and I only
modify it in the most basic way, it won't be able
to implement uAPSD with this yet. But it can't do
that anyway since there's no way to selectively
release frames to the peer yet.

Since drivers will buffer frames per TID, let them
inform mac80211 on a per TID basis, mac80211 will
then sort out the AC mapping itself.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wireless/ath/ath9k/ath9k.h |    3 ++-
 drivers/net/wireless/ath/ath9k/main.c  |    3 +--
 drivers/net/wireless/ath/ath9k/xmit.c  |   14 +++++++-------
 include/net/mac80211.h                 |   30 ++++++++++++++++++++++++------
 net/mac80211/sta_info.c                |    8 ++++++--
 5 files changed, 40 insertions(+), 18 deletions(-)

--- a/include/net/mac80211.h	2011-09-06 12:46:19.000000000 +0200
+++ b/include/net/mac80211.h	2011-09-06 12:49:15.000000000 +0200
@@ -2340,17 +2340,35 @@ static inline int ieee80211_sta_ps_trans
 #define IEEE80211_TX_STATUS_HEADROOM	13
 
 /**
- * ieee80211_sta_set_tim - set the TIM bit for a sleeping station
+ * ieee80211_sta_set_buffered - inform mac80211 about driver-buffered frames
  * @sta: &struct ieee80211_sta pointer for the sleeping station
+ * @tid: the TID that has buffered frames
+ * @buffered: indicates whether or not frames are buffered for this TID
  *
  * If a driver buffers frames for a powersave station instead of passing
- * them back to mac80211 for retransmission, the station needs to be told
- * to wake up using the TIM bitmap in the beacon.
+ * them back to mac80211 for retransmission, the station may still need
+ * to be told that there are buffered frames via the TIM bit.
  *
- * This function sets the station's TIM bit - it will be cleared when the
- * station wakes up.
+ * This function informs mac80211 whether or not there are frames that are
+ * buffered in the driver for a given TID; mac80211 can then use this data
+ * to set the TIM bit (NOTE: This may call back into the driver's set_tim
+ * call! Beware of the locking!)
+ *
+ * If all frames are released to the station (due to PS-poll or uAPSD)
+ * then the driver needs to inform mac80211 that there no longer are
+ * frames buffered. However, when the station wakes up mac80211 assumes
+ * that all buffered frames will be transmitted and clears this data,
+ * drivers need to make sure they inform mac80211 about all buffered
+ * frames on the sleep transition (sta_notify() with %STA_NOTIFY_SLEEP).
+ *
+ * Note that technically mac80211 only needs to know this per AC, not per
+ * TID, but since driver buffering will inevitably happen per TID (since
+ * it is related to aggregation) it is easier to make mac80211 map the
+ * TID to the AC as required instead of keeping track in all drivers that
+ * use this API.
  */
-void ieee80211_sta_set_tim(struct ieee80211_sta *sta);
+void ieee80211_sta_set_buffered(struct ieee80211_sta *sta,
+				u8 tid, bool buffered);
 
 /**
  * ieee80211_tx_status - transmit status callback
--- a/net/mac80211/sta_info.c	2011-09-06 12:48:06.000000000 +0200
+++ b/net/mac80211/sta_info.c	2011-09-06 12:49:15.000000000 +0200
@@ -1117,11 +1117,15 @@ void ieee80211_sta_block_awake(struct ie
 }
 EXPORT_SYMBOL(ieee80211_sta_block_awake);
 
-void ieee80211_sta_set_tim(struct ieee80211_sta *pubsta)
+void ieee80211_sta_set_buffered(struct ieee80211_sta *pubsta,
+				u8 tid, bool buffered)
 {
 	struct sta_info *sta = container_of(pubsta, struct sta_info, sta);
 
+	if (!buffered)
+		return;
+
 	set_sta_flags(sta, WLAN_STA_PS_DRIVER_BUF);
 	sta_info_set_tim_bit(sta);
 }
-EXPORT_SYMBOL(ieee80211_sta_set_tim);
+EXPORT_SYMBOL(ieee80211_sta_set_buffered);
--- a/drivers/net/wireless/ath/ath9k/ath9k.h	2011-09-06 12:46:19.000000000 +0200
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h	2011-09-06 12:49:15.000000000 +0200
@@ -343,7 +343,8 @@ void ath_tx_aggr_stop(struct ath_softc *
 void ath_tx_aggr_resume(struct ath_softc *sc, struct ieee80211_sta *sta, u16 tid);
 
 void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an);
-bool ath_tx_aggr_sleep(struct ath_softc *sc, struct ath_node *an);
+void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
+		       struct ath_node *an);
 
 /********/
 /* VIFs */
--- a/drivers/net/wireless/ath/ath9k/main.c	2011-09-06 12:46:19.000000000 +0200
+++ b/drivers/net/wireless/ath/ath9k/main.c	2011-09-06 12:49:15.000000000 +0200
@@ -1851,8 +1851,7 @@ static void ath9k_sta_notify(struct ieee
 	switch (cmd) {
 	case STA_NOTIFY_SLEEP:
 		an->sleeping = true;
-		if (ath_tx_aggr_sleep(sc, an))
-			ieee80211_sta_set_tim(sta);
+		ath_tx_aggr_sleep(sta, sc, an);
 		break;
 	case STA_NOTIFY_AWAKE:
 		an->sleeping = false;
--- a/drivers/net/wireless/ath/ath9k/xmit.c	2011-09-06 12:46:19.000000000 +0200
+++ b/drivers/net/wireless/ath/ath9k/xmit.c	2011-09-06 12:49:15.000000000 +0200
@@ -545,7 +545,7 @@ static void ath_tx_complete_aggr(struct
 	/* prepend un-acked frames to the beginning of the pending frame queue */
 	if (!list_empty(&bf_pending)) {
 		if (an->sleeping)
-			ieee80211_sta_set_tim(sta);
+			ieee80211_sta_set_buffered(sta, tid->tidno, true);
 
 		spin_lock_bh(&txq->axq_lock);
 		if (clear_filter)
@@ -933,12 +933,13 @@ void ath_tx_aggr_stop(struct ath_softc *
 	ath_tx_flush_tid(sc, txtid);
 }
 
-bool ath_tx_aggr_sleep(struct ath_softc *sc, struct ath_node *an)
+void ath_tx_aggr_sleep(struct ieee80211_sta *sta, struct ath_softc *sc,
+		       struct ath_node *an)
 {
 	struct ath_atx_tid *tid;
 	struct ath_atx_ac *ac;
 	struct ath_txq *txq;
-	bool buffered = false;
+	bool buffered;
 	int tidno;
 
 	for (tidno = 0, tid = &an->tid[tidno];
@@ -952,8 +953,7 @@ bool ath_tx_aggr_sleep(struct ath_softc
 
 		spin_lock_bh(&txq->axq_lock);
 
-		if (!list_empty(&tid->buf_q))
-			buffered = true;
+		buffered = !list_empty(&tid->buf_q);
 
 		tid->sched = false;
 		list_del(&tid->list);
@@ -964,9 +964,9 @@ bool ath_tx_aggr_sleep(struct ath_softc
 		}
 
 		spin_unlock_bh(&txq->axq_lock);
-	}
 
-	return buffered;
+		ieee80211_sta_set_buffered(sta, tidno, buffered);
+	}
 }
 
 void ath_tx_aggr_wakeup(struct ath_softc *sc, struct ath_node *an)

[RFC 2/3] mac80211: unify TIM bit handling

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Currently, the TIM bit for a given station is set
and cleared all over the place. Since the logic to
set/clear it will become much more complex when we
add uAPSD support, as a first step let's collect
the entire logic in one place. This requires a few
small adjustments to other places.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/sta_info.c |   94 +++++++++++++++++-------------------------------
 net/mac80211/sta_info.h |    3 -
 net/mac80211/status.c   |    1 
 net/mac80211/tx.c       |    9 +---
 4 files changed, 39 insertions(+), 68 deletions(-)

--- a/net/mac80211/sta_info.h	2011-09-06 12:46:18.000000000 +0200
+++ b/net/mac80211/sta_info.h	2011-09-06 12:49:39.000000000 +0200
@@ -518,8 +518,7 @@ int sta_info_destroy_addr(struct ieee802
 int sta_info_destroy_addr_bss(struct ieee80211_sub_if_data *sdata,
 			      const u8 *addr);
 
-void sta_info_set_tim_bit(struct sta_info *sta);
-void sta_info_clear_tim_bit(struct sta_info *sta);
+void sta_info_recalc_tim(struct sta_info *sta);
 
 void sta_info_init(struct ieee80211_local *local);
 void sta_info_stop(struct ieee80211_local *local);
--- a/net/mac80211/tx.c	2011-09-06 12:46:19.000000000 +0200
+++ b/net/mac80211/tx.c	2011-09-06 12:49:39.000000000 +0200
@@ -470,13 +470,10 @@ ieee80211_tx_h_unicast_ps_buf(struct iee
 			tx->local->total_ps_buffered++;
 
 		/*
-		 * Queue frame to be sent after STA wakes up/polls,
-		 * but don't set the TIM bit if the driver is blocking
-		 * wakeup or poll response transmissions anyway.
+		 * We queued up some frames, so the TIM bit might
+		 * need to be set, recalculate it.
 		 */
-		if (skb_queue_empty(&sta->ps_tx_buf) &&
-		    !(staflags & WLAN_STA_PS_DRIVER))
-			sta_info_set_tim_bit(sta);
+		sta_info_recalc_tim(sta);
 
 		info->control.jiffies = jiffies;
 		info->control.vif = &tx->sdata->vif;
--- a/net/mac80211/sta_info.c	2011-09-06 12:49:15.000000000 +0200
+++ b/net/mac80211/sta_info.c	2011-09-06 12:59:05.000000000 +0200
@@ -641,54 +641,35 @@ static inline void __bss_tim_clear(struc
 	bss->tim[aid / 8] &= ~(1 << (aid % 8));
 }
 
-static void __sta_info_set_tim_bit(struct ieee80211_if_ap *bss,
-				   struct sta_info *sta)
-{
-	BUG_ON(!bss);
-
-	__bss_tim_set(bss, sta->sta.aid);
-
-	if (sta->local->ops->set_tim) {
-		sta->local->tim_in_locked_section = true;
-		drv_set_tim(sta->local, &sta->sta, true);
-		sta->local->tim_in_locked_section = false;
-	}
-}
-
-void sta_info_set_tim_bit(struct sta_info *sta)
+void sta_info_recalc_tim(struct sta_info *sta)
 {
+	struct ieee80211_local *local = sta->local;
+	struct ieee80211_if_ap *bss = sta->sdata->bss;
 	unsigned long flags;
+	bool have_data;
 
-	BUG_ON(!sta->sdata->bss);
-
-	spin_lock_irqsave(&sta->local->sta_lock, flags);
-	__sta_info_set_tim_bit(sta->sdata->bss, sta);
-	spin_unlock_irqrestore(&sta->local->sta_lock, flags);
-}
-
-static void __sta_info_clear_tim_bit(struct ieee80211_if_ap *bss,
-				     struct sta_info *sta)
-{
-	BUG_ON(!bss);
+	/* No need to do anything if the driver does all */
+	if (local->hw.flags & IEEE80211_HW_AP_LINK_PS)
+		return;
 
-	__bss_tim_clear(bss, sta->sta.aid);
+	spin_lock_irqsave(&local->sta_lock, flags);
 
-	if (sta->local->ops->set_tim) {
-		sta->local->tim_in_locked_section = true;
-		drv_set_tim(sta->local, &sta->sta, false);
-		sta->local->tim_in_locked_section = false;
+	have_data = test_sta_flags(sta, WLAN_STA_PS_DRIVER_BUF) ||
+		    !skb_queue_empty(&sta->tx_filtered) ||
+		    !skb_queue_empty(&sta->ps_tx_buf);
+
+	if (have_data)
+		__bss_tim_set(bss, sta->sta.aid);
+	else
+		__bss_tim_clear(bss, sta->sta.aid);
+
+	if (local->ops->set_tim) {
+		local->tim_in_locked_section = true;
+		drv_set_tim(local, &sta->sta, have_data);
+		local->tim_in_locked_section = false;
 	}
-}
 
-void sta_info_clear_tim_bit(struct sta_info *sta)
-{
-	unsigned long flags;
-
-	BUG_ON(!sta->sdata->bss);
-
-	spin_lock_irqsave(&sta->local->sta_lock, flags);
-	__sta_info_clear_tim_bit(sta->sdata->bss, sta);
-	spin_unlock_irqrestore(&sta->local->sta_lock, flags);
+	spin_unlock_irqrestore(&local->sta_lock, flags);
 }
 
 static bool sta_info_buffer_expired(struct sta_info *sta, struct sk_buff *skb)
@@ -736,9 +717,9 @@ static bool sta_info_cleanup_expire_buff
 #endif
 		dev_kfree_skb(skb);
 
-		if (skb_queue_empty(&sta->ps_tx_buf) &&
-		    !test_sta_flags(sta, WLAN_STA_PS_DRIVER_BUF))
-			sta_info_clear_tim_bit(sta);
+		/* if the queue is now empty recalc TIM bit */
+		if (skb_queue_empty(&sta->ps_tx_buf))
+			sta_info_recalc_tim(sta);
 	}
 
 	return !skb_queue_empty(&sta->ps_tx_buf);
@@ -748,7 +729,6 @@ static int __must_check __sta_info_destr
 {
 	struct ieee80211_local *local;
 	struct ieee80211_sub_if_data *sdata;
-	struct sk_buff *skb;
 	unsigned long flags;
 	int ret, i;
 
@@ -787,12 +767,16 @@ static int __must_check __sta_info_destr
 
 	sta->dead = true;
 
+	local->total_ps_buffered -= skb_queue_len(&sta->ps_tx_buf);
+	__skb_queue_purge(&sta->ps_tx_buf);
+	__skb_queue_purge(&sta->tx_filtered);
+
 	if (test_and_clear_sta_flags(sta,
 				WLAN_STA_PS_STA | WLAN_STA_PS_DRIVER)) {
 		BUG_ON(!sdata->bss);
 
 		atomic_dec(&sdata->bss->num_sta_ps);
-		sta_info_clear_tim_bit(sta);
+		sta_info_recalc_tim(sta);
 	}
 
 	local->num_sta--;
@@ -840,14 +824,6 @@ static int __must_check __sta_info_destr
 	}
 #endif
 
-	while ((skb = skb_dequeue(&sta->ps_tx_buf)) != NULL) {
-		local->total_ps_buffered--;
-		dev_kfree_skb_any(skb);
-	}
-
-	while ((skb = skb_dequeue(&sta->tx_filtered)) != NULL)
-		dev_kfree_skb_any(skb);
-
 	__sta_info_free(local, sta);
 
 	return 0;
@@ -1027,9 +1003,6 @@ void ieee80211_sta_ps_deliver_wakeup(str
 	if (!(local->hw.flags & IEEE80211_HW_AP_LINK_PS))
 		drv_sta_notify(local, sdata, STA_NOTIFY_AWAKE, &sta->sta);
 
-	if (!skb_queue_empty(&sta->ps_tx_buf))
-		sta_info_clear_tim_bit(sta);
-
 	/* Send all buffered frames to the station */
 	sent = ieee80211_add_pending_skbs(local, &sta->tx_filtered);
 	buffered = ieee80211_add_pending_skbs_fn(local, &sta->ps_tx_buf,
@@ -1037,6 +1010,8 @@ void ieee80211_sta_ps_deliver_wakeup(str
 	sent += buffered;
 	local->total_ps_buffered -= buffered;
 
+	sta_info_recalc_tim(sta);
+
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 	printk(KERN_DEBUG "%s: STA %pM aid %d sending %d filtered/%d PS frames "
 	       "since STA not sleeping anymore\n", sdata->name,
@@ -1086,8 +1061,7 @@ void ieee80211_sta_ps_deliver_poll_respo
 
 		ieee80211_add_pending_skb(local, skb);
 
-		if (no_pending_pkts)
-			sta_info_clear_tim_bit(sta);
+		sta_info_recalc_tim(sta);
 #ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
 	} else {
 		/*
@@ -1126,6 +1100,6 @@ void ieee80211_sta_set_buffered(struct i
 		return;
 
 	set_sta_flags(sta, WLAN_STA_PS_DRIVER_BUF);
-	sta_info_set_tim_bit(sta);
+	sta_info_recalc_tim(sta);
 }
 EXPORT_SYMBOL(ieee80211_sta_set_buffered);
--- a/net/mac80211/status.c	2011-09-06 13:14:35.000000000 +0200
+++ b/net/mac80211/status.c	2011-09-06 13:15:01.000000000 +0200
@@ -106,6 +106,7 @@ static void ieee80211_handle_filtered_fr
 	if (test_sta_flags(sta, WLAN_STA_PS_STA) &&
 	    skb_queue_len(&sta->tx_filtered) < STA_MAX_TX_BUFFER) {
 		skb_queue_tail(&sta->tx_filtered, skb);
+		sta_info_recalc_tim(sta);
 		return;
 	}
 

[RFC 3/3] mac80211: also expire filtered frames

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

mac80211 will expire normal PS-buffered frames, but
if the device rejected some frames for a sleeping
station, these won't be on the ps_tx_buf queue but
on the tx_filtered queue instead; this is done to
avoid reordering.

However, mac80211 will not expire frames from the
filtered queue, let's fix that.

Also add a more comments to what all this expiry is
doing and how it works.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/sta_info.c |   57 +++++++++++++++++++++++++++++++++++++++++++-----
 net/mac80211/status.c   |    5 ++++
 2 files changed, 57 insertions(+), 5 deletions(-)

--- a/net/mac80211/sta_info.c	2011-09-06 13:19:20.000000000 +0200
+++ b/net/mac80211/sta_info.c	2011-09-06 13:57:30.000000000 +0200
@@ -698,6 +698,39 @@ static bool sta_info_cleanup_expire_buff
 	unsigned long flags;
 	struct sk_buff *skb;
 
+	/*
+	 * First check for frames that should expire on the filtered
+	 * queue. Frames here were rejected by the driver and are on
+	 * a separate queue to avoid reordering with normal PS-buffered
+	 * frames. They also aren't accounted for right now in the
+	 * total_ps_buffered counter.
+	 */
+	for (;;) {
+		spin_lock_irqsave(&sta->tx_filtered.lock, flags);
+		skb = skb_peek(&sta->tx_filtered);
+		if (sta_info_buffer_expired(sta, skb))
+			skb = __skb_dequeue(&sta->tx_filtered);
+		else
+			skb = NULL;
+		spin_unlock_irqrestore(&sta->tx_filtered.lock, flags);
+
+		/*
+		 * Frames are queued in order, so if this one
+		 * hasn't expired yet we can stop testing. If
+		 * we actually reached the end of the queue we
+		 * also need to stop, of course.
+		 */
+		if (!skb)
+			break;
+		dev_kfree_skb(skb);
+	}
+
+	/*
+	 * Now also check the normal PS-buffered queue, this will
+	 * only find something if the filtered queue was emptied
+	 * since the filtered frames are all before the normal PS
+	 * buffered frames.
+	 */
 	for (;;) {
 		spin_lock_irqsave(&sta->ps_tx_buf.lock, flags);
 		skb = skb_peek(&sta->ps_tx_buf);
@@ -707,6 +740,11 @@ static bool sta_info_cleanup_expire_buff
 			skb = NULL;
 		spin_unlock_irqrestore(&sta->ps_tx_buf.lock, flags);
 
+		/*
+		 * frames are queued in order, so if this one
+		 * hasn't expired yet (or we reached the end of
+		 * the queue) we can stop testing
+		 */
 		if (!skb)
 			break;
 
@@ -716,13 +754,22 @@ static bool sta_info_cleanup_expire_buff
 		       sta->sta.addr);
 #endif
 		dev_kfree_skb(skb);
-
-		/* if the queue is now empty recalc TIM bit */
-		if (skb_queue_empty(&sta->ps_tx_buf))
-			sta_info_recalc_tim(sta);
 	}
 
-	return !skb_queue_empty(&sta->ps_tx_buf);
+	/*
+	 * Finally, recalculate the TIM bit for this station -- it might
+	 * now be clear because the station was too slow to retrieve its
+	 * frames.
+	 */
+	sta_info_recalc_tim(sta);
+
+	/*
+	 * Return whether there are any frames still buffered, this is
+	 * used to check whether the cleanup timer still needs to run,
+	 * if there are no frames we don't need to rearm the timer.
+	 */
+	return !(skb_queue_empty(&sta->ps_tx_buf) &&
+		 skb_queue_empty(&sta->tx_filtered));
 }
 
 static int __must_check __sta_info_destroy(struct sta_info *sta)
--- a/net/mac80211/status.c	2011-09-06 13:24:28.000000000 +0200
+++ b/net/mac80211/status.c	2011-09-06 13:29:24.000000000 +0200
@@ -107,6 +107,11 @@ static void ieee80211_handle_filtered_fr
 	    skb_queue_len(&sta->tx_filtered) < STA_MAX_TX_BUFFER) {
 		skb_queue_tail(&sta->tx_filtered, skb);
 		sta_info_recalc_tim(sta);
+
+		if (!timer_pending(&local->sta_cleanup))
+			mod_timer(&local->sta_cleanup,
+				  round_jiffies(jiffies +
+						STA_INFO_CLEANUP_INTERVAL));
 		return;
 	}
 

Re: [RFC 2/3] mac80211: unify TIM bit handling

[Johannes Berg]

> +void sta_info_recalc_tim(struct sta_info *sta)
>  {

> -	__bss_tim_clear(bss, sta->sta.aid);
> +	spin_lock_irqsave(&local->sta_lock, flags);
>  
> -	if (sta->local->ops->set_tim) {
> -		sta->local->tim_in_locked_section = true;
> -		drv_set_tim(sta->local, &sta->sta, false);
> -		sta->local->tim_in_locked_section = false;
> +	have_data = test_sta_flags(sta, WLAN_STA_PS_DRIVER_BUF) ||
> +		    !skb_queue_empty(&sta->tx_filtered) ||
> +		    !skb_queue_empty(&sta->ps_tx_buf);

I'll move the locking to after the have_data check, it's not needed
around it.

johannes