[dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio

Dey, Souvik sodey at sonusnet.com
Wed Oct 5 16:05:04 CEST 2016


Yes Mark, I have modified the patch with the below comments.

drivers/net/virtio/virtio_ethdev.c | 17 +++++++++++++++++
 1 file changed, 17 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_LEN           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_LEN +
+		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 - ether_hdr_len));
+		return -EINVAL;
+	}
+	return 0;
+}

Let mem know if this looks good or we have few more comments. 

--
Regards,
Souvik

-----Original Message-----
From: Kavanagh, Mark B [mailto:mark.b.kavanagh at intel.com] 
Sent: Wednesday, October 5, 2016 4:16 AM
To: Dey, Souvik <sodey at sonusnet.com>; yuanhan.liu at linux.intel.com; stephen at networkplumber.org
Cc: dev at dpdk.org
Subject: RE: [PATCH v7] net/virtio: add set_mtu in virtio

>Hi All,
>	Is there any further comments or modifications required for this 
>patch, or what next steps do you guys suggest here ?

Hi Souvik,

Some minor comments inline.

Thanks,
Mark

>
>--
>Regards,
>Souvik
>
>-----Original Message-----
>From: Dey, Souvik
>Sent: Saturday, October 1, 2016 10:09 AM
>To: mark.b.kavanagh at intel.com; yuanhan.liu at linux.intel.com; 
>stephen at networkplumber.org; dev at dpdk.org
>Subject: RE: [PATCH v7] net/virtio: add set_mtu in virtio
>
>Hi Liu/Stephen/Mark,
>
>	I have submitted Version 7 of this patch. Do let me know if this looks proper.
>
>--
>Regards,
>Souvik
>
>-----Original Message-----
>From: Dey, Souvik
>Sent: Thursday, September 29, 2016 4:32 PM
>To: mark.b.kavanagh at intel.com; yuanhan.liu at linux.intel.com; 
>stephen at networkplumber.org; dev at dpdk.org
>Cc: Dey, Souvik <sodey at sonusnet.com>
>Subject: [PATCH v7] net/virtio: add set_mtu in virtio
>
>
>Virtio interfaces do not currently allow the user to specify a 
>particular Maximum Transmission Unit (MTU).Consequently, the MTU of 
>Virtio interfaces is typically set to the Ethernet default value of 1500.
>This is problematic in the case of cloud deployments, in which a 
>specific (and potentially non-standard) MTU needs to be set by a DHCP 
>server, which needs to be honored by all interfaces across the traffic 
>path.To achieve this Virtio interfaces should support setting of MTU.
>In case when GRE/VXLAN tunneling is used for internal communication, 
>there will be an overhead added by the infrastructure in the packet 
>over and above the ETHER MTU of 1518. So to take care of this overhead 
>in these cases the DHCP server corrects the L3 MTU to 1454. But since 
>virtio interfaces was not having the MTU set functionality that MTU 
>sent by the DHCP server was ignored and the instance will still send 
>packets with 1500 MTU which after encapsulation will become more than 
>1518 and eventually gets dropped in the infrastructure.
>By adding an additional 'set_mtu' function to the Virtio driver, we can 
>honor the MTU sent by the DHCP server. The dhcp server/controller can 
>then leverage this 'set_mtu' functionality to resolve the above 
>mentioned issue of packets getting dropped due to incorrect size.
>
>
>Signed-off-by: Souvik Dey <sodey at sonusnet.com>
>
>---
>Changes in v7:
>- Replaced the CRC_LEN with the merge rx buf header length.
>- Changed the frame_len max validation to VIRTIO_MAX_RX_PKTLEN.
>Changes in v6:
>- Description of change corrected
>- Corrected the identations
>- Corrected the subject line too
>- The From line was also not correct
>- Re-submitting as the below patch was not proper Changes in v5:
>- Fix log message for out-of-bounds MTU parameter in virtio_mtu_set
>- Calculate frame size, based on 'mtu' parameter
>- Corrected the upper bound and lower bound checks in virtio_mtu_set 
>Changes in v4: Incorporated review comments.
>Changes in v3: Corrected few style errors as reported by sys-stv.
>Changes in v2: Incorporated review comments.
>
> 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_LEN           4    /* 802.3ac tag (not DMA'd) */

There should be a blank line between the #define and the function prototype beneath.

>+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_LEN +
>+		hw->vtnet_hdr_size;

I'll rely on Stephen and Yuanhan's judgment for this.

>+	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);

Shouldn't last format print parameter should be (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len)?
i.e PMD_INIT_LOG(ERR, "MTU should........%d\n",
          ETHER_MIN_MTU, (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len));

>+		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,
>--
>2.9.3.windows.1



More information about the dev mailing list