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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
