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

[Kavanagh, Mark B]

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