>
>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 at 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
