Re: [PATCH v6] net/virtio: add set_mtu in virtio

From: Dey, Souvik <hidden>
Date: 2016-09-28 15:31:38

OK sure will do that. 

-----Original Message-----
From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
Sent: Tuesday, September 27, 2016 7:12 PM
To: Dey, Souvik <redacted>
Cc: Stephen Hemminger <stephen@networkplumber.org>; Kavanagh, Mark B <redacted>; dev@dpdk.org
Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio

On Tue, Sep 27, 2016 at 08:44:20PM +0000, Dey, Souvik wrote:

Hi Stephen, 
	So what should be my next steps , should I submit a v7 for this patch or you suggest otherwise.

Yes, please. Another note is please don't use white space for indentation, use TAB instead.

	-yliu

--
Regards,
Souvik

-----Original Message-----
From: Stephen Hemminger [mailto:stephen@networkplumber.org]
Sent: Tuesday, September 27, 2016 2:57 PM
To: Dey, Souvik <redacted>
Cc: Kavanagh, Mark B <redacted>; Yuanhan Liu 
[off-list ref]; dev@dpdk.org
Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio

On Fri, 23 Sep 2016 15:17:37 +0000
"Dey, Souvik" [off-list ref] wrote:

quoted

Hi Liu/Mark/Stephen,

              I have tried to modify the code with all of your latest comments. Do let me know if this looks fine or you have more comments.



Changes done :

-- max frame ize is compare to VIRTIO_MAX_RX_PKTLEN instead of 
dev_info.max_rx_pktlen

-- removed the CRC_LEN from the ether_len calculation and added the 
merge rx buf hdr len. ether_hdr_len = ETHER_HDR_LEN + VLAN_TAG_SIZE 
+
hw->vtnet_hdr_size

-- Still retained the VLAN Size as the worst case scenario.





--

drivers/net/virtio/virtio_ethdev.c | 16 ++++++++++++++++

1 file changed, 16 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c

b/drivers/net/virtio/virtio_ethdev.c

index 423c597..1dbfea6 100644

--- a/drivers/net/virtio/virtio_ethdev.c

+++ b/drivers/net/virtio/virtio_ethdev.c

@@ -653,12 +653,20 @@ virtio_dev_allmulticast_disable(struct

rte_eth_dev *dev)

                PMD_INIT_LOG(ERR, "Failed to disable allmulticast");

}



+#define VLAN_TAG_SIZE           4    /* 802.3ac tag (not DMA'd) */

+

+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)

+{

+            struct virtio_hw *hw = dev->data->dev_private;

+            uint32_t ether_hdr_len = ETHER_HDR_LEN + VLAN_TAG_SIZE 
+ +
+ hw->vtnet_hdr_size;

+            uint32_t frame_size = mtu + ether_hdr_len;

+

+            if (mtu < ETHER_MIN_MTU || frame_size > 
+ VIRTIO_MAX_RX_PKTLEN ) {

+                           PMD_INIT_LOG(ERR, "MTU should be between 
+ %d and %d\n",

+                                         ETHER_MIN_MTU, 
+ VIRTIO_MAX_RX_PKTLEN);

+                           return -EINVAL;

+            }

+            return 0;

+}



/*

  * dev_ops for virtio, bare necessities for basic operation

  */

@@ -677,7 +685,6 @@ static const struct eth_dev_ops

virtio_eth_dev_ops = {

             .allmulticast_enable     = virtio_dev_allmulticast_enable,

             .allmulticast_disable    = virtio_dev_allmulticast_disable,

+            .mtu_set                 = virtio_mtu_set,

             .dev_infos_get           = virtio_dev_info_get,

             .stats_get               = virtio_dev_stats_get,

             .xstats_get              = virtio_dev_xstats_get,

--

Please do let me know if this looks good to you all. Thanks



--

Regards,

Souvik



-----Original Message-----
From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com]
Sent: Friday, September 23, 2016 5:08 AM
To: Yuanhan Liu <redacted>; Stephen Hemminger 
[off-list ref]
Cc: Dey, Souvik <redacted>; dev@dpdk.org
Subject: RE: [PATCH v6] net/virtio: add set_mtu in virtio

quoted

Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio

quoted

On Wed, Sep 21, 2016 at 06:45:05PM -0700, Stephen Hemminger wrote:

quoted

On Thu, 22 Sep 2016 00:08:38 +0000

quoted

"Dey, Souvik" <sodey@sonusnet.com<mailto:sodey@sonusnet.com>> wrote:

quoted

