Re: [6/6] [VIRTIO] net: Allow receiving SG packets

Re: [6/6] [VIRTIO] net: Allow receiving SG packets

[Rusty Russell]

On Friday 18 April 2008 13:24:27 Herbert Xu wrote:
> Finally this patch lets virtio_net receive GSO packets in addition
> to sending them.
...
>  static void try_fill_recv(struct virtnet_info *vi)
>  {
>  	struct sk_buff *skb;
> -	struct scatterlist sg[1+MAX_SKB_FRAGS];
> +	struct scatterlist sg[2+MAX_SKB_FRAGS];
>  	int num, err;

I'm not sure what the right number is here.  Say worst case is header which 
goes over a page boundary then MAX_SKB_FRAGS in the skb, but for some reason 
that already has a +2:

/* To allow 64K frame to be packed as single skb without frag_list */
#define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)

Unless someone explains, I'll change the xmit sg to 2+MAX_SKB_FRAGS as well.

Thanks,
Rusty.

Re: [6/6] [VIRTIO] net: Allow receiving SG packets

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 22 Apr 2008 05:06:16 +1000

> I'm not sure what the right number is here.  Say worst case is header which 
> goes over a page boundary then MAX_SKB_FRAGS in the skb, but for some reason 
> that already has a +2:
> 
> /* To allow 64K frame to be packed as single skb without frag_list */
> #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
> 
> Unless someone explains, I'll change the xmit sg to 2+MAX_SKB_FRAGS as well.

MAX_SKB_FRAGS + 1 is what you ought to need.

MAX_SKB_FRAGS is only accounting for the skb frag pages.
If you want to know how many segments skb->data might
consume as well, you have to add one.

skb->data is linear, therefore it's not possible to need
more than one scatterlist entry for it.

Re: [6/6] [VIRTIO] net: Allow receiving SG packets

[Herbert Xu]

On Mon, Apr 21, 2008 at 01:04:18PM -0700, David Miller wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
> Date: Tue, 22 Apr 2008 05:06:16 +1000
> 
> > I'm not sure what the right number is here.  Say worst case is header which 
> > goes over a page boundary then MAX_SKB_FRAGS in the skb, but for some reason 
> > that already has a +2:

The +2 (i.e., extra +1) is for the virtio GSO header.
 
> skb->data is linear, therefore it's not possible to need
> more than one scatterlist entry for it.

Theoretically yes :) But for virtualisation the underlying transport
may present meta-physically contiguous memory that is physically
discrete.  So we may actually need to have multiple SG entries for
skb->data.  However, no current code path should generate packets
with both long skb->data areas *and* skb page frags so we could
just drop them if they show up.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [6/6] [VIRTIO] net: Allow receiving SG packets

[Rusty Russell]

On Tuesday 22 April 2008 06:04:18 David Miller wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
> Date: Tue, 22 Apr 2008 05:06:16 +1000
>
> > I'm not sure what the right number is here.  Say worst case is header
> > which goes over a page boundary then MAX_SKB_FRAGS in the skb, but for
> > some reason that already has a +2:
> >
> > /* To allow 64K frame to be packed as single skb without frag_list */
> > #define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
> >
> > Unless someone explains, I'll change the xmit sg to 2+MAX_SKB_FRAGS as
> > well.
>
> MAX_SKB_FRAGS + 1 is what you ought to need.

Right, and so that's +2 for virtio_net because we have an extra header as  
Herbert points out.

But I was curious as to why the +2 in the MAX_SKB_FRAGS definition?

Thanks,
Rusty.

Re: [6/6] [VIRTIO] net: Allow receiving SG packets

[David Miller]

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 22 Apr 2008 12:50:27 +1000

> But I was curious as to why the +2 in the MAX_SKB_FRAGS definition?

To be honest I have no idea.

When Alexey added the TSO changeset way back then, it had the
"+2", from the history-2.6 tree:

commit 80223d5186f73bf42a7e260c66c9cb9f7d8ec9cf
Author: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Date:   Wed Aug 28 11:52:03 2002 -0700

    [NET]: Add TCP segmentation offload core infrastructure.

 ...
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a812681..9b6e6ad 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -109,7 +109,8 @@ struct sk_buff_head {
 
 struct sk_buff;
 
-#define MAX_SKB_FRAGS 6
+/* To allow 64K frame to be packed as single skb without frag_list */
+#define MAX_SKB_FRAGS (65536/PAGE_SIZE + 2)
 
 typedef struct skb_frag_struct skb_frag_t;