Re: [virtio-dev] [RFC PATCH v2] virtio-spec: Introduce new Receive-Segment-Coalescing feature for guest

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon, 13 Jun 2016 20:55:31 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 08, 2016 at 08:43:28AM +0800, Wei Xu wrote:

> > @@ -3247,8 +3252,11 @@
> >          u8 flags;
> >  #define VIRTIO_NET_HDR_GSO_NONE        0
> >  #define VIRTIO_NET_HDR_GSO_TCPV4       1
> > -#define VIRTIO_NET_HDR_GSO_UDP         3
> > -#define VIRTIO_NET_HDR_GSO_TCPV6       4
> > +#define VIRTIO_NET_HDR_GSO_UDP         2
> > +#define VIRTIO_NET_HDR_GSO_TCPV6       3
> 
> This is not a good idea I think.
> We have code out there using the existing flags.
> 
> Also, pls look at 34a48579e4fb380604d06f0409db3851bd22d785 in linux -
> some very old guests used
> -#define VIRTIO_NET_HDR_GSO_TCPV4_ECN   2       // GSO frame, IPv4 TCP w/ ECN
> so it's best not to touch it.

Should we maybe add some "reserved" flag, so nobody tries to reuse them?

> >  when VIRTIO_NET_F_MRG_RXBUF was negotiated; without that feature the
> >  structure was 2 bytes shorter.
> > 
> > +The legacy driver only presented \field{coalesced_pkts},
> > \field{coalesced_size}
> > +and \field{dup_ack} in the struct virtio_net_hdr when
> > VIRTIO_NET_F_GUEST_RSC4
> > +or VIRTIO_NET_F_GUEST_RSC6 was negotiated and a packet really has coalesced
> > +additional packet(s), without satisfying the 2 conditions the structure was
> > +6 bytes shorter.
> > +
> >  When using the legacy interface, the driver SHOULD ignore the
> >  \field{len} value in used ring entries for the transmit queues
> >  and the controlq queue.
> 
> 
> I don't see why do you ever need to discuss legacy.
> Legacy drivers never negotiate any RSC options.
> Just drop this part?

Agreed.

> > +\item If one or both VIRTIO_NET_F_GUEST_RSC4, RSC6 were negotiated,

"If one or both of..."

> > +  but it is a original packet which didn't coalesce any packet,
> > +  then the \field{flags} should be set to VIRTIO_NET_HDR_RSC_NONE.
> > +
> > +\item If the VIRTIO_NET_F_GUEST_RSC4, RSC6 options were negotiated,

Also one or both? (Also below.)

No additional comments from me right now.