* [dpdk-dev] [PATCH v5]net/virtio: add mtu set in virtio @ 2016-09-16 17:13 souvikdey33 2016-09-21 15:22 ` Kavanagh, Mark B 2016-09-22 13:56 ` [dpdk-dev] [PATCH v6] net/virtio: add set_mtu " Dey 0 siblings, 2 replies; 16+ messages in thread From: souvikdey33 @ 2016-09-16 17:13 UTC (permalink / raw) To: dev, stephen, yuanhan.liu, mark.b.kavanagh; +Cc: souvikdey33 Virtio interfaces should also support setting of mtu, as in case of cloud it is expected to have the consistent mtu across the infrastructure that the dhcp server sends and not hardcoded to 1500(default). In case when GRE/VXLAN tunneling is used, it adds overheads to the total size of 1518, and in that cases the DHCP server corrects the L3 MTU to 1454 to take care of the overhead. BUt since virtio interfaces was not having the mtu set that mtu sent by dhcp was ignored and teh instance will still send packets with 1500 mtu which afetr encapsulation will become more than 1518 and eventually gets dropped in the infrastructure. This issue can be solved by honoring the mtu 1454 sent by dhcp server, which this below patch will take care. Changes in v5: Incorporated review comments. Changes in v4: Incorporated review comments. Changes in v3: Corrected few style errors as reported by sys-stv. Changes in v2: Incorporated review comments. Signed-off-by: Souvik Dey <sodey@sonusnet.com> --- drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index 07d6449..da16ad4 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_SIZE 4 /* 802.3ac tag (not DMA'd) */ + +static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) +{ + struct rte_eth_dev_info dev_info; + uint32_t ether_hdr_len = ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE; + uint32_t frame_size = mtu + ether_hdr_len; + + virtio_dev_info_get(dev, &dev_info); + + if (frame_size < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) { + PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", + (ETHER_MIN_MTU - ether_hdr_len), + (dev_info.max_rx_pktlen - ether_hdr_len)); + return -EINVAL; + } + return 0; +} @@ -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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v5]net/virtio: add mtu set in virtio 2016-09-16 17:13 [dpdk-dev] [PATCH v5]net/virtio: add mtu set in virtio souvikdey33 @ 2016-09-21 15:22 ` Kavanagh, Mark B 2016-09-22 13:56 ` [dpdk-dev] [PATCH v6] net/virtio: add set_mtu " Dey 1 sibling, 0 replies; 16+ messages in thread From: Kavanagh, Mark B @ 2016-09-21 15:22 UTC (permalink / raw) To: souvikdey33, dev, stephen, yuanhan.liu > > Hi Souvik, There are some very basic errors in this patch, particularly with respect to format. Review comments are inline - please address same and resubmit the patch. I also recommend running $DPDK_DIR/utilities/checkpatch.py on any future submissions. Thanks, Mark > As a general comment, please capitalize 'MTU' throughout the commit message. >Virtio interfaces should also support setting of mtu, as in case of cloud 'also' is redundant in the above sentence - please remove it. >it is expected to have the consistent mtu across the infrastructure that >the dhcp server sends and not hardcoded to 1500(default). Try to make the problem statement here more general, before going into specifics. i.e. something along the lines of "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, and honored by all interfaces across the traffic path. ...., etc., etc." >In case when GRE/VXLAN tunneling is used, it adds overheads to the total Please be specific in your description - what is 'it' in the above context? >size of 1518, and in that cases the DHCP server corrects the L3 MTU to 1454 >to take care of the overhead. BUt since virtio interfaces was not having the Should be lowercase 'u' in 'But' >mtu set that mtu sent by dhcp was ignored and teh instance will still send Typo - 'teh' >packets with 1500 mtu which afetr encapsulation will become more than 1518 Typo - 'afetr' >and eventually gets dropped in the infrastructure. This issue can be solved >by honoring the mtu 1454 sent by dhcp server, which this below patch will >take care. Explain how the patch resolves the issue (i.e. by adding an additional 'set_mtu' function to the Virtio driver, which can be leveraged by the dhcp server/controller). > > >Changes in v5: Incorporated review comments. It is implicit that a new patch version incorporates review comments. To make things easier for the reviewer, please provide concise descriptions of the changes made/review comments addressed. For example: v5: - Fix log message for out-of-bounds MTU parameter in virtio_mtu_set - Calculate frame size, based on 'mtu' parameter, for upper bounds check in virtio_mtu_set - .... - etc., etc. >Changes in v4: Incorporated review comments. >Changes in v3: Corrected few style errors as reported by sys-stv. >Changes in v2: Incorporated review comments. > >Signed-off-by: Souvik Dey <sodey@sonusnet.com> > >--- Version history should not be part of the commit message - please move it to below this dashed line. > drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > >diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >index 07d6449..da16ad4 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_SIZE 4 /* 802.3ac tag (not DMA'd) */ >+ >+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) Additional whitespace between 'int' and 'virtio_mtu_set' - please remove. >+{ >+ struct rte_eth_dev_info dev_info; >+ uint32_t ether_hdr_len = ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE; >+ uint32_t frame_size = mtu + ether_hdr_len; >+ >+ virtio_dev_info_get(dev, &dev_info); >+ >+ if (frame_size < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) { >+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", >+ (ETHER_MIN_MTU - ether_hdr_len), >+ (dev_info.max_rx_pktlen - ether_hdr_len)); >+ return -EINVAL; >+ } >+ return 0; >+} >@@ -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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v6] net/virtio: add set_mtu in virtio 2016-09-16 17:13 [dpdk-dev] [PATCH v5]net/virtio: add mtu set in virtio souvikdey33 2016-09-21 15:22 ` Kavanagh, Mark B @ 2016-09-22 13:56 ` Dey 2016-09-29 20:31 ` [dpdk-dev] [PATCH v7] " Dey 1 sibling, 1 reply; 16+ messages in thread From: Dey @ 2016-09-22 13:56 UTC (permalink / raw) To: mark.b.kavanagh, yuanhan.liu, stephen, dev; +Cc: Dey 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 acheive 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@sonusnet.com> --- 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 | 19 +++++++++++++++++++ 1 file changed, 19 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_SIZE 4 /* 802.3ac tag (not DMA'd) */ + +static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) +{ + struct rte_eth_dev_info dev_info; + uint32_t ether_hdr_len = ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE; + uint32_t frame_size = mtu + ether_hdr_len; + + virtio_dev_info_get(dev, &dev_info); + + if (mtu < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) { + PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", + ETHER_MIN_MTU, + (dev_info.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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-09-22 13:56 ` [dpdk-dev] [PATCH v6] net/virtio: add set_mtu " Dey @ 2016-09-29 20:31 ` Dey 2016-10-01 14:08 ` Dey, Souvik ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Dey @ 2016-09-29 20:31 UTC (permalink / raw) To: mark.b.kavanagh, yuanhan.liu, stephen, dev; +Cc: Dey 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 acheive 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@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) */ +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); + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-09-29 20:31 ` [dpdk-dev] [PATCH v7] " Dey @ 2016-10-01 14:08 ` Dey, Souvik 2016-10-04 23:18 ` Dey, Souvik 2016-10-09 3:52 ` Yuanhan Liu 2 siblings, 0 replies; 16+ messages in thread From: Dey, Souvik @ 2016-10-01 14:08 UTC (permalink / raw) To: mark.b.kavanagh, yuanhan.liu, stephen, dev 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@intel.com; yuanhan.liu@linux.intel.com; stephen@networkplumber.org; dev@dpdk.org Cc: Dey, Souvik <sodey@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 acheive 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@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) */ +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); + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-09-29 20:31 ` [dpdk-dev] [PATCH v7] " Dey 2016-10-01 14:08 ` Dey, Souvik @ 2016-10-04 23:18 ` Dey, Souvik 2016-10-05 8:15 ` Kavanagh, Mark B 2016-10-09 3:52 ` Yuanhan Liu 2 siblings, 1 reply; 16+ messages in thread From: Dey, Souvik @ 2016-10-04 23:18 UTC (permalink / raw) To: mark.b.kavanagh, yuanhan.liu, stephen; +Cc: dev Hi All, Is there any further comments or modifications required for this patch, or what next steps do you guys suggest here ? -- Regards, Souvik -----Original Message----- From: Dey, Souvik Sent: Saturday, October 1, 2016 10:09 AM To: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com; stephen@networkplumber.org; dev@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@intel.com; yuanhan.liu@linux.intel.com; stephen@networkplumber.org; dev@dpdk.org Cc: Dey, Souvik <sodey@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 acheive 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@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) */ +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); + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-04 23:18 ` Dey, Souvik @ 2016-10-05 8:15 ` Kavanagh, Mark B 2016-10-05 14:05 ` Dey, Souvik 0 siblings, 1 reply; 16+ messages in thread From: Kavanagh, Mark B @ 2016-10-05 8:15 UTC (permalink / raw) To: Dey, Souvik, yuanhan.liu, stephen; +Cc: dev >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@intel.com; yuanhan.liu@linux.intel.com; stephen@networkplumber.org; >dev@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@intel.com; yuanhan.liu@linux.intel.com; stephen@networkplumber.org; >dev@dpdk.org >Cc: Dey, Souvik <sodey@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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-05 8:15 ` Kavanagh, Mark B @ 2016-10-05 14:05 ` Dey, Souvik 2016-10-06 13:39 ` Dey, Souvik 0 siblings, 1 reply; 16+ messages in thread From: Dey, Souvik @ 2016-10-05 14:05 UTC (permalink / raw) To: Kavanagh, Mark B, yuanhan.liu, stephen; +Cc: dev 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@intel.com] Sent: Wednesday, October 5, 2016 4:16 AM To: Dey, Souvik <sodey@sonusnet.com>; yuanhan.liu@linux.intel.com; stephen@networkplumber.org Cc: dev@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@intel.com; yuanhan.liu@linux.intel.com; >stephen@networkplumber.org; dev@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@intel.com; yuanhan.liu@linux.intel.com; >stephen@networkplumber.org; dev@dpdk.org >Cc: Dey, Souvik <sodey@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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-05 14:05 ` Dey, Souvik @ 2016-10-06 13:39 ` Dey, Souvik 2016-10-06 13:58 ` Kavanagh, Mark B 0 siblings, 1 reply; 16+ messages in thread From: Dey, Souvik @ 2016-10-06 13:39 UTC (permalink / raw) To: Dey, Souvik, Kavanagh, Mark B, yuanhan.liu, stephen; +Cc: dev Hi Stephen/Liu, Any other comments or suggestions for this. If the below patch looks fine then please do suggest the next steps . -- Regards, Souvik -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik Sent: Wednesday, October 5, 2016 10:05 AM To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; yuanhan.liu@linux.intel.com; stephen@networkplumber.org Cc: dev@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@intel.com] Sent: Wednesday, October 5, 2016 4:16 AM To: Dey, Souvik <sodey@sonusnet.com>; yuanhan.liu@linux.intel.com; stephen@networkplumber.org Cc: dev@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@intel.com; yuanhan.liu@linux.intel.com; >stephen@networkplumber.org; dev@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@intel.com; yuanhan.liu@linux.intel.com; >stephen@networkplumber.org; dev@dpdk.org >Cc: Dey, Souvik <sodey@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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-06 13:39 ` Dey, Souvik @ 2016-10-06 13:58 ` Kavanagh, Mark B 2016-10-06 22:29 ` Stephen Hemminger 0 siblings, 1 reply; 16+ messages in thread From: Kavanagh, Mark B @ 2016-10-06 13:58 UTC (permalink / raw) To: Dey, Souvik, yuanhan.liu, stephen; +Cc: dev > >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@dpdk.org] On Behalf Of Dey, Souvik >Sent: Wednesday, October 5, 2016 10:05 AM >To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; yuanhan.liu@linux.intel.com; >stephen@networkplumber.org >Cc: dev@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@intel.com] >Sent: Wednesday, October 5, 2016 4:16 AM >To: Dey, Souvik <sodey@sonusnet.com>; yuanhan.liu@linux.intel.com; stephen@networkplumber.org >Cc: dev@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@intel.com; yuanhan.liu@linux.intel.com; >>stephen@networkplumber.org; dev@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@intel.com; yuanhan.liu@linux.intel.com; >>stephen@networkplumber.org; dev@dpdk.org >>Cc: Dey, Souvik <sodey@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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-06 13:58 ` Kavanagh, Mark B @ 2016-10-06 22:29 ` Stephen Hemminger 2016-10-07 2:06 ` Dey, Souvik 0 siblings, 1 reply; 16+ messages in thread From: Stephen Hemminger @ 2016-10-06 22:29 UTC (permalink / raw) To: Kavanagh, Mark B; +Cc: Dey, Souvik, yuanhan.liu, dev I think current patch is fine. If someone has later problem with it on some scenario we can fix the bug then. Please merge ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-06 22:29 ` Stephen Hemminger @ 2016-10-07 2:06 ` Dey, Souvik 0 siblings, 0 replies; 16+ messages in thread From: Dey, Souvik @ 2016-10-07 2:06 UTC (permalink / raw) To: Stephen Hemminger, Kavanagh, Mark B; +Cc: yuanhan.liu, dev Hi Stephen, As I am new to this patch submission, I did not get what you exactly meant my “Please merge”, do you mean that you Ack the patch and it can be upstreamed or you want me submit a new version or something. Sorry for my ignorance. Thanks -- Souvik From: Stephen Hemminger [mailto:stephen@networkplumber.org] Sent: Thursday, October 6, 2016 6:30 PM To: Kavanagh, Mark B <mark.b.kavanagh@intel.com> Cc: Dey, Souvik <sodey@sonusnet.com>; yuanhan.liu@linux.intel.com; dev@dpdk.org Subject: Re: [PATCH v7] net/virtio: add set_mtu in virtio I think current patch is fine. If someone has later problem with it on some scenario we can fix the bug then. Please merge ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-09-29 20:31 ` [dpdk-dev] [PATCH v7] " Dey 2016-10-01 14:08 ` Dey, Souvik 2016-10-04 23:18 ` Dey, Souvik @ 2016-10-09 3:52 ` Yuanhan Liu 2016-10-10 10:49 ` Kavanagh, Mark B 2 siblings, 1 reply; 16+ messages in thread From: Yuanhan Liu @ 2016-10-09 3:52 UTC (permalink / raw) To: Dey; +Cc: mark.b.kavanagh, stephen, dev On Thu, Sep 29, 2016 at 04:31:30PM -0400, Dey wrote: > /* > * 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 Your patch is malformed: I got an error while trying to apply it. patch: **** malformed patch at line 167: * dev_ops for virtio, bare necessities for basic operation Seems like the way you were generating the patch is wrong. Anyway, I applied it manually, with the "- frame_size" fix as well as few more minor coding style fixes. So applied to dpdk-next-virtio. --yliu ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-09 3:52 ` Yuanhan Liu @ 2016-10-10 10:49 ` Kavanagh, Mark B 2016-10-11 4:01 ` Yuanhan Liu 0 siblings, 1 reply; 16+ messages in thread From: Kavanagh, Mark B @ 2016-10-10 10:49 UTC (permalink / raw) To: Yuanhan Liu, Dey; +Cc: stephen, dev > >On Thu, Sep 29, 2016 at 04:31:30PM -0400, Dey wrote: >> /* >> * 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 > >Your patch is malformed: I got an error while trying to apply it. > > patch: **** malformed patch at line 167: * dev_ops for virtio, > bare necessities for basic operation > >Seems like the way you were generating the patch is wrong. > >Anyway, I applied it manually, with the "- frame_size" fix as well >as few more minor coding style fixes. > >So applied to dpdk-next-virtio. Hi Yuanhan/Souvik, Given my contributions to this patch (and in particular since comments from here - http://dpdk.org/ml/archives/dev/2016-September/047208.html - were copied directly into the commit message), I think that I should have been added as co-author of the patch? Let me know if you think that I am mistaken. Thanks in advance, Mark > > --yliu ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-10 10:49 ` Kavanagh, Mark B @ 2016-10-11 4:01 ` Yuanhan Liu 2016-10-11 8:12 ` Kavanagh, Mark B 0 siblings, 1 reply; 16+ messages in thread From: Yuanhan Liu @ 2016-10-11 4:01 UTC (permalink / raw) To: Kavanagh, Mark B; +Cc: Dey, stephen, dev On Mon, Oct 10, 2016 at 10:49:22AM +0000, Kavanagh, Mark B wrote: > > > >On Thu, Sep 29, 2016 at 04:31:30PM -0400, Dey wrote: > >> /* > >> * 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 > > > >Your patch is malformed: I got an error while trying to apply it. > > > > patch: **** malformed patch at line 167: * dev_ops for virtio, > > bare necessities for basic operation > > > >Seems like the way you were generating the patch is wrong. > > > >Anyway, I applied it manually, with the "- frame_size" fix as well > >as few more minor coding style fixes. > > > >So applied to dpdk-next-virtio. > > Hi Yuanhan/Souvik, > > Given my contributions to this patch (and in particular since comments from here - http://dpdk.org/ml/archives/dev/2016-September/047208.html - were copied directly into the commit message), I think that I should have been added as co-author of the patch? Mark, I appreciate your contributions here. But for this case, I think it might not be enough for adding you as the co-author: you don't co-write the code with Souvik after all. However, I'd suggest you to add your "Reviewed-by" if a patch looks to you after your review effort. This is another way to recognize your contributions to a patch. Thanks. --yliu > > Let me know if you think that I am mistaken. > > Thanks in advance, > Mark > > > > --yliu ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio 2016-10-11 4:01 ` Yuanhan Liu @ 2016-10-11 8:12 ` Kavanagh, Mark B 0 siblings, 0 replies; 16+ messages in thread From: Kavanagh, Mark B @ 2016-10-11 8:12 UTC (permalink / raw) To: Yuanhan Liu; +Cc: Dey, stephen, dev >-----Original Message----- >From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] >Sent: Tuesday, October 11, 2016 5:01 AM >To: Kavanagh, Mark B <mark.b.kavanagh@intel.com> >Cc: Dey <sodey@sonusnet.com>; stephen@networkplumber.org; dev@dpdk.org >Subject: Re: [PATCH v7] net/virtio: add set_mtu in virtio > >On Mon, Oct 10, 2016 at 10:49:22AM +0000, Kavanagh, Mark B wrote: >> > >> >On Thu, Sep 29, 2016 at 04:31:30PM -0400, Dey wrote: >> >> /* >> >> * 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 >> > >> >Your patch is malformed: I got an error while trying to apply it. >> > >> > patch: **** malformed patch at line 167: * dev_ops for virtio, >> > bare necessities for basic operation >> > >> >Seems like the way you were generating the patch is wrong. >> > >> >Anyway, I applied it manually, with the "- frame_size" fix as well >> >as few more minor coding style fixes. >> > >> >So applied to dpdk-next-virtio. >> >> Hi Yuanhan/Souvik, >> >> Given my contributions to this patch (and in particular since comments from here - >http://dpdk.org/ml/archives/dev/2016-September/047208.html - were copied directly into the >commit message), I think that I should have been added as co-author of the patch? > >Mark, > >I appreciate your contributions here. But for this case, I think it >might not be enough for adding you as the co-author: you don't co-write >the code with Souvik after all. > >However, I'd suggest you to add your "Reviewed-by" if a patch looks to >you after your review effort. This is another way to recognize your >contributions to a patch. No problem Yuanhan - thanks for clarifying. Reviewed-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > >Thanks. > > --yliu > > >> >> Let me know if you think that I am mistaken. >> >> Thanks in advance, >> Mark >> > >> > --yliu ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-10-11 8:13 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-16 17:13 [dpdk-dev] [PATCH v5]net/virtio: add mtu set in virtio souvikdey33 2016-09-21 15:22 ` Kavanagh, Mark B 2016-09-22 13:56 ` [dpdk-dev] [PATCH v6] net/virtio: add set_mtu " Dey 2016-09-29 20:31 ` [dpdk-dev] [PATCH v7] " Dey 2016-10-01 14:08 ` Dey, Souvik 2016-10-04 23:18 ` Dey, Souvik 2016-10-05 8:15 ` Kavanagh, Mark B 2016-10-05 14:05 ` Dey, Souvik 2016-10-06 13:39 ` Dey, Souvik 2016-10-06 13:58 ` Kavanagh, Mark B 2016-10-06 22:29 ` Stephen Hemminger 2016-10-07 2:06 ` Dey, Souvik 2016-10-09 3:52 ` Yuanhan Liu 2016-10-10 10:49 ` Kavanagh, Mark B 2016-10-11 4:01 ` Yuanhan Liu 2016-10-11 8:12 ` Kavanagh, Mark B
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).