* pending queue depth in ieee80211_local data structure
@ 2010-03-18 10:12 Lorenzo Bianconi
2010-03-18 10:44 ` Bruno Randolf
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2010-03-18 10:12 UTC (permalink / raw)
To: linux-wireless
Hi all,
I noticed a possible issue in the pending queue management of the
ieee80211_local data structure.
In particular, there is no control of the queue depth and this could
cause a memory overflow.
In the test I carried out this happen when I use a low priority queue
(e.g. Backgreound queue) and
I transmit a data stream that exceeds the channel capacity (e.g.
50Mbps@MCS 3, 800ns GI and 20MHz
channel width). I wrote this patch in order to fix the issue.
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -703,6 +703,8 @@
struct work_struct sta_finish_work;
int sta_generation;
+ /* Pending buffer dimension */
+ #define PENDING_BUF 512
struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
struct tasklet_struct tx_pending_tasklet;
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1399,13 +1399,15 @@
skb = tx.skb;
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
-
+
if (local->queue_stop_reasons[queue] ||
!skb_queue_empty(&local->pending[queue])) {
/*
- * if queue is stopped, queue up frames for later
- * transmission from the tasklet
+ * if queue is stopped and there is enough space in the queue,
+ * queue up frames for later transmission from the tasklet
*/
+ if (skb_queue_len(&local->pending[queue]) >= PENDING_BUF)
+ goto drop;
do {
next = skb->next;
skb->next = NULL;
@@ -2028,8 +2030,12 @@
flags);
txok = ieee80211_tx_pending_skb(local, skb);
- if (!txok)
- __skb_queue_head(&local->pending[i], skb);
+ if (!txok) {
+ if (skb_queue_len(&local->pending[i]) < PENDING_BUF)
+ __skb_queue_head(&local->pending[i], skb);
+ else
+ kfree_skb(skb);
+ }
spin_lock_irqsave(&local->queue_stop_reason_lock,
flags);
if (!txok)
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -383,7 +383,10 @@
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
__ieee80211_stop_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
- __skb_queue_tail(&local->pending[queue], skb);
+ if (skb_queue_len(&local->pending[queue]) < PENDING_BUF)
+ __skb_queue_tail(&local->pending[queue], skb);
+ else
+ kfree_skb(skb);
__ieee80211_wake_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
}
@@ -409,9 +412,12 @@
continue;
}
- ret++;
queue = skb_get_queue_mapping(skb);
- __skb_queue_tail(&local->pending[queue], skb);
+ if (skb_queue_len(&local->pending[queue]) < PENDING_BUF) {
+ ret++;
+ __skb_queue_tail(&local->pending[queue], skb);
+ } else
+ kfree_skb(skb);
}
for (i = 0; i < hw->queues; i++)
Regards
Lorenzo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: pending queue depth in ieee80211_local data structure
2010-03-18 10:12 pending queue depth in ieee80211_local data structure Lorenzo Bianconi
@ 2010-03-18 10:44 ` Bruno Randolf
2010-03-18 11:35 ` Lorenzo Bianconi
0 siblings, 1 reply; 4+ messages in thread
From: Bruno Randolf @ 2010-03-18 10:44 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-wireless
On Thursday 18 March 2010 19:12:32 Lorenzo Bianconi wrote:
> Hi all,
>
> I noticed a possible issue in the pending queue management of the
> ieee80211_local data structure.
> In particular, there is no control of the queue depth and this could
> cause a memory overflow.
> In the test I carried out this happen when I use a low priority queue
> (e.g. Backgreound queue) and
> I transmit a data stream that exceeds the channel capacity (e.g.
> 50Mbps@MCS 3, 800ns GI and 20MHz
> channel width). I wrote this patch in order to fix the issue.
i think, i noticed the same issue: sending a UDP stream which is higher than
the possible bandwidth will eventually cause an out of memory panic.
bruno
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -703,6 +703,8 @@
> struct work_struct sta_finish_work;
> int sta_generation;
>
> + /* Pending buffer dimension */
> + #define PENDING_BUF 512
> struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
> struct tasklet_struct tx_pending_tasklet;
>
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1399,13 +1399,15 @@
> skb = tx.skb;
>
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> -
> +
> if (local->queue_stop_reasons[queue] ||
> !skb_queue_empty(&local->pending[queue])) {
> /*
> - * if queue is stopped, queue up frames for later
> - * transmission from the tasklet
> + * if queue is stopped and there is enough space in
the queue,
> + * queue up frames for later transmission from the
tasklet
> */
> + if (skb_queue_len(&local->pending[queue]) >=
PENDING_BUF)
> + goto drop;
> do {
> next = skb->next;
> skb->next = NULL;
> @@ -2028,8 +2030,12 @@
> flags);
>
> txok = ieee80211_tx_pending_skb(local, skb);
> - if (!txok)
> - __skb_queue_head(&local->pending[i], skb);
> + if (!txok) {
> + if (skb_queue_len(&local->pending[i]) <
PENDING_BUF)
> + __skb_queue_head(&local->pending[i],
skb);
> + else
> + kfree_skb(skb);
> + }
> spin_lock_irqsave(&local->queue_stop_reason_lock,
> flags);
> if (!txok)
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -383,7 +383,10 @@
>
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> __ieee80211_stop_queue(hw, queue,
IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
> - __skb_queue_tail(&local->pending[queue], skb);
> + if (skb_queue_len(&local->pending[queue]) < PENDING_BUF)
> + __skb_queue_tail(&local->pending[queue], skb);
> + else
> + kfree_skb(skb);
> __ieee80211_wake_queue(hw, queue,
IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
> spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> }
> @@ -409,9 +412,12 @@
> continue;
> }
>
> - ret++;
> queue = skb_get_queue_mapping(skb);
> - __skb_queue_tail(&local->pending[queue], skb);
> + if (skb_queue_len(&local->pending[queue]) < PENDING_BUF) {
> + ret++;
> + __skb_queue_tail(&local->pending[queue], skb);
> + } else
> + kfree_skb(skb);
> }
>
> for (i = 0; i < hw->queues; i++)
>
>
> Regards
>
> Lorenzo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: pending queue depth in ieee80211_local data structure
2010-03-18 10:44 ` Bruno Randolf
@ 2010-03-18 11:35 ` Lorenzo Bianconi
2010-03-18 12:56 ` Larry Finger
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2010-03-18 11:35 UTC (permalink / raw)
To: br1, ht6100; +Cc: linux-wireless
> On Thursday 18 March 2010 19:12:32 Lorenzo Bianconi wrote:
>> Hi all,
>>
>> I noticed a possible issue in the pending queue management of the
>> ieee80211_local data structure.
>> In particular, there is no control of the queue depth and this could
>> cause a memory overflow.
>> In the test I carried out this happen when I use a low priority queue
>> (e.g. Backgreound queue) and
>> I transmit a data stream that exceeds the channel capacity (e.g.
>> 50Mbps@MCS 3, 800ns GI and 20MHz
>> channel width). I wrote this patch in order to fix the issue.
>
> i think, i noticed the same issue: sending a UDP stream which is higher than
> the possible bandwidth will eventually cause an out of memory panic.
>
> bruno
>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>>
>> --- a/net/mac80211/ieee80211_i.h
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -703,6 +703,8 @@
>> struct work_struct sta_finish_work;
>> int sta_generation;
>>
>> + /* Pending buffer dimension */
>> + #define PENDING_BUF 512
>> struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
>> struct tasklet_struct tx_pending_tasklet;
>>
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -1399,13 +1399,15 @@
>> skb = tx.skb;
>>
>> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
>> -
>> +
>> if (local->queue_stop_reasons[queue] ||
>> !skb_queue_empty(&local->pending[queue])) {
>> /*
>> - * if queue is stopped, queue up frames for later
>> - * transmission from the tasklet
>> + * if queue is stopped and there is enough space in
> the queue,
>> + * queue up frames for later transmission from the
> tasklet
>> */
>> + if (skb_queue_len(&local->pending[queue]) >=
> PENDING_BUF)
>> + goto drop;
>> do {
>> next = skb->next;
>> skb->next = NULL;
>> @@ -2028,8 +2030,12 @@
>> flags);
>>
>> txok = ieee80211_tx_pending_skb(local, skb);
>> - if (!txok)
>> - __skb_queue_head(&local->pending[i], skb);
>> + if (!txok) {
>> + if (skb_queue_len(&local->pending[i]) <
> PENDING_BUF)
>> + __skb_queue_head(&local->pending[i],
> skb);
>> + else
>> + kfree_skb(skb);
>> + }
>> spin_lock_irqsave(&local->queue_stop_reason_lock,
>> flags);
>> if (!txok)
>> --- a/net/mac80211/util.c
>> +++ b/net/mac80211/util.c
>> @@ -383,7 +383,10 @@
>>
>> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
>> __ieee80211_stop_queue(hw, queue,
> IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
>> - __skb_queue_tail(&local->pending[queue], skb);
>> + if (skb_queue_len(&local->pending[queue]) < PENDING_BUF)
>> + __skb_queue_tail(&local->pending[queue], skb);
>> + else
>> + kfree_skb(skb);
>> __ieee80211_wake_queue(hw, queue,
> IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
>> spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>> }
>> @@ -409,9 +412,12 @@
>> continue;
>> }
>>
>> - ret++;
>> queue = skb_get_queue_mapping(skb);
>> - __skb_queue_tail(&local->pending[queue], skb);
>> + if (skb_queue_len(&local->pending[queue]) < PENDING_BUF) {
>> + ret++;
>> + __skb_queue_tail(&local->pending[queue], skb);
>> + } else
>> + kfree_skb(skb);
>> }
>>
>> for (i = 0; i < hw->queues; i++)
>>
>>
>> Regards
>>
>> Lorenzo
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
>> in the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi all,
I pasted the first version of the patch where I missed to unlock the
spinlock in the ieee80211_tx().
This is the last version of the patch.
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -708,6 +708,8 @@
struct work_struct sta_finish_work;
int sta_generation;
+ /* Pending buffer dimension */
+ #define PENDING_BUF 512
struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
struct tasklet_struct tx_pending_tasklet;
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1449,14 +1449,18 @@
skb = tx.skb;
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
-
+
if (local->queue_stop_reasons[queue] ||
!skb_queue_empty(&local->pending[queue])) {
/*
- * if queue is stopped, queue up frames for later
- * transmission from the tasklet
+ * if queue is stopped and there is enough space in the queue,
+ * queue up frames for later transmission from the tasklet
*/
- do {
+ if (skb_queue_len(&local->pending[queue]) >= PENDING_BUF) {
+ spin_unlock_irqrestore(&local->queue_stop_reason_lock,
+ flags);
+ goto drop;
+ } do {
next = skb->next;
skb->next = NULL;
if (unlikely(txpending))
@@ -2074,8 +2078,12 @@
flags);
txok = ieee80211_tx_pending_skb(local, skb);
- if (!txok)
- __skb_queue_head(&local->pending[i], skb);
+ if (!txok) {
+ if (skb_queue_len(&local->pending[i]) < PENDING_BUF)
+ __skb_queue_head(&local->pending[i], skb);
+ else
+ kfree_skb(skb);
+ }
spin_lock_irqsave(&local->queue_stop_reason_lock,
flags);
if (!txok)
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -383,7 +383,10 @@
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
__ieee80211_stop_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
- __skb_queue_tail(&local->pending[queue], skb);
+ if (skb_queue_len(&local->pending[queue]) < PENDING_BUF)
+ __skb_queue_tail(&local->pending[queue], skb);
+ else
+ kfree_skb(skb);
__ieee80211_wake_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
}
@@ -409,9 +412,12 @@
continue;
}
- ret++;
queue = skb_get_queue_mapping(skb);
- __skb_queue_tail(&local->pending[queue], skb);
+ if (skb_queue_len(&local->pending[queue]) < PENDING_BUF) {
+ ret++;
+ __skb_queue_tail(&local->pending[queue], skb);
+ } else
+ kfree_skb(skb);
}
for (i = 0; i < hw->queues; i++)
Regards.
Lorenzo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: pending queue depth in ieee80211_local data structure
2010-03-18 11:35 ` Lorenzo Bianconi
@ 2010-03-18 12:56 ` Larry Finger
0 siblings, 0 replies; 4+ messages in thread
From: Larry Finger @ 2010-03-18 12:56 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: br1, ht6100, linux-wireless
On 03/18/2010 06:35 AM, Lorenzo Bianconi wrote:
> Hi all,
>
> I pasted the first version of the patch where I missed to unlock the
> spinlock in the ieee80211_tx().
> This is the last version of the patch.
Probably not.
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
>
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -708,6 +708,8 @@
> struct work_struct sta_finish_work;
> int sta_generation;
>
> + /* Pending buffer dimension */
> + #define PENDING_BUF 512
> struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
> struct tasklet_struct tx_pending_tasklet;
>
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1449,14 +1449,18 @@
> skb = tx.skb;
>
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> -
> +
The new line here has trailing white space. I wondered why you were changing one
blank line for another. You should use scripts/checkpatch to verify your patch.
That script would have caught this.
> if (local->queue_stop_reasons[queue] ||
> !skb_queue_empty(&local->pending[queue])) {
> /*
> - * if queue is stopped, queue up frames for later
> - * transmission from the tasklet
> + * if queue is stopped and there is enough space in the queue,
> + * queue up frames for later transmission from the tasklet
> */
> - do {
> + if (skb_queue_len(&local->pending[queue]) >= PENDING_BUF) {
> + spin_unlock_irqrestore(&local->queue_stop_reason_lock,
> + flags);
> + goto drop;
> + } do {
> next = skb->next;
> skb->next = NULL;
> if (unlikely(txpending))
> @@ -2074,8 +2078,12 @@
> flags);
>
> txok = ieee80211_tx_pending_skb(local, skb);
> - if (!txok)
> - __skb_queue_head(&local->pending[i], skb);
> + if (!txok) {
> + if (skb_queue_len(&local->pending[i]) < PENDING_BUF)
> + __skb_queue_head(&local->pending[i], skb);
> + else
> + kfree_skb(skb);
> + }
> spin_lock_irqsave(&local->queue_stop_reason_lock,
> flags);
> if (!txok)
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -383,7 +383,10 @@
>
> spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
> __ieee80211_stop_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
> - __skb_queue_tail(&local->pending[queue], skb);
> + if (skb_queue_len(&local->pending[queue]) < PENDING_BUF)
> + __skb_queue_tail(&local->pending[queue], skb);
> + else
> + kfree_skb(skb);
> __ieee80211_wake_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
> spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
> }
> @@ -409,9 +412,12 @@
> continue;
> }
>
> - ret++;
> queue = skb_get_queue_mapping(skb);
> - __skb_queue_tail(&local->pending[queue], skb);
> + if (skb_queue_len(&local->pending[queue]) < PENDING_BUF) {
> + ret++;
> + __skb_queue_tail(&local->pending[queue], skb);
> + } else
> + kfree_skb(skb);
> }
>
> for (i = 0; i < hw->queues; i++)
John Linville's efforts as the wireless maintainer are made easier when everyone
follows the guidelines in Documentation/SubmittingPatches. For instance, this
patch should have been submitted with the subject "[PATCH V2] mac80211: Revise
pending queue depth in ieee80211_local data structure", or some such title. At
the beginning of the submission, you should describe the problem following the
guidelines mentioned above. This section is followed by the "Signed-off-by:"
line with a line consisting of "---". Everything above this line becomes part of
the official record if/when the patch is accepted. In this case, the quoting of
previous emails and the inclusion of the previous patch is inappropriate. Below
the ---, you can include additional information such as how this version differs
from previous submissions, and any instructions to John.
I have not reviewed the content of this patch - only the problem with the white
space caught my eye.
Larry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-18 12:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-18 10:12 pending queue depth in ieee80211_local data structure Lorenzo Bianconi
2010-03-18 10:44 ` Bruno Randolf
2010-03-18 11:35 ` Lorenzo Bianconi
2010-03-18 12:56 ` Larry Finger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.