| Message ID | d016cad18842b71c6f06710cbc71cb3d55466a2e.1535567864.git.lorenzo.bianconi@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] mac80211: do not aggregate frames if max_frags is set to one | expand |
On Wed, 2018-08-29 at 21:03 +0200, Lorenzo Bianconi wrote: > Do not try to aggregate packets in a A-MSDU frame and add A-MSDU header > on the first packet if max_tx_fragments or max_amsdu_subframes are > set to one > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v1: > - rebased on top of mac80211 master branch > - removed ieee80211_amsdu_realloc_pad chunk > Don't you still have to account for the changed header length if the second one fails or something? You said you wanted to account for the pad bytes - which now aren't there - but didn't that also take the changed header of the first subframe into account? johannes
> > On Wed, 2018-08-29 at 21:03 +0200, Lorenzo Bianconi wrote: > > Do not try to aggregate packets in a A-MSDU frame and add A-MSDU header > > on the first packet if max_tx_fragments or max_amsdu_subframes are > > set to one > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Changes since v1: > > - rebased on top of mac80211 master branch > > - removed ieee80211_amsdu_realloc_pad chunk > > > Don't you still have to account for the changed header length if the > second one fails or something? > That has been fixed by Sara's patch since now we add the pad directly on the head of the subsequent subframes (not to the tail of the first one). Am I missing something? Regards, Lorenzo > You said you wanted to account for the pad bytes - which now aren't > there - but didn't that also take the changed header of the first > subframe into account? > > johannes
On Wed, Aug 29, 2018 at 9:13 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2018-08-29 at 21:12 +0200, Lorenzo Bianconi wrote: > > > That has been fixed by Sara's patch since now we add the pad directly > > on the head of the > > subsequent subframes (not to the tail of the first one). Am I missing something? > > As far as the pad is concerned, yes, but I think the header length also > changes? Ops, maybe I got your point: ieee80211_amsdu_realloc_pad() in ieee80211_amsdu_prepare_head() can expand the headroom on the first frame but if ieee80211_amsdu_realloc_pad() on the second one fails, we do not take into account the extra len added on the first subframe. Is that what you mean? > > I'm too tired now - will think about it again tomorrow. > ack :) Regards, Lorenzo > johannes
First of all, I applied your patch with now, but changed the commit message. I hope it still makes sense. > Ops, maybe I got your point: > ieee80211_amsdu_realloc_pad() in ieee80211_amsdu_prepare_head() can > expand the headroom on the first frame Right. > but if ieee80211_amsdu_realloc_pad() on the second one fails, we do > not take into account the extra len added on the > first subframe. Is that what you mean? Yes, that's what I was thinking of, but you described it much better than me :) If this needs to be addressed, please send a separate patch. johannes
> First of all, I applied your patch with now, but changed the commit > message. I hope it still makes sense. Thx, definitely better than mine :) > > > Ops, maybe I got your point: > > ieee80211_amsdu_realloc_pad() in ieee80211_amsdu_prepare_head() can > > expand the headroom on the first frame > > Right. > > > but if ieee80211_amsdu_realloc_pad() on the second one fails, we do > > not take into account the extra len added on the > > first subframe. Is that what you mean? > > Yes, that's what I was thinking of, but you described it much better > than me :) > > If this needs to be addressed, please send a separate patch. Reviewing the code I guess it is not necessary since pskb_expand_head routine does not modify head->len (or skb->len). Packet len (if we consider padding) is only modified in: memset(skb_push(skb, pad), 0, pad); and if we hit that point, we will account new skb->len in flow backlog. Do you agree? Looking at the code maybe I spotted another issue, I guess there is an off-by-one issue in 'n' estimation since it does not take into account the first frame. We hit the line: while (*frag_tail) { } starting from the second subframe, but if the head does not have packet in the fraglist we will end up having n = 1, while it is actually the second frame. Does n count just subsequent frames or also the first one? Regards, Lorenzo > > johannes
On Thu, 2018-08-30 at 10:31 +0200, Lorenzo Bianconi wrote: > Reviewing the code I guess it is not necessary since pskb_expand_head routine > does not modify head->len (or skb->len). True. > Packet len (if we consider padding) is only modified in: > > memset(skb_push(skb, pad), 0, pad); > > and if we hit that point, we will account new skb->len in flow backlog. Do you > agree? Right, but that's the *pad*. I was thinking about the header conversion. Let's say you decided to add the second frame to the A-MSDU, at which point the first one isn't really an A-MSDU yet. So we get to: if (!ieee80211_amsdu_prepare_head(sdata, fast_tx, head)) which changes the header of "head" to be 14 bytes longer: skb_push(skb, sizeof(*amsdu_hdr)); But now let's say we get a failure here when reallocating the second subframe: if (!ieee80211_amsdu_realloc_pad(local, skb, sizeof(rfc1042_header) + 2 + pad)) goto out; Now we have changed "head", which is on the FQ, but we haven't changed the FQ accounting. So I *think* we still need this: --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3239,7 +3239,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata, if (!ieee80211_amsdu_realloc_pad(local, skb, sizeof(rfc1042_header) + 2 + pad)) - goto out; + goto out_recalc; ret = true; data = skb_push(skb, ETH_ALEN + 2); @@ -3256,11 +3256,13 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata, head->data_len += skb->len; *frag_tail = skb; - flow->backlog += head->len - orig_len; - tin->backlog_bytes += head->len - orig_len; - - fq_recalc_backlog(fq, tin, flow); +out_recalc: + if (head->len != orig_len) { + flow->backlog += head->len - orig_len; + tin->backlog_bytes += head->len - orig_len; + fq_recalc_backlog(fq, tin, flow); + } out: spin_unlock_bh(&fq->lock); > Looking at the code maybe I spotted another issue, I guess there is an > off-by-one issue in 'n' estimation since it does not take into account > the first frame. We hit the line: > > while (*frag_tail) { > } > > starting from the second subframe, but if the head does not have packet in the > fraglist we will end up having n = 1, while it is actually the second frame. Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU header and the first subframe in skb->data (and/or frags), with the subframes 2..N in the fraglist. So I think this is right? johannes
> On Thu, 2018-08-30 at 10:31 +0200, Lorenzo Bianconi wrote: > > > Reviewing the code I guess it is not necessary since pskb_expand_head routine > > does not modify head->len (or skb->len). > > True. > > > Packet len (if we consider padding) is only modified in: > > > > memset(skb_push(skb, pad), 0, pad); > > > > and if we hit that point, we will account new skb->len in flow backlog. Do you > > agree? > > Right, but that's the *pad*. I was thinking about the header conversion. > > Let's say you decided to add the second frame to the A-MSDU, at which > point the first one isn't really an A-MSDU yet. So we get to: > > if (!ieee80211_amsdu_prepare_head(sdata, fast_tx, head)) > > which changes the header of "head" to be 14 bytes longer: > > skb_push(skb, sizeof(*amsdu_hdr)); > > But now let's say we get a failure here when reallocating the second > subframe: > > if (!ieee80211_amsdu_realloc_pad(local, skb, sizeof(rfc1042_header) + > 2 + pad)) > goto out; > > Now we have changed "head", which is on the FQ, but we haven't changed > the FQ accounting. So I *think* we still need this: > > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -3239,7 +3239,7 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata, > > if (!ieee80211_amsdu_realloc_pad(local, skb, sizeof(rfc1042_header) + > 2 + pad)) > - goto out; > + goto out_recalc; > > ret = true; > data = skb_push(skb, ETH_ALEN + 2); > @@ -3256,11 +3256,13 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata, > head->data_len += skb->len; > *frag_tail = skb; > > - flow->backlog += head->len - orig_len; > - tin->backlog_bytes += head->len - orig_len; > - > - fq_recalc_backlog(fq, tin, flow); > +out_recalc: > + if (head->len != orig_len) { > + flow->backlog += head->len - orig_len; > + tin->backlog_bytes += head->len - orig_len; > > + fq_recalc_backlog(fq, tin, flow); > + } > out: > spin_unlock_bh(&fq->lock); > ack, I agree. Do you want I send a patch to fix it? > > > > Looking at the code maybe I spotted another issue, I guess there is an > > off-by-one issue in 'n' estimation since it does not take into account > > the first frame. We hit the line: > > > > while (*frag_tail) { > > } > > > > starting from the second subframe, but if the head does not have packet in the > > fraglist we will end up having n = 1, while it is actually the second frame. > > Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU > header and the first subframe in skb->data (and/or frags), with the > subframes 2..N in the fraglist. > > So I think this is right? yep, correct. But when we are analyzing the second subframe what is the correct value for 'n'? 1 or 2? At the moment I guess it is set to 1 if frag_tail is NULL for head. Regards, Lorenzo > > johannes >
On Thu, 2018-08-30 at 10:50 +0200, Lorenzo Bianconi wrote: > > ack, I agree. Do you want I send a patch to fix it? I have it written now, I'll just commit & send it out. > > Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU > > header and the first subframe in skb->data (and/or frags), with the > > subframes 2..N in the fraglist. > > > > So I think this is right? > > yep, correct. But when we are analyzing the second subframe what is the correct value for 'n'? > 1 or 2? At the moment I guess it is set to 1 if frag_tail is NULL for head. Ah. I guess you're right. So basically setting max_subframes to 1 doesn't avoid A-MSDUs completely, since n will still be 1 when we get here ... good point, care to send a patch? johannes
> On Thu, 2018-08-30 at 10:50 +0200, Lorenzo Bianconi wrote: > > > > ack, I agree. Do you want I send a patch to fix it? > > I have it written now, I'll just commit & send it out. Sound good, thx :) > > > > Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU > > > header and the first subframe in skb->data (and/or frags), with the > > > subframes 2..N in the fraglist. > > > > > > So I think this is right? > > > > yep, correct. But when we are analyzing the second subframe what is the correct value for 'n'? > > 1 or 2? At the moment I guess it is set to 1 if frag_tail is NULL for head. > > Ah. I guess you're right. So basically setting max_subframes to 1 > doesn't avoid A-MSDUs completely, since n will still be 1 when we get > here ... good point, care to send a patch? > ack, I will send a patch for it Regards, Lorenzo > johannes
On Thu, 2018-08-30 at 11:00 +0200, Lorenzo Bianconi wrote: > > On Thu, 2018-08-30 at 10:50 +0200, Lorenzo Bianconi wrote: > > > > > > ack, I agree. Do you want I send a patch to fix it? > > > > I have it written now, I'll just commit & send it out. > > Sound good, thx :) > > > > > > > Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU > > > > header and the first subframe in skb->data (and/or frags), with the > > > > subframes 2..N in the fraglist. > > > > > > > > So I think this is right? > > > > > > yep, correct. But when we are analyzing the second subframe what is the correct value for 'n'? > > > 1 or 2? At the moment I guess it is set to 1 if frag_tail is NULL for head. > > > > Ah. I guess you're right. So basically setting max_subframes to 1 > > doesn't avoid A-MSDUs completely, since n will still be 1 when we get > > here ... good point, care to send a patch? > > > > ack, I will send a patch for it The same is true for nfrags, btw, no? johannes
On Thu, 2018-08-30 at 11:03 +0200, Johannes Berg wrote: > On Thu, 2018-08-30 at 11:00 +0200, Lorenzo Bianconi wrote: > > > On Thu, 2018-08-30 at 10:50 +0200, Lorenzo Bianconi wrote: > > > > > > > > ack, I agree. Do you want I send a patch to fix it? > > > > > > I have it written now, I'll just commit & send it out. > > > > Sound good, thx :) > > > > > > > > > > Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU > > > > > header and the first subframe in skb->data (and/or frags), with the > > > > > subframes 2..N in the fraglist. > > > > > > > > > > So I think this is right? > > > > > > > > yep, correct. But when we are analyzing the second subframe what is the correct value for 'n'? > > > > 1 or 2? At the moment I guess it is set to 1 if frag_tail is NULL for head. > > > > > > Ah. I guess you're right. So basically setting max_subframes to 1 > > > doesn't avoid A-MSDUs completely, since n will still be 1 when we get > > > here ... good point, care to send a patch? > > > > > > > ack, I will send a patch for it > > The same is true for nfrags, btw, no? No, it's not, just misread the code. johannes
On Aug 30, Johannes Berg wrote: > On Thu, 2018-08-30 at 11:00 +0200, Lorenzo Bianconi wrote: > > > On Thu, 2018-08-30 at 10:50 +0200, Lorenzo Bianconi wrote: > > > > > > > > ack, I agree. Do you want I send a patch to fix it? > > > > > > I have it written now, I'll just commit & send it out. > > > > Sound good, thx :) > > > > > > > > > > Hmm, not sure I follow? "head" is the A-MSDU, containing the A-MSDU > > > > > header and the first subframe in skb->data (and/or frags), with the > > > > > subframes 2..N in the fraglist. > > > > > > > > > > So I think this is right? > > > > > > > > yep, correct. But when we are analyzing the second subframe what is the correct value for 'n'? > > > > 1 or 2? At the moment I guess it is set to 1 if frag_tail is NULL for head. > > > > > > Ah. I guess you're right. So basically setting max_subframes to 1 > > > doesn't avoid A-MSDUs completely, since n will still be 1 when we get > > > here ... good point, care to send a patch? > > > > > > > ack, I will send a patch for it > > The same is true for nfrags, btw, no? I do not think so since for nfrags we have: nfrags = 1 + skb_shinfo(skb)->nr_frags; nfrags += 1 + skb_shinfo(head)->nr_frags; and even if frag_tail is NULL we will have nfrags = 2. Agree? Lorenzo > > johannes
On Thu, 2018-08-30 at 11:07 +0200, Lorenzo Bianconi wrote: > > I do not think so since for nfrags we have: > nfrags = 1 + skb_shinfo(skb)->nr_frags; > nfrags += 1 + skb_shinfo(head)->nr_frags; > > and even if frag_tail is NULL we will have nfrags = 2. > > Agree? Yeah, I misread those two lines. johannes
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 6a362b2882d3..75646e3fb3d9 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3213,9 +3213,6 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata, if (skb->len + head->len > max_amsdu_len) goto out; - if (!ieee80211_amsdu_prepare_head(sdata, fast_tx, head)) - goto out; - nfrags = 1 + skb_shinfo(skb)->nr_frags; nfrags += 1 + skb_shinfo(head)->nr_frags; frag_tail = &skb_shinfo(head)->frag_list; @@ -3231,6 +3228,9 @@ static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata, if (max_frags && nfrags > max_frags) goto out; + if (!ieee80211_amsdu_prepare_head(sdata, fast_tx, head)) + goto out; + /* * Pad out the previous subframe to a multiple of 4 by adding the * padding to the next one, that's being added. Note that head->len
Do not try to aggregate packets in a A-MSDU frame and add A-MSDU header on the first packet if max_tx_fragments or max_amsdu_subframes are set to one Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- Changes since v1: - rebased on top of mac80211 master branch - removed ieee80211_amsdu_realloc_pad chunk --- net/mac80211/tx.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)