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