Re: [Xen-devel] [PATCH net-next] xen-netback: fix xenvif_count_skb_slots()

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Oct 07, 2013 at 11:37:31AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> > bounces@xxxxxxxxxxxxx] On Behalf Of Paul Durrant
> > Sent: 07 October 2013 11:24
> > To: David Vrabel
> > Cc: Wei Liu; Ian Campbell; netdev@xxxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> > Annie Li; Matt Wilson; Xi Xiong
> > Subject: Re: [Xen-devel] [PATCH net-next] xen-netback: fix
> > xenvif_count_skb_slots()
> > 
> > > -----Original Message-----
> > > From: David Vrabel
> > > Sent: 07 October 2013 10:50
> > > To: Paul Durrant
> > > Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Wei Liu; Ian
> > Campbell;
> > > Annie Li; Matt Wilson; Xi Xiong
> > > Subject: Re: [Xen-devel] [PATCH net-next] xen-netback: fix
> > > xenvif_count_skb_slots()
> > >
> > > On 04/10/13 17:26, Paul Durrant wrote:
> > > > Commit 4f0581d25827d5e864bcf07b05d73d0d12a20a5c introduced an
> > error
> > > into
> > > > xenvif_count_skb_slots() for skbs with a linear area spanning a page
> > > > boundary. The alignment of skb->data needs to be taken into account,
> > not
> > > > just the head length. This patch fixes the issue by dry-running the code
> > > > from xenvif_gop_skb() (and adjusting the comment above the function
> > to
> > > note
> > > > that).
> > >
> > > If 4f0581d2582 is causing the skb->data to be fully packed into a
> > > minimal number of slots then the simple
> > > DIV_ROUND_UP(skb_headlen(skb))
> > > is correct.
> > >
> > > I think this change will miscount in the number of slots,
> > > over-estimating the count which I think will eventually cause netback to
> > > think the ring has no space when it has some.
> > >
> > > Is the problem here not the miscounting of slots but running out of
> > > space in the grant table op array because we know use more copy ops?
> > >
> > 
> > Essentially yes. Netback is built on the assumption of no more than two 
> > grant
> > copies per ring slot.
> > 
> 
> To be clear; I believe that, with the packing change, a third grant copy may 
> be used for the initial slot and that is why we blow the array.
> 

OK, thanks for the explanation. I'm fine with reverting the problematic
changeset as that part of code is quite fragile now. Looking at the git
history we've tripped over this many times, sigh.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel