[dpdk-dev] [PATCH v4]net/virtio: add mtu set in virtio

Dey, Souvik sodey at sonusnet.com
Mon Sep 12 17:11:54 CEST 2016


	

-----Original Message-----
From: Kavanagh, Mark B [mailto:mark.b.kavanagh at intel.com] 
Sent: Monday, September 12, 2016 10:48 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 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?
[Dey, Souvik]  Yes , only the sanity check for the mtu is required here and the setting of the call back, else the rte_eth_dev_set_mtu() just returns without setting the MTU in the 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?
[Dey, Souvik] I am not sure why virtio min_rx_bufsize is less than ETHER_MIN_MTU, i think it will be good to have the  compare statement as 
If(frame_size < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) , then error. What do you suggest ?

>+               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 at intel.com]
>Sent: Friday, September 9, 2016 11:44 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 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


More information about the dev mailing list