Re: [PATCH] xen-netfront: pull on receive skb may need to happen earlier

[Dion Kant <g.w.kant@hunenet.nl>]

On 07/13/2013 01:26 PM, Dion Kant wrote:
> There is something wrong with the part
> 
> +if (nr_frags == MAX_SKB_FRAGS) {
> +       unsigned int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
> +
> +       BUG_ON(pull_to <= skb_headlen(skb));
> +       __pskb_pull_tail(skb, pull_to - skb_headlen(skb));
> +       nr_frags = shinfo->nr_frags;
> +}
> +BUG_ON(nr_frags >= MAX_SKB_FRAGS);
> 
> When nr_frags reaches MAX_SKB_FRAGS (and this happens), nr_frags is
> "reset" to shinfo->nr_frags. In fact I see the nr_frags set to 0 the
> first time nr_frags reaches MAX_SKB_FRAGS.
> 
> The first problem with this is of course that the "skb->truesize +=
> PAGE_SIZE * skb_shinfo(skb)->nr_frags" calculation following the "i =
> xennet_fill_frags(np, skb, &tmpq)" in  neif_poll leads to a wrong
> result. At the end the skb has ->truesize way smaller than ->len.
> 
> The second problem is that the BUG_ON(nr_frags >= MAX_SKB_FRAGS) shall
> not occur, since "shinfo->nr_frags" afaict, is not updated in between.
> 
> Furthermore, I cannot figure out why, when control enters
> xennet_fill_frags(), shinfo->nr_frags equals 1, and a little later when
> nr_frags reaches MAX_SKB_FRAGS, it is 0.

Jan,

After some reading I now understand, it is __pskb_pull_tail(skb, pull_to
- skb_headlen(skb)) which lowers shinfo->nr_frags by one.

Must we then not first set shinfo->nr_frags to its correct value before
the call to __pskb_pull_tail(skb, pull_to - skb_headlen(skb)), like below?

+if (nr_frags == MAX_SKB_FRAGS) {
+       unsigned int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
+
+       BUG_ON(pull_to <= skb_headlen(skb));
+       shinfo->nr_frags = nr_frags;
+       __pskb_pull_tail(skb, pull_to - skb_headlen(skb));
+       nr_frags = shinfo->nr_frags;
+}
+BUG_ON(nr_frags >= MAX_SKB_FRAGS);

I'll do some testing with this.

Dion