From: "Dey, Souvik" <sodey@sonusnet.com>
To: "Kavanagh, Mark B" <mark.b.kavanagh@intel.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: Fri, 9 Sep 2016 15:33:32 +0000 [thread overview]
Message-ID: <CY1PR03MB1503EBBC768F105F702086B2DAFA0@CY1PR03MB1503.namprd03.prod.outlook.com> (raw)
In-Reply-To: <DC5AD7FA266D86499789B1BCAEC715F85C6FC36D@irsmsx105.ger.corp.intel.com>
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) {
+ PMD_INIT_LOG(ERR, "Mtu should be between VIRTIO_MIN_RX_BUFSIZE and VIRTIO_MAX_RX_PKTLEN \n");
+ 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-09 15:33 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 [this message]
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
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=CY1PR03MB1503EBBC768F105F702086B2DAFA0@CY1PR03MB1503.namprd03.prod.outlook.com \
--to=sodey@sonusnet.com \
--cc=dev@dpdk.org \
--cc=mark.b.kavanagh@intel.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).