From: "Kavanagh, Mark B" <mark.b.kavanagh@intel.com>
To: "Dey, Souvik" <sodey@sonusnet.com>,
Yuanhan Liu <yuanhan.liu@linux.intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: Re: [dpdk-dev] [PATCH v4]net/virtio: add mtu set in virtio
Date: Mon, 12 Sep 2016 14:47:55 +0000 [thread overview]
Message-ID: <DC5AD7FA266D86499789B1BCAEC715F85C6FDCA0@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <BY1PR03MB1499011D33C91ADFA2517965DAFA0@BY1PR03MB1499.namprd03.prod.outlook.com>
>
>Hi Mark/Liu,
> Thanks to both of you for being so patient with a series of silly errors. I
>have tried to make it better this time. Also there were some un-used variable in the function
>which I have removed. Please check the new patch with all your comments incorporated. Also
>along with the L2_HDR_LEN and L2_CRC_LEN, I have taken in consideration the VLAN_LEN too.
>
>One doubt though,
>"9728 is the largest L2 frame size allowed by the NIC" -- this 9728 size is some constrain
>due to DPDK as the virtio driver in the kernel can support till mtu size of 68 to 65535,
>where as in DPDK pmd we are trying with 64 to 9728.
Yes, I meant the NIC as controlled by a DPDK PMD (I thought this was implicit, given the context of this discussion). I'll be more explicit in future mails though.
>
>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_LEN 4 /* 802.3ac tag (not DMA'd) */
>+
>+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
One thing on this function: it doesn't actually set any fields, but rather just sanitizes 'mtu'.
Maybe this is acceptable, since the calling function (i.e. rte_eth_dev_set_mtu) sets rte_eth_dev->data->mtu?
>+{
>+ struct rte_eth_dev_info dev_info;
>+ uint32_t ether_hdr_len = ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_LEN;
>+ uint32_t frame_size = mtu + ether_hdr_len;
>+
>+ virtio_dev_info_get(dev, &dev_info);
>+
>+ if (mtu < dev_info.min_rx_bufsize || frame_size > dev_info.max_rx_pktlen) {
It's not clear to me whether 'mtu' in this case should be compared with ETHER_MIN_MTU, as per other DPDK drivers, or alternatively whether 'frame_size' should be compared with dev_info.min_rx_bufsize.
Any thoughts?
>+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n",
>+ dev_info.min_rx_bufsize,
As above.
>+ (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,
>
>
>--
>Regards,
>Souvik
>
>-----Original Message-----
>From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com]
>Sent: Friday, September 9, 2016 11:44 AM
>To: Dey, Souvik <sodey@sonusnet.com>; Yuanhan Liu <yuanhan.liu@linux.intel.com>
>Cc: dev@dpdk.org; stephen@networkplumber.org
>Subject: RE: [PATCH v4]net/virtio: add mtu set in virtio
>
>>
>>Hi Mark,
>>
>>Yes I thought I did that change. Sorry once again.. making too many mistakes. Changed it .
>>Thanks.
>>The MTU here is L3 MTU. Setting this will help in reducing the
>>fragmented/multi-segmented packets and also in case we want to reduce
>>the MTU below 1500, to support VXLAN or GRE tunnel for the packets in openstack and cloud
>environments.
>>
>>---
>> drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++
>> 1 file changed, 12 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
>>
>>static int virtio_dev_queue_stats_mapping_set(
>> __rte_unused struct rte_eth_dev *eth_dev, @@ -652,6 +653,16 @@
>>virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
>> PMD_INIT_LOG(ERR, "Failed to disable allmulticast"); }
>>
>>+static int
>>+virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
>>+ struct virtio_hw *hw = dev->data->dev_private;
>>+ if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
>
>If MTU is the L3 MTU as you've indicated, then you need to take account of L2 overhead before
>performing the comparison above.
>Say the user supplies 'mtu' as 9728 - the corresponding minimum frame size is L2_HDR_LEN +
>9728 + L2_CRC_LEN = 9746 bytes, which is larger than the NIC can accommodate (note that 9728
>is the largest L2 frame size allowed by the NIC, not the largest IP packet size).
>
>>+ PMD_INIT_LOG(ERR, "Mtu should be between VIRTIO_MIN_RX_BUFSIZE and
>>VIRTIO_MAX_RX_PKTLEN \n");
>
>Two things on this statement:
>1) in the context of a log message, VIRTIO_XXX_RX_BUFSIZE most likely means very little, and
>as such is not helpful. I suggest going with the format that I included in my earlier mail,
>which prints the numeric value of the min and max rx defines
>2) <micronit> MTU is an initialization, and should be printed as such in a log (i.e. all
>caps) </micronit>
>
>Hope this helps,
>Mark
>
>>+ return -EINVAL;
>>+ }
>>+ return 0;
>>+}
>>+
>> /*
>> * dev_ops for virtio, bare necessities for basic operation
>> */
>>@@ -664,6 +675,7 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
>> .promiscuous_disable = virtio_dev_promiscuous_disable,
>> .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,
>>--
>>
>>--
>>Regards,
>>Souvik
>>-----Original Message-----
>>From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com]
>>Sent: Friday, September 9, 2016 11:05 AM
>>To: Dey, Souvik <sodey@sonusnet.com>; Yuanhan Liu
>><yuanhan.liu@linux.intel.com>
>>Cc: dev@dpdk.org; stephen@networkplumber.org
>>Subject: RE: [PATCH v4]net/virtio: add mtu set in virtio
>>
>>>
>>>Hi Liu,
>>>
>>>Yes agreed your comment. I will definitely remove the declaration as
>>>it is not really required.
>>> So the latest patch will look like this . Yes I did rush a bit to
>>>submit the patch last will correct my suite. So sending the patch in a
>>>reply if we have more comments we can take a look and they re-submit
>>>the final reviewed
>>patch. Thanks for the help though.
>>>
>>>---
>>> drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++
>>> 1 file changed, 12 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
>>>
>>>+static int
>>>+virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
>>>+ struct virtio_hw *hw = dev->data->dev_private;
>>>+ if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
>>>+ PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");
>>
>>Hi Souvik,
>>
>>Why hardcode the values in the PMD_INIT_LOG?
>>
>>Why not do the following: PMD_INIT_LOG(ERR, "MTU should be between %d and %d",
>>
> VIRTIO_MIN_RX_BUFSIZE,
>> VIRTIO_MAX
>_RX_PKTLEN);
>>
>>That way, if the values that the macros evaluate to change, the log
>>will update correspondingly.
>>
>>Also, does 'MTU' in this context relate to the L2 or L3 MTU?
>>
>>>+ return -EINVAL;
>>>+ }
>>>+ return 0;
>>>+}
>>>+
>>> /*
>>> * dev_ops for virtio, bare necessities for basic operation
>>> */
>>>@@ -664,6 +675,7 @@ static const struct eth_dev_ops virtio_eth_dev_ops = {
>>> .promiscuous_disable = virtio_dev_promiscuous_disable,
>>> .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,
>>>--
>>>
>>>--
>>>Regards,
>>>Souvik
>>>
>>>-----Original Message-----
>>>From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
>>>Sent: Friday, September 9, 2016 3:00 AM
>>>To: Dey, Souvik <sodey@sonusnet.com>
>>>Cc: dev@dpdk.org; stephen@networkplumber.org
>>>Subject: Re: [PATCH v4]net/virtio: add mtu set in virtio
>>>
>>>On Wed, Sep 07, 2016 at 04:21:30AM +0000, Dey, Souvik wrote:
>>>> Hi Liu,
>>>> Submitted the version 4 of the patch as you suggested ,
>>>
>>>Your patch is looking much better. But not really, you ignored few of my comments.
>>>
>>>> and have removed the Reviewed by line.
>>>> I have still kept the function definition as to follow the same suit
>>>> as we have done for
>>>other eth_dev_ops.
>>>
>>>That's because most of the method implementions are after the
>>>reference, thus the declaration is needed.
>>>
>>>For your case, I see no good reason to do that. BTW, if you disagree
>>>with my comment, you shoud made a reply, instead of rushing to sending a new version.
>>>
>>>> --
>>>> Regards,
>>>> Souvik
>>>>
>>>> -----Original Message-----
>>>> From: Dey, Souvik
>>>> Sent: Wednesday, September 7, 2016 12:19 AM
>>>> To: dev@dpdk.org; stephen@networkplumber.org;
>>>> yuanhan.liu@linux.intel.com
>>>> Cc: Dey, Souvik <sodey@sonusnet.com>
>>>> Subject: [PATCH v4]net/virtio: add mtu set in virtio
>>>>
>>>>
>>>> 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).
>>>>
>>>> Changes in v4: Incorporated review comments.
>>>> Changes in v3: Corrected few style errors as reported by sys-stv.
>>>> Changes in v2: Incorporated review comments.
>>>
>>>DPDK prefers to put the change log to ...
>>>>
>>>> Signed-off-by: Souvik Dey <sodey@sonusnet.com>
>>>>
>>>> ---
>>>
>>>... here.
>>>
>>>So that they will be showed in mailing list (for review), but they will be gone after
>apply.
>>>In another word, we don't like to see those change log in git history,
>>>but we'd like to see them while review.
>>>
>>>> drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++
>>>> 1 file changed, 12 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
>>>> @@ -92,6 +92,7 @@ static void virtio_mac_addr_add(struct rte_eth_dev
>>>> *dev, static void
>>>virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
>>>static void virtio_mac_addr_set(struct rte_eth_dev *dev,
>>>> struct ether_addr *mac_addr);
>>>> +static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
>>>>
>>>> static int virtio_dev_queue_stats_mapping_set(
>>>> __rte_unused struct rte_eth_dev *eth_dev, @@ -652,6 +653,16 @@
>>>virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
>>>> PMD_INIT_LOG(ERR, "Failed to disable allmulticast"); }
>>>>
>>>> +static int
>>>> +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) {
>>>> + struct virtio_hw *hw = dev->data->dev_private;
>>>> + if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) {
>>>> + PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n");
>>>
>>>I still saw those numbers.
>>>
>>> --yliu
next prev parent reply other threads:[~2016-09-12 14:48 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-07 4:18 souvikdey33
2016-09-07 4:21 ` Dey, Souvik
2016-09-09 7:00 ` Yuanhan Liu
2016-09-09 14:19 ` Dey, Souvik
2016-09-09 15:05 ` Kavanagh, Mark B
2016-09-09 15:33 ` Dey, Souvik
2016-09-09 15:42 ` Yuanhan Liu
2016-09-09 15:43 ` Kavanagh, Mark B
2016-09-09 18:16 ` Dey, Souvik
2016-09-12 14:25 ` Dey, Souvik
2016-09-12 14:47 ` Kavanagh, Mark B [this message]
2016-09-12 15:11 ` Dey, Souvik
2016-09-14 0:25 ` Dey, Souvik
2016-09-14 4:32 ` Yuanhan Liu
2016-09-14 12:15 ` Kavanagh, Mark B
2016-09-14 21:12 ` Dey, Souvik
2016-09-20 7:11 ` Yuanhan Liu
2016-09-20 18:42 ` Dey, Souvik
2016-09-21 2:21 ` Yuanhan Liu
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=DC5AD7FA266D86499789B1BCAEC715F85C6FDCA0@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).