quoted

quoted

quoted
quoted
Answers inline.

quoted

quoted
quoted

quoted

quoted
quoted
--

quoted

quoted
quoted
Regards,

quoted

quoted
quoted
Souvik

quoted

quoted
quoted

quoted

quoted
quoted
-----Original Message-----

quoted

From: Stephen Hemminger [mailto:stephen@networkplumber.org]

quoted

Sent: Wednesday, September 21, 2016 7:22 PM

quoted

To: Dey, Souvik <sodey@sonusnet.com<mailto:sodey@sonusnet.com>>

quoted

Cc: 
mark.b.kavanagh@intel.com<mailto:mark.b.kavanagh@intel.com>;
yuanhan.liu@linux.intel.com<mailto:yuanhan.liu@linux.intel.com>
;

quoted

dev@dpdk.org<mailto:dev@dpdk.org>

quoted

Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio

quoted

quoted
quoted

quoted

On Wed, 21 Sep 2016 19:11:47 -0400

quoted

Dey <sodey@sonusnet.com<mailto:sodey@sonusnet.com>> wrote:

quoted

quoted
quoted

quoted

quoted
quoted
quoted

quoted

quoted
quoted
quoted
+

quoted

+#define VLAN_TAG_SIZE           4    /* 802.3ac tag (not DMA'd) */

quoted

quoted
quoted
quoted
+

quoted

+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t
+mtu) {

quoted

quoted
quoted
quoted
+       struct rte_eth_dev_info dev_info;

quoted

+       uint32_t ether_hdr_len = ETHER_HDR_LEN + 
+ ETHER_CRC_LEN
+ + VLAN_TAG_SIZE;

quoted

+       uint32_t frame_size = mtu + ether_hdr_len;

quoted

quoted
quoted
quoted
+

quoted

+       virtio_dev_info_get(dev, &dev_info);

quoted

quoted
quoted
quoted
+

quoted

+       if (mtu < ETHER_MIN_MTU || frame_size >
+ dev_info.max_rx_pktlen) {

quoted

+               PMD_INIT_LOG(ERR, "MTU should be between %d 
+ and %d\n",

quoted

+                               ETHER_MIN_MTU,

quoted

+                               (dev_info.max_rx_pktlen - 
+ ether_hdr_len));

quoted

quoted
quoted
quoted
+               return -EINVAL;

quoted

quoted
quoted
quoted
+       }

quoted

quoted
quoted
quoted
+       return 0;

quoted

quoted
quoted
quoted
+}

quoted

quoted
quoted

quoted

I am fine with the general idea of this patch but:

quoted

  1. Calling virtio_dev_info_get is needlessly wasteful when 
all you want

quoted

     is to access the max packet length. Since max_rx_pktlen is 
always

quoted

     VIRTIO_MAX_RX_PKTLEN, please just use that.

quoted

[Dey, Souvik] I am using the virtio_dev_info_get as in future

quoted

quoted
quoted
can/may support the

quoted

max_rx_pktlen as a variable to be set by  the application. This 
will

quoted

keep the changes future proof. As we need to support till 65535 instead of 9728 as the linux does.

quoted

quoted

quoted

Fine, then just dereference hw->rx_max_pktlen. Driver code 
can/should

quoted

reference its own data directly.

quoted

Dey, maybe you could just use VIRTIO_MAX_RX_PKTLEN here, like what 
you

quoted

did in early versions.

quoted

quoted
quoted

quoted

  2. Defining VLAN_TAG_SIZE is irrelevant if doing vlan offload.

quoted

[Dey, Souvik] vlan offload is not mandatory. Se again still 
have

quoted

quoted
quoted
vlan being sent up to

quoted

the application. In that case we need to consider the vlan length in the Ethernet size.

quoted

quoted

quoted

The code needs to handle both vlan offload (or not), correctly. 
You

quoted

are assuming the worst case here.

quoted

I think we are fine here to assume worst case.

quoted

quoted
quoted

quoted

  3. Virtio doesn't insert CRC, therefore CRC_LEN is irrelevant

quoted

[Dey, Souvik] I am not sure of this. Mark commented earlier to consider this length too.

quoted

Mark what do you suggest ?

quoted

quoted

quoted

Actually, the thing that matters is the size of the merge rx buf header, not the CRC.

This patch is good enough for now, but not fully correct.
The actual max value depends on the host negotiation protocol, ie merge rx buf is only sometimes used.

There was some discussion on netdev mailing list to pass MTU from host to guest in virtio, not sure where that landed.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help