[PATCH] ath9k: limit multicast buffer hardware queue depth

[PATCH] ath9k: limit multicast buffer hardware queue depth

[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.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 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(+)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index bd35a54..fb57d2f 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -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;
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index fd1eeba..8a0aa22 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -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);
 	}
 
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index d701098..5c20b3d 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -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;
-- 
1.8.0.2

Re: [PATCH] ath9k: limit multicast buffer hardware queue depth

[Jouni Malinen]

On Mon, Jun 03, 2013 at 07:31:54PM +0200, Felix Fietkau wrote:
> 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.

> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -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.
 
-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] ath9k: limit multicast buffer hardware queue depth

[Felix Fietkau]

On 2013-06-05 4:09 PM, Jouni Malinen wrote:
> On Mon, Jun 03, 2013 at 07:31:54PM +0200, Felix Fietkau wrote:
>> 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.
Makes sense.

> 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