Re: [PATCH 0/3] vhost cleanups and separate module

[Jason Wang <jasowang@redhat.com>]

On 05/07/2013 08:44 PM, Michael S. Tsirkin wrote:
> On Tue, May 07, 2013 at 02:13:44PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>> On Mon, May 06, 2013 at 03:41:36PM +0930, Rusty Russell wrote:
>>>> Asias He <asias@redhat.com> writes:
>>>>> Asias He (3):
>>>>>   vhost: Remove vhost_enable_zcopy in vhost.h
>>>>>   vhost: Move VHOST_NET_FEATURES to net.c
>>>>>   vhost: Make vhost a separate module
>>>> I like these cleanups, MST pleasee apply.
>>> Absolutely. Except it's 3.11 material and I can only
>>> usefully create a -next branch once -rc1 is out.
>>>
>>>> I have some other cleanups which are on hold for the moment pending
>>>> MST's vhost_net simplification.  MST, how's that going?
>>> Not too well. The array of status bytes which was designed to complete
>>> packets in order turns out to be a very efficient datastructure:
>>>
>>> It gives us a way to signal completions that is completely lockless for
>>> multiple completers, and using the producer/consumer model saves extra
>>> scans for the common case.
>>>
>>> Overall I can save some memory and clean up some code but can't get rid
>>> of the producer/consumer idices (currently named upend/done indices)
>>> which is what you asked me to do.
>>> Your cleanups basically don't work with zcopy because they
>>> ignore the upend/done indices?
>>> Would you like to post them, noting they only work with zcopy off, and
>>> we'll look for a way to apply them, together?
>> Not quite; it's just that I don't understand that code.  It seemed to be
>> achieving something (ordered completion) which was entirely unnecessary,
>> so I went on with other things while you removed it.  Now that's not
>> possible, I'll revisit.
>>
>> AFAICT we should always do zero copy.
> It seems not to be a win for small packets.
> I speculate the issue is that ring space isn't released as promptly.
> Further, we can't do it safely for guest to guest and guest to host.
> And if we try, net core just does a packet copy later (which is less
> efficient). So there's a hack in place to detect that and suppress zero
> copy.

We can do something to eliminate this copy:

- change the vnet header to NET_SKB_PAD
- use build_skb() to build the skb->data from the page directly

Then for packet size smaller than PAGE_SIZE - NET_SKB_PAD -
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), we can build the packet
directly instead of copy 128 bytes.

>
>> Though I do wonder if we should
>> use a dedicated hook to get an skb into the tun driver and generate it
>> ourselves, rather than going sg -> iov -> skb.
>>
>> Cheers,
>> Rusty.
> I think we'd have to export two interfaces:
> - alloc_skb()
>   .... add frags ...
> - send_skb
>
> the code to add frags could maybe use some
> library functions ...
>