From: "Kavanagh, Mark B" <mark.b.kavanagh@intel.com>
To: souvikdey33 <sodey@sonusnet.com>, "dev@dpdk.org" <dev@dpdk.org>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"yuanhan.liu@linux.intel.com" <yuanhan.liu@linux.intel.com>
Subject: Re: [dpdk-dev] [PATCH v5]net/virtio: add mtu set in virtio
Date: Wed, 21 Sep 2016 15:22:55 +0000 [thread overview]
Message-ID: <DC5AD7FA266D86499789B1BCAEC715F85C719B5B@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <20160916171357.69640-1-sodey@sonusnet.com>
>
>
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
next prev parent reply other threads:[~2016-09-21 15:22 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-16 17:13 souvikdey33
2016-09-21 15:22 ` Kavanagh, Mark B [this message]
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
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=DC5AD7FA266D86499789B1BCAEC715F85C719B5B@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).