Re: [PATCH] virtio_net: Introduce extended RSC feature

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

On Wed, Jan 30, 2019 at 09:42:07AM +0200, Yuri Benditovich wrote:
> 
> 
> On Tue, Jan 29, 2019 at 6:07 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Jan 29, 2019 at 02:52:36PM +0200, Yuri Benditovich wrote:
>     > VIRTIO_NET_F_RSC_EXT feature bit indicates that the device
>     > is able to provide extended RSC information. When the feature
>     > is active and 'gso_type' field in received packet is not GSO_NONE,
>     > the device reports number of coalesced packets in 'csum_start'
>     > field and number of duplicated acks in 'csum_offset' field
>     > and sets VIRTIO_NET_HDR_F_RSC_INFO in 'flags' field.
>     >
>     > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>     > ---
>     >  include/uapi/linux/virtio_net.h | 15 ++++++++++++++-
>     >  1 file changed, 14 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/
>     virtio_net.h
>     > index a3715a3224c1..93c71d714475 100644
>     > --- a/include/uapi/linux/virtio_net.h
>     > +++ b/include/uapi/linux/virtio_net.h
>     > @@ -56,7 +56,7 @@
>     >  #define VIRTIO_NET_F_MQ      22      /* Device supports Receive Flow
>     >                                        * Steering */
>     >  #define VIRTIO_NET_F_CTRL_MAC_ADDR 23        /* Set MAC address */
>     > -
>     > +#define VIRTIO_NET_F_RSC_EXT   61    /* Provides extended RSC info */
>     >  #define VIRTIO_NET_F_STANDBY   62    /* Act as standby for another
>     device
>     >                                        * with the same MAC.
>     >                                        */
>     > @@ -104,6 +104,7 @@ struct virtio_net_config {
>     >  struct virtio_net_hdr_v1 {
>     >  #define VIRTIO_NET_HDR_F_NEEDS_CSUM  1       /* Use csum_start,
>     csum_offset */
>     >  #define VIRTIO_NET_HDR_F_DATA_VALID  2       /* Csum is valid */
>     > +#define VIRTIO_NET_HDR_F_RSC_INFO    4   /* rsc_ext data in csum_ fields
>     */
>     >       __u8 flags;
>     >  #define VIRTIO_NET_HDR_GSO_NONE              0       /* Not a GSO frame
>     */
>     >  #define VIRTIO_NET_HDR_GSO_TCPV4     1       /* GSO frame, IPv4 TCP
>     (TSO) */
>     > @@ -140,6 +141,18 @@ struct virtio_net_hdr_mrg_rxbuf {
>     >       struct virtio_net_hdr hdr;
>     >       __virtio16 num_buffers; /* Number of merged rx buffers */
>     >  };
>     > +
>     > +static inline __virtio16 *virtio_net_rsc_ext_num_packets(
>     > +     struct virtio_net_hdr *hdr)
>     > +{
>     > +     return &hdr->csum_start;
>     > +}
>     > +
>     > +static inline __virtio16 *virtio_net_rsc_ext_num_dupacks(
>     > +     struct virtio_net_hdr *hdr)
>     > +{
>     > +     return &hdr->csum_offset;
>     > +}
>     >  #endif /* ...VIRTIO_NET_NO_LEGACY */

I also wonder why do we want to put this code in the legacy section
and use the legacy virtio_net_hdr as opposed to the new virtio_net_hdr_v1.

> 
>     Coding style is off here. But really I don't think these inlines are
>     needed here. Put them in qemu or something.
> 
> 
> 
> The are copied from qemu as is (what exactly is wrong?).


coding style says:

	Descendants are always substantially shorter than the parent and
	are placed substantially to the right.

placing a line to the left of ( doesn't count as substantially to the
right :)
Maybe start a new line at virtio_net_rsc_ext_num_dupacks.

Lack of documentation is also a problem.

> The reason I place these inlines here is following:
> We pull this include sometimes to virtio-win repo exactly as qemu do.
> If I place them into qemu, then we'll have these inlines in virtio-win and in
> qemu and they are not synchronized.
> 
> So, I suggest to keep them in one common header and fix style problems, if they
> present. 
> Please respond if you still disagree.

Okay but this assumes specific usage. E.g. someone might want
an offset and not a pointer. Or have a struct instance on stack.

Given all above issues (and also header version issues
described above) I'm inclined to say macros are better:


#define virtio_net_rsc_ext_num_packets csum_start
#define virtio_net_rsc_ext_num_dupacks csum_offset


But please in any case also add documentation same as we have for
fields.

> 
>     >  /*
>     > --
>     > 2.17.1
>