2019-01-29 12:54:32

by Yuri Benditovich

[permalink] [raw]
Subject: [PATCH] virtio_net: Introduce extended RSC feature

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 <[email protected]>
---
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 */

/*
--
2.17.1



2019-01-29 16:08:41

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH] virtio_net: Introduce extended RSC feature

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 <[email protected]>
> ---
> 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 */

Coding style is off here. But really I don't think these inlines are
needed here. Put them in qemu or something.

> /*
> --
> 2.17.1

2019-01-30 16:28:55

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [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 <[email protected]> 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 <[email protected]>
> > ---
> >? 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
>