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
