RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.

Xin, Xiaohui
2010-06-18T05:26:49Z

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

>-----Original Message-----
>From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
>Sent: Thursday, June 17, 2010 7:21 PM
>To: Xin, Xiaohui
>Cc: Stephen Hemminger; netdev@vger.kernel.org; kvm@vger.kernel.org;
>linux-kernel@vger.kernel.org; mst@redhat.com; mingo@elte.hu; davem@davemloft.net;
>jdike@linux.intel.com
>Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>
>On Sun, Jun 13, 2010 at 04:58:36PM +0800, Xin, Xiaohui wrote:
>>
>> Herbert,
>> In this way, I think we should create 3 functions at least in drivers to allocate rx buffer, to
>receive the rx buffers, and to clean the rx buffers.
>>
>> We can also have another way here. We can provide a function to only substitute
>> alloc_page(), and a function to release the pages when cleaning the rx buffers.
>
>Yes that's exactly what I had in mind.
>
Herbert,
I have questions about the idea above:
1) Since netdev_alloc_skb() is still there, and we only modify alloc_page(),
then we don't need napi_gro_frags() any more, the driver's original receiving
function is ok. Right?

2) Is napi_gro_frags() only suitable for TCP protocol packet?
I have done a small test for ixgbe driver to let it only allocate paged buffers
and found kernel hangs when napi_gro_frags() receives an ARP packet.

3) As I have mentioned above, with this idea, netdev_alloc_skb() will allocate
as usual, the data pointed by skb->data will be copied into the first guest buffer.
That means we should reserve sufficient room in guest buffer. For PS mode
supported driver (for example ixgbe), the room will be more than 128. After 128bytes,
we will put the first frag data. Look into virtio-net.c the function page_to_skb()
and receive_mergeable(), that means we should modify guest virtio-net driver to
compute the offset as the parameter for skb_set_frag().

How do you think about this? Attached is a patch to how to modify the guest driver.
I reserve 512 bytes as an example, and transfer the header len of the skb in hdr->hdr_len.

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

[-- Attachment #2: mod-guest.diff --]
[-- Type: application/octet-stream, Size: 2031 bytes --]

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b0577dd..a4e6988 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -36,7 +36,9 @@ module_param(gso, bool, 0444);

/* FIXME: MTU in config. */
#define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
-#define GOOD_COPY_LEN 128
+#define GOOD_COPY_LEN 512

#define VIRTNET_SEND_COMMAND_SG_MAX 2

@@ -147,11 +149,12 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
{
struct sk_buff *skb;
struct skb_vnet_hdr *hdr;
- unsigned int copy, hdr_len, offset;
+ unsigned int copy, hdr_len, offset, end;
char *p;

p = page_address(page);
@@ -172,14 +175,25 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
len -= hdr_len;
p += offset;

- copy = len;
+ if (!hdr->hdr.hdr_len)
+ copy = len;
+ else {
+ copy = hdr->hdr.hdr_len;
+ end = skb_tailroom(skb);
+ printk(KERN_INFO "c %d, e %d\n", copy, end);
+ }
+
if (copy > skb_tailroom(skb))
copy = skb_tailroom(skb);
memcpy(skb_put(skb, copy), p, copy);

len -= copy;
- offset += copy;
+ if (!hdr->hdr.hdr_len)
+ offset += copy;
+ else
+ offset = end;

+ printk(KERN_INFO "off %d\n", offset);
while (len) {
set_skb_frag(skb, page, offset, &len);
page = (struct page *)page->private;
@@ -218,7 +238,12 @@ static int receive_mergeable(struct virtnet_info *vi, struct sk_buff *skb)
len = PAGE_SIZE;

set_skb_frag(skb, page, 0, &len);
-
+ if (hdr->hdr.hdr_len)
+ set_skb_frag(skb, page, 542, &len);
+ else
+ set_skb_frag(skb, page, 0, &len);
--vi->num;
}
return 0;
@@ -311,7 +340,15 @@ static void receive_buf(struct net_device *dev, void *buf, unsigned int len)
skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
skb_shinfo(skb)->gso_segs = 0;
}
-
+ skb->ip_summed = 1;
netif_receive_skb(skb);
return;

@@ -479,7 +516,8 @@ again:
received++;
}

- if (vi->num < vi->max / 2) {
+ if (vi->num < vi->max - 10) {
if (!try_fill_recv(vi, GFP_ATOMIC))
schedule_delayed_work(&vi->refill, 0);
}