Re: [virtio] A plea on the first draft...

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 30, 2013 at 03:07:04PM +1030, Rusty Russell wrote:
> Hi all,
> 
>         I'd really appreciate it if you could make time during the next
> week to comb your various areas of the draft to see if there are any
> places we can remove old features or advice.
> 
> Skimming through, there are three I am aware of:
> 
> 1) We should unconditionally recommend resetting the device as first
>    step.  It's good practice.
> 
> 2) We should not suggest that notifications occur in the
>    no-free-descriptor case.  It's bad advice, and only helps with buggy
>    hosts.
> 
> 3) We should make the net header a constant size by always having a
>    "num_buffers" field, and insisting that it be 1 if !VIRTIO_NET_F_MRG_RXBUF.
> 
> Cheers,
> Rusty.

I am not sure I agree with 3.
In the end it does not seem simplify either guests or hosts.
And if you implement transitional drivers, it complicates
non mergeable buffer handling.

Agree with 1 and 2.
Let's split them so we can vote separately?


What would simplify drivers a lot is if we said that
all devices must support mergeable buffers
if enabled by driver.
That's a large chunk of code in guest.

Want to write a text like this?


> diff --git a/virtio-v1.0-wd01-part1-specification.txt b/virtio-v1.0-wd01-part1-specification.txt
> index ce79c05..fc04d4a 100644
> --- a/virtio-v1.0-wd01-part1-specification.txt
> +++ b/virtio-v1.0-wd01-part1-specification.txt
> @@ -492,7 +492,7 @@ how to communicate with the specific device.
>  2.2.1. Device Initialization
>  ---------------------------
>  
> -1. Reset the device. This is not required on initial start up.
> +1. Reset the device.
>  
>  2. The ACKNOWLEDGE status bit is set: we have noticed the device.
>  
> @@ -561,10 +561,6 @@ operates as follows:
>  
>  1. Place the buffer(s) into free descriptor(s).
>  
> -  (a) If there are no free descriptors, the guest may choose to
> -    notify the device even if notifications are suppressed (to
> -    reduce latency).[8]
> -
>  2. Place the id of the buffer in the next ring entry of the
>    available ring.
>  
> @@ -1955,7 +1951,7 @@ case, the packet itself is preceeded by a header:
>  		u16 gso_size;
>  		u16 csum_start;
>  		u16 csum_offset;
> -	/* Only if VIRTIO_NET_F_MRG_RXBUF: */
> +		/* Ignored for transmitted packets. */
>  		u16 num_buffers;
>  	};
>  
> @@ -2001,12 +1997,9 @@ the different features the driver negotiated.
>      the VIRTIO_NET_HDR_GSO_ECN bit may be set in “gso_type” as
>      well, indicating that the TCP packet has the ECN bit set.[18]
>  
> -3. If the driver negotiated the VIRTIO_NET_F_MRG_RXBUF feature,
> -  the num_buffers field is set to zero.
> -
> -4. The header and packet are added as one output buffer to the
> +3. The header and packet are added as one output buffer to the
>    transmitq, and the device is notified of the new entry (see "2.4.1.4.
> -  Notifying The Device").[19]
> +  Notifying The Device").
>  
>  2.4.1.5.1.1. Packet Transmission Interrupt
>  -----------------------------------------
> @@ -2046,17 +2039,17 @@ packets until no more are found, then re-enable them.
>  
>  Processing packet involves:
>  
> -1. If the driver negotiated the VIRTIO_NET_F_MRG_RXBUF feature,
> -  then the “num_buffers” field indicates how many descriptors
> -  this packet is spread over (including this one). This allows
> -  receipt of large packets without having to allocate large
> +1. The “num_buffers” field indicates how many descriptors this packet
> +  is spread over (including this one).  The device must set this to 1
> +  if VIRTIO_NET_F_MRG_RXBUF has not been negotiated.
> +
> +  This allows receipt of large packets without having to allocate large
>    buffers. In this case, there will be at least “num_buffers” in
>    the used ring, and they should be chained together to form a
>    single packet. The other buffers will not begin with a struct
>    virtio_net_hdr.
>  
> -2. If the VIRTIO_NET_F_MRG_RXBUF feature was not negotiated, or
> -  the “num_buffers” field is one, then the entire packet will be
> +2. If “num_buffers” field is one, then the entire packet will be
>    contained within this buffer, immediately following the struct
>    virtio_net_hdr.
>  
> @@ -3581,11 +3574,6 @@ separate cache lines.
>  [7] These fields are kept here because this is the only part of the
>  virtqueue written by the device
>  
> -[8] The Linux drivers do this only for read-only buffers: for
> -write-only buffers, it is assumed that the driver is merely
> -trying to keep the receive buffer ring full, and no notification
> -of this expected condition is necessary.
> -
>  [9] https://lists.linux-foundation.org/mailman/listinfo/virtualization
>  
>  [11] Even if it does mean documenting design or implementation
> @@ -3620,9 +3608,6 @@ as a guarantee of the transport header size.
>  [18] This case is not handled by some older hardware, so is called out
>  specifically in the protocol.
>  
> -[19] Note that the header will be two bytes longer for the
> -VIRTIO_NET_F_MRG_RXBUF case.
> -
>  [20] Obviously each one can be split across multiple descriptor
>  elements.
>  
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe from this mail list, you must leave the OASIS TC that
> generates this mail.  Follow this link to all your TCs in OASIS at:
> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php