-----Original Message-----
From: Kavanagh, Mark B [mailto:mark.b.kavan...@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
