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

[Kavanagh, Mark B]

>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