[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] Re: [kvm-devel] [PATCH RFC 1/3] virtio infrastructure



Rusty Russell wrote:
> On Sat, 2007-06-02 at 09:30 +0300, Avi Kivity wrote:
>   
>> Rusty Russell wrote:
>>     
>>> + * virtio_ops - virtio abstraction layer
>>> + * @add_outbuf: prepare to send data to the other end:
>>> + * vdev: the virtio_device
>>> + * sg: the description of the buffer(s).
>>> + * num: the size of the sg array.
>>> + * used: the length sent (set once sending is done).
>>> + *      Returns an identifier or an error.
>>> + * @add_inbuf: prepare to receive data from the other end:
>>> + * vdev: the virtio_device
>>> + * sg: the description of the buffer(s).
>>> + * num: the size of the sg array.
>>> + * used: the length sent (set once data received).
>>> + *      Returns an identifier or an error (eg. -ENOSPC).
>>>   
>>>       
>> Instead of 'used', how about a completion callback (with associated data
>> pointer)?  A new helper, virtio_complete(), would call the callback for
>> all completed requests.  It would eliminate all the tedious scanning
>> used to match the identifier.
>>     
>
> Hi Avi,
>
>       There were several considerations here.  My first was that the drivers
> look much more like normal devices than getting a callback for every
> buffer.   Secondly, "used" batches much more nicely than a completion.
> Finally, it's also something you really want to know, so the driver
> doesn't have to zero its inbufs (an untrusted other side says it sends
> you 1500 bytes but actually sent nothing, and now you spray kernel
> memory out the NIC).
>   

Sure, 'used' is important (how else do you get the packet size on
receive?),  I'm just bitching about the linear scan.

>       I also considered some scheme like:
>
>       struct virtio_used_info
>       {
>               unsigned long len;
>               void *next_token;
>       };
>       ...
>       unsigned long (*add_outbuf)(struct virtio_device *vdev,
>                                   const struct scatterlist sg[],
>                                   unsigned int num,
>                                   void *token,
>                                   struct virtio_used_info *used_info);
>
> So the "used" becomes a "used/next" pair and you can just walk the
> linked list.  But I wasn't convinced that walking the buffers is going
> to be a performance issue (tho the net driver puts them in a continuous
> array for cache friendliness as a nod to this concern).
>   

Well, if you have 256 slots per direction, you will scan 4KB of memory
per interrupt (256 slots x 2 directions x 8 bytes).  You may need a
queue length greater than 256 for a high bandwidth interface, if you
want to reduce your interrupt rate, or if your guest is scheduled out
for lengthy periods (say, a millisecond).


>   
>> It would also be nice to support a bit of non-buffer data, like a set of
>> bitflags.
>>     
>
>       I expect this might be necessary, but it wasn't so far.  The non-buffer
> data tends to go in sg[0]: the block driver works this way, and the
> network driver will for GSO.  Of course, a specialized virtio_ops
> backend might well take this and put the info somewhere else.
>   

Yeah.  Well, it's a somewhat roundabout way of doing things.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to 
panic.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.