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

[Kavanagh, Mark B]

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