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.kavan...@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
