From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dion Kant <g.w.kant@hunenet.nl>
Subject: Re: [PATCH] xen-netfront: pull on receive skb may need
 to happen earlier
Date: Sat, 13 Jul 2013 22:17:51 +0200
Message-ID: <51E1B5EF.6010603@hunenet.nl>
References: <8511913.uMAmUdIO30@eistomin.edss.local>
	<20130517085923.GC14401@zion.uk.xensource.com>
	<51D57C1F.8070909@hunenet.nl>
	<20130704150137.GW7483@zion.uk.xensource.com>
	<51D6AED902000078000E2EA9@nat28.tlf.novell.com>
	<20130705145319.GB9050@zion.uk.xensource.com>
	<51DAA9B202000078000E3357@nat28.tlf.novell.com>
	<51DAE6CA02000078000E3566@nat28.tlf.novell.com>
	<20130712083248.GF23269@zion.uk.xensource.com>
	<51DFE0C702000078000E462A@nat28.tlf.novell.com>
	<51E13961.4030101@hunenet.nl>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <51E13961.4030101@hunenet.nl>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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