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

Kavanagh, Mark B mark.b.kavanagh at intel.com
Thu Oct 6 15:58:59 CEST 2016


>
>Hi Stephen/Liu,
>       Any other comments or suggestions for this. If the below patch looks fine then please
>do suggest the next steps .


Hi Souvik,

Just to let you know that Yuanhan is out of office on account of public holidays in PRC.

AFAIA he should be back online from next week.

Thanks,
Mark

>
>--
>Regards,
>Souvik
>
>-----Original Message-----
>From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Dey, Souvik
>Sent: Wednesday, October 5, 2016 10:05 AM
>To: Kavanagh, Mark B <mark.b.kavanagh at intel.com>; yuanhan.liu at linux.intel.com;
>stephen at networkplumber.org
>Cc: dev at dpdk.org
>Subject: Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio
>
>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