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