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 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
> > ---
> > 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
>
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
> ---
> 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_INFO4 /* 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
[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
---
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_MQ22 /* 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_CSUM1 /* Use csum_start, csum_offset
*/
#define VIRTIO_NET_HDR_F_DATA_VALID2 /* Csum is valid */
+#define VIRTIO_NET_HDR_F_RSC_INFO 4 /* rsc_ext data in csum_ fields */
__u8 flags;
#define VIRTIO_NET_HDR_GSO_NONE0 /* 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
