Skip to contentSkip to conversation

ath9k: limit multicast buffer hardware queue depth

PATCH In review Felix Fietkau 2 participants 3 messages Jun 3, 2013 – Jun 5, 2013 Qualcomm Atheros ATH9K wireless driver

lore
Share & tools
lore

Felix Fietkau

author
#0 lore raw

PATCH 1/1 ath9k: limit multicast buffer hardware queue depth

The CAB (Content after Beacon) queue is used for beacon-triggered transmission of buffered multicast frames. If lots of multicast frames were buffered and this queue fills up, it drowns out all regular traffic. To limit the damage that buffered traffic can do, try to limit the queued data to becaon_interval / 8.

Signed-off-by Felix Fietkau <[email protected]>
---
 drivers/net/wireless/ath/ath9k/ath9k.h  | 1 +
 drivers/net/wireless/ath/ath9k/beacon.c | 6 ++++++
 drivers/net/wireless/ath/ath9k/xmit.c   | 3 +++
 3 files changed, 10 insertions(+)
drivers/net/wireless/ath/ath9k/ath9k.h +1 -0
index bd35a54..fb57d2f 100644
@@ -413,6 +413,7 @@ struct ath_beacon {
 	struct ath_descdma bdma;
 	struct ath_txq *cabq;
 	struct list_head bbuf;
+	int cabq_dur;
 
 	bool tx_processed;
 	bool tx_last;
drivers/net/wireless/ath/ath9k/beacon.c +6 -0
index fd1eeba..8a0aa22 100644
@@ -205,9 +205,15 @@ static struct ath_buf *ath9k_beacon_generate(struct ieee80211_hw *hw,
 	}
 
 	ath9k_beacon_setup(sc, vif, bf, info->control.rates[0].idx);
+	sc->beacon.cabq_dur = 0;
 
 	while (skb) {
 		ath9k_tx_cabq(hw, skb);
+
+		if (sc->beacon.cabq_dur / 1000 - 1 >
+		    sc->cur_beacon_conf.beacon_interval / ATH_BCBUF)
+			break;
+
 		skb = ieee80211_get_buffered_bc(hw, vif);
 	}
drivers/net/wireless/ath/ath9k/xmit.c +3 -0
index d701098..5c20b3d 100644
@@ -1156,6 +1156,9 @@ static void ath_tx_fill_desc(struct ath_softc *sc, struct ath_buf *bf,
 				info.flags |= ATH9K_TXDESC_LDPC;
 
 			ath_buf_set_rate(sc, bf, &info, len);
+
+			if (txq == sc->beacon.cabq)
+				sc->beacon.cabq_dur += info.rates[0].PktDuration;
 		}
 
 		info.buf_addr[0] = bf->bf_buf_addr;
Signature · 1 line
1.8.0.2
#1 lore raw
Quoted 5 lines · Felix Fietkau

The CAB (Content after Beacon) queue is used for beacon-triggered transmission of buffered multicast frames. If lots of multicast frames were buffered and this queue fills up, it drowns out all regular traffic. To limit the damage that buffered traffic can do, try to limit the queued data to becaon_interval / 8.

I'm not sure this would be compliant with the standard, but I guess something along these lines could be reasonable in some cases. However, it could be useful to take into account different DTIM Period parameters in the limit and instead of hardcoding this to one eight of the Beacon interval, the limit could be set based on Beacon interval * DTIM Period. Especially with large DTIM Period values, one eight of a Beacon interval may not be sufficient to handle even reasonable amount of group addressed frames.

Does this commit address More Data field updates when the driver decides to stop getting more frames without notifying mac80211 of this? The associated STAs would need to know when they can go back to sleep after the DTIM Beacon and the More Data field needs to be set to zero in the last frame the AP is sending out in the case this new limit is hit.

drivers/net/wireless/ath/ath9k/beacon.c static struct ath_buf *ath9k_beacon_generate(struct ieee80211_hw *hw, · line 214

Quoted 12 lines
drivers/net/wireless/ath/ath9k/beacon.c +5 -0
@@ -205,9 +205,15 @@ static struct ath_buf *ath9k_beacon_generate(struct ieee80211_hw *hw,
 
 	while (skb) {
 		ath9k_tx_cabq(hw, skb);
+
+		if (sc->beacon.cabq_dur / 1000 - 1 >
+		    sc->cur_beacon_conf.beacon_interval / ATH_BCBUF)
+			break;
+
 		skb = ieee80211_get_buffered_bc(hw, vif);
 	}

mac80211 handles the More Data field values based on what is actually buffered. I don't see how this patch would fix that either by modifying mac80211 to know that no more buffered frames are sent or by having the driver update the frame.

Signature · 1 line
Jouni Malinen                                            PGP id EFC895FA
author
#2 lore raw
Quoted 7 lines · Jouni Malinen
Quoted 5 lines · Felix Fietkau

The CAB (Content after Beacon) queue is used for beacon-triggered transmission of buffered multicast frames. If lots of multicast frames were buffered and this queue fills up, it drowns out all regular traffic. To limit the damage that buffered traffic can do, try to limit the queued data to becaon_interval / 8.

Quoted 8 lines

I'm not sure this would be compliant with the standard, but I guess something along these lines could be reasonable in some cases. However, it could be useful to take into account different DTIM Period parameters in the limit and instead of hardcoding this to one eight of the Beacon interval, the limit could be set based on Beacon interval * DTIM Period. Especially with large DTIM Period values, one eight of a Beacon interval may not be sufficient to handle even reasonable amount of group addressed frames.

Makes sense.

Quoted 5 lines

Does this commit address More Data field updates when the driver decides to stop getting more frames without notifying mac80211 of this? The associated STAs would need to know when they can go back to sleep after the DTIM Beacon and the More Data field needs to be set to zero in the last frame the AP is sending out in the case this new limit is hit.

I'll make a new version of this patch that takes care of the More Data field.

- Felix