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.kavan...@intel.com]
Sent: Friday, September 9, 2016 11:05 AM
To: Dey, Souvik <sodey at sonusnet.com>; Yuanhan Liu <yuanhan.liu at
linux.intel.com>
Cc: dev at dpdk.org; stephen at 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 at linux.intel.com]
>Sent: Friday, September 9, 2016 3:00 AM
>To: Dey, Souvik <sodey at sonusnet.com>
>Cc: dev at dpdk.org; stephen at 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 at dpdk.org; stephen at networkplumber.org;
>> yuanhan.liu at linux.intel.com
>> Cc: Dey, Souvik <sodey at 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 at 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
