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

[Wei Xu <wexu@redhat.com>]

On 2016年12月19日 14:05, Jason Wang wrote:
> On 2016年12月19日 00:54, Wei Xu wrote:
> > Hi There,
> > This is the updated specification for Receive Segment Coalescing feature.
> >
> > Changelog:
> > v4->v5
> > -- Redefine support of the feature bit from
> >    by 'VIRTIO_NET_F_CTRL_GUEST_OFFLOADS' feature bit to
> >    VIRTIO_NET_GUEST_RSC4/6.
> > -- Add varying of the header fields description in case of
> >    if or not the feature is negotiated.
> > -- Add supporting of dynamically control of the feature by
> >    'VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET' control command.
> >
> >
> > v3->v4
> > -- Remove maximum packets size section.
> > -- Remove useless 'rsc_size' field after checked out the Microsoft
> > whql spec again.
> >
> >
> > v2->v3:
> > -- Revise flag definition for RSC4/6 to start from 5, add a 'RESERVED'
> > definition for already used number '2' which stands for historical
> > meaning of 'VIRTIO_NET_HDR_GSO_TCPV4_ECN'.
> > -- Add prefix 'rsc_' to new fields in virtio_net_hdr.
> > -- Drop wrong legacy part.
> > -- Revise maximum payload size for coalesced packets, also revise max
> > ip payload from 65536 to 65535 for TSO4/6/UFO.
> > -- Add default value of new fields member as zero for original packets.
> > -- Drop redundant hint in 'hdr_len' for RSC header size, both
> > send/receive should use the same header after the feature was
> > acknowledged.
> >
> > v1->v2:
> > Feature 22 should not be used by device, replacing it with 41 and 42 for
> > VIRTIO_NET_GUEST_RSC4 and VIRTIO_NET_GUEST_RSC6.
> >
> >
> >
> > Index: content.tex
> > ===================================================================
> > --- content.tex    (revision 569)
> > +++ content.tex    (working copy)
> > @@ -3086,6 +3086,11 @@
> >
> >  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> >      channel.
> > +
> > +\item[VIRTIO_NET_F_GUEST_RSC4 (41)] Driver can receive coalesced IPv4
> > tcp packets.
> > +
> > +\item[VIRTIO_NET_F_GUEST_RSC6 (42)] Driver can receive coalesced IPv6
> > tcp packets.
>
> Several questions here:
>
> - Do we need to clarify the difference between GUEST_TSOX and
> GUEST_RSCX? Are they mutually exclusive?

Sure, for sure we should tell the difference.

> - Can RSC coalesce UDP packets in the future? If not, is it better to
> add TCP in the name of the feature?

OK.

> - Consider it's a brand new feature, do we really want two feature bits?
> (Or just use _F_GUEST_RSC to make sure both ipv4 and ipv6 work)?

Though both IPv4/6 work at the same time for most cases, while the two
features function as independent modules, so it is better to make the
feature bits separately manageable to easy the life(tuning,
debugging,etc) imho.

> > +
> >  \end{description}
> >
> >  \subsubsection{Feature bit requirements}\label{sec:Device Types /
> > Network Device / Feature bits / Feature bit requirements}
> > @@ -3247,8 +3252,12 @@
> >          u8 flags;
> >  #define VIRTIO_NET_HDR_GSO_NONE        0
> >  #define VIRTIO_NET_HDR_GSO_TCPV4       1
> > +#define VIRTIO_NET_HDR_RESERVED        2
> >  #define VIRTIO_NET_HDR_GSO_UDP         3
> >  #define VIRTIO_NET_HDR_GSO_TCPV6       4
> > +#define VIRTIO_NET_HDR_RSC_NONE        5
>
> Why not reuse VIRTIO_NET_HDR_GSO_NONE?

This feature works like general linux GSO from the perspective of
functionality, while the idea was thoroughly derived from a
specification of Microsoft Receive-Segment-Offload(RSC), since new
feature bits have been introduced, it might be clearer to get an
individual 'NONE' flag for it, how about keep it?

> > +#define VIRTIO_NET_HDR_RSC_TCPV4       6
> > +#define VIRTIO_NET_HDR_RSC_TCPV6       7
> >  #define VIRTIO_NET_HDR_GSO_ECN      0x80
> >          u8 gso_type;
> >          le16 hdr_len;
> > @@ -3256,6 +3265,8 @@
> >          le16 csum_start;
> >          le16 csum_offset;
> >          le16 num_buffers;
> > +        le16 rsc_pkts;
> > +        le16 rsc_dup_acks;
> >  };
> >  \end{lstlisting}
> >
> > @@ -3514,6 +3525,24 @@
> >    set: if so, device has validated the packet checksum.
> >    In case of multiple encapsulated protocols, one level of checksums
> >    has been validated.
> > +
> > +\item If one or both VIRTIO_NET_F_GUEST_RSC4, RSC6 have been negotiated,
> > +  while an original packet which have not coalesced any packet is
> > going to
>
> s/have/has/, not a native English speaker, but this sentences is not
> easy to understand.

Ha.., will make an brand new easier sentence for this, thanks for the 
feedback.

> > +  be sent, then the \field{flags} should be set to
> > VIRTIO_NET_HDR_RSC_NONE.
> > +
> > +\item If one or both VIRTIO_NET_F_GUEST_RSC4, RSC6 features have been
> > negotiated,
> > +  and \field{flags} is either RSC4 or RSC6, then \field{rsc_pkts}
> > indicates
> > +  how many packets have been coalesced into this packet. Otherwise it
> > should
> > +  be zero.
> > +
> > +\item If one or both VIRTIO_NET_F_GUEST_RSC4, RSC6 features have been
> > negotiated,
> > +  and \field{flags} is either RSC4 or RSC6, then \field{rsc_dup_acks}
> > indicates
> > +  how many duplicated ack packets have been coalesced into this
> > packet. Otherwise
> > +  it should be zero.
> > +
> > +\item If none of VIRTIO_NET_F_GUEST_RSC4, RSC6 feature bits have been
> > negotiated,
> > +  the 'rsc_pkts' and 'rsc_dup_acks' fields should not be presented in
> > the
> > +  'virtio_net_hdr', which means the header is 4 bytes shorter.
> >  \end{enumerate}
> >
> >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN
> > @@ -3959,6 +3988,8 @@
> >  #define VIRTIO_NET_F_GUEST_TSO6       8
> >  #define VIRTIO_NET_F_GUEST_ECN        9
> >  #define VIRTIO_NET_F_GUEST_UFO        10
> > +#define VIRTIO_NET_F_GUEST_RSC4       41
> > +#define VIRTIO_NET_F_GUEST_RSC6       42
> >
> >  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS       5
> >   #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET   0