From: "Kavanagh, Mark B" <mark.b.kavanagh@intel.com>
To: "Dey, Souvik" <sodey@sonusnet.com>,
"yuanhan.liu@linux.intel.com" <yuanhan.liu@linux.intel.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio
Date: Thu, 6 Oct 2016 13:58:59 +0000 [thread overview]
Message-ID: <DC5AD7FA266D86499789B1BCAEC715F85C72400D@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <BN3PR03MB14946676AD33FC565680FF74DAC70@BN3PR03MB1494.namprd03.prod.outlook.com>
>
>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
next prev parent reply other threads:[~2016-10-06 13:59 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-16 17:13 [dpdk-dev] [PATCH v5]net/virtio: add mtu set " 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DC5AD7FA266D86499789B1BCAEC715F85C72400D@irsmsx105.ger.corp.intel.com \
--to=mark.b.kavanagh@intel.com \
--cc=dev@dpdk.org \
--cc=sodey@sonusnet.com \
--cc=stephen@networkplumber.org \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).