lkml.org 
[lkml]   [2019]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] virtio_net: Introduce extended RSC feature
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
>

\
 
 \ /
  Last update: 2019-01-30 17:29    [W:0.045 / U:50.376 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site