[dpdk-dev] [PATCH v4]net/virtio: add mtu set in virtio
Dey, Souvik
sodey at sonusnet.com
Wed Sep 14 23:12:38 CEST 2016
Hi Mark/Liu,
I know this might be a redundant question, but should I put your names in the Reviewed-by section in v5 ?
--
Regards,
Souvik
-----Original Message-----
From: Kavanagh, Mark B [mailto:mark.b.kavanagh at intel.com]
Sent: Wednesday, September 14, 2016 8:16 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.
Hi Souvik, apologies for the delayed response.
That's what I thought, just wanted to verify that no additional structures should be updated here.
>
>>+{
>>+ 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 ?
Again, this all depends on what 'mtu' means in this context.
Since you mentioned previously that it relates to the packet (i.e. L3) length, and not the frame (i.e. L2) length, I would suggest that the comparison should be:
if (mtu < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen)
Yuanhan, any thoughts on this?
>
>>+ 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