> >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
