From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 43661A04DC; Tue, 20 Oct 2020 10:13:49 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A804ABBAE; Tue, 20 Oct 2020 10:13:42 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 41253BAEE for ; Tue, 20 Oct 2020 10:13:40 +0200 (CEST) IronPort-SDR: lwYnoqpPoH6KEUus0d5AQs5GOdnZbmMJoHruaSEGgTSAhs9Wrozlqb81hMXJ/UmBCZOc50WzQV evXAxOazz2RA== X-IronPort-AV: E=McAfee;i="6000,8403,9779"; a="154957493" X-IronPort-AV: E=Sophos;i="5.77,396,1596524400"; d="scan'208";a="154957493" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Oct 2020 01:13:38 -0700 IronPort-SDR: FZvg56SNas40dNd+JhZDRO2Lt77U4bfWS0FD5QW/x6+/KbY34Ema3tptXyMua9h7HP4ueN0gn7 bOJ09GR8LtBg== X-IronPort-AV: E=Sophos;i="5.77,396,1596524400"; d="scan'208";a="347773391" Received: from kmabbasi-mobl1.ger.corp.intel.com (HELO [10.213.247.249]) ([10.213.247.249]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Oct 2020 01:13:35 -0700 To: "Yang, SteveX" , "Zhang, Qi Z" , "Ananyev, Konstantin" , "dev@dpdk.org" Cc: "Zhao1, Wei" , "Guo, Jia" , "Yang, Qiming" , "Wu, Jingjing" , "Xing, Beilei" , "Stokes, Ian" References: <20200923040909.73418-1-stevex.yang@intel.com> <20200928065541.7520-1-stevex.yang@intel.com> <20200928065541.7520-4-stevex.yang@intel.com> <8459e979b76c43cdbd5a9fbd809f9b00@intel.com> <6ad9e3ec00194e31891d97849135655c@intel.com> <7704b7ce95fd4db2a9c6a8a33c3f0805@intel.com> <77ac2293-e532-e702-2370-c07cdd957c57@intel.com> From: Ferruh Yigit Message-ID: Date: Tue, 20 Oct 2020 09:13:31 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4 3/5] net/ice: fix max mtu size packets with vlan tag cannot be received by default X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/20/2020 3:57 AM, Yang, SteveX wrote: > > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Tuesday, October 20, 2020 2:05 AM >> To: Yang, SteveX ; Zhang, Qi Z >> ; Ananyev, Konstantin >> ; dev@dpdk.org >> Cc: Zhao1, Wei ; Guo, Jia ; Yang, >> Qiming ; Wu, Jingjing ; >> Xing, Beilei ; Stokes, Ian >> Subject: Re: [dpdk-dev] [PATCH v4 3/5] net/ice: fix max mtu size packets >> with vlan tag cannot be received by default >> >> On 10/19/2020 4:07 AM, Yang, SteveX wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ferruh Yigit >>>> Sent: Wednesday, October 14, 2020 11:38 PM >>>> To: Zhang, Qi Z ; Yang, SteveX >>>> ; Ananyev, Konstantin >>>> ; dev@dpdk.org >>>> Cc: Zhao1, Wei ; Guo, Jia ; >>>> Yang, Qiming ; Wu, Jingjing >>>> ; Xing, Beilei ; >>>> Stokes, Ian >>>> Subject: Re: [dpdk-dev] [PATCH v4 3/5] net/ice: fix max mtu size >>>> packets with vlan tag cannot be received by default >>>> >>>> On 9/30/2020 3:32 AM, Zhang, Qi Z wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Yang, SteveX >>>>>> Sent: Wednesday, September 30, 2020 9:32 AM >>>>>> To: Zhang, Qi Z ; Ananyev, Konstantin >>>>>> ; dev@dpdk.org >>>>>> Cc: Zhao1, Wei ; Guo, Jia ; >>>>>> Yang, Qiming ; Wu, Jingjing >>>>>> ; Xing, Beilei >>>>>> Subject: RE: [PATCH v4 3/5] net/ice: fix max mtu size packets with >>>>>> vlan tag cannot be received by default >>>>>> >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Zhang, Qi Z >>>>>>> Sent: Wednesday, September 30, 2020 8:35 AM >>>>>>> To: Ananyev, Konstantin ; Yang, >>>> SteveX >>>>>>> ; dev@dpdk.org >>>>>>> Cc: Zhao1, Wei ; Guo, Jia >>>>>>> ; Yang, Qiming ; Wu, >>>>>>> Jingjing ; Xing, Beilei >>>>>>> >>>>>>> Subject: RE: [PATCH v4 3/5] net/ice: fix max mtu size packets with >>>>>>> vlan tag cannot be received by default >>>>>>> >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Ananyev, Konstantin >>>>>>>> Sent: Wednesday, September 30, 2020 7:02 AM >>>>>>>> To: Zhang, Qi Z ; Yang, SteveX >>>>>>>> ; dev@dpdk.org >>>>>>>> Cc: Zhao1, Wei ; Guo, Jia >>>>>>>> ; Yang, Qiming ; Wu, >>>>>>>> Jingjing ; Xing, Beilei >>>>>>>> >>>>>>>> Subject: RE: [PATCH v4 3/5] net/ice: fix max mtu size packets >>>>>>>> with vlan tag cannot be received by default >>>>>>>> >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Yang, SteveX >>>>>>>>>> Sent: Monday, September 28, 2020 2:56 PM >>>>>>>>>> To: dev@dpdk.org >>>>>>>>>> Cc: Zhao1, Wei ; Guo, Jia >>>>>>>>>> ; Yang, Qiming ; >>>> Zhang, >>>>>>>>>> Qi Z ; Wu, Jingjing >>>>>>>>>> ; Xing, Beilei ; >>>>>>>>>> Ananyev, Konstantin ; Yang, >>>> SteveX >>>>>>>>>> >>>>>>>>>> Subject: [PATCH v4 3/5] net/ice: fix max mtu size packets with >>>>>>>>>> vlan tag cannot be received by default >>>>>>>>>> >>>>>>>>>> testpmd will initialize default max packet length to 1518 which >>>>>>>>>> doesn't include vlan tag size in ether overheader. Once, send >>>>>>>>>> the max mtu length packet with vlan tag, the max packet length >>>>>>>>>> will exceed 1518 that will cause packets dropped directly from >>>>>>>>>> NIC hw >>>>>> side. >>>>>>>>>> >>>>>>>>>> ice can support dual vlan tags that need more 8 bytes for max >>>>>>>>>> packet size, so, configures the correct max packet size in >>>>>>>>>> dev_config >>>>>>> ops. >>>>>>>>>> >>>>>>>>>> Fixes: 50cc9d2a6e9d ("net/ice: fix max frame size") >>>>>>>>>> >>>>>>>>>> Signed-off-by: SteveX Yang >>>>>>>>>> --- >>>>>>>>>> drivers/net/ice/ice_ethdev.c | 11 +++++++++++ >>>>>>>>>> 1 file changed, 11 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/net/ice/ice_ethdev.c >>>>>>>>>> b/drivers/net/ice/ice_ethdev.c index >>>>>>>>>> cfd357b05..6b7098444 100644 >>>>>>>>>> --- a/drivers/net/ice/ice_ethdev.c >>>>>>>>>> +++ b/drivers/net/ice/ice_ethdev.c >>>>>>>>>> @@ -3146,6 +3146,7 @@ ice_dev_configure(struct rte_eth_dev >>>> *dev) >>>>>>>>>> struct ice_adapter *ad = >>>>>>>>>> ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private); >>>>>>>>>> struct ice_pf *pf = >>>>>>>>>> ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private); >>>>>>>>>> +uint32_t frame_size = dev->data->mtu + ICE_ETH_OVERHEAD; >>>>>>>>>> int ret; >>>>>>>>>> >>>>>>>>>> /* Initialize to TRUE. If any of Rx queues doesn't meet the >>>>>>>>>> @@ >>>>>>>>>> -3157,6 >>>>>>>>>> +3158,16 @@ ice_dev_configure(struct rte_eth_dev *dev) >>>>>>>>>> if (dev->data->dev_conf.rxmode.mq_mode & >>>> ETH_MQ_RX_RSS_FLAG) >>>>>>>>>> dev->data->dev_conf.rxmode.offloads |= >>>>>>> DEV_RX_OFFLOAD_RSS_HASH; >>>>>>>>>> >>>>>>>>>> +/** >>>>>>>>>> + * Considering QinQ packet, max frame size should be equal or >>>>>>>>>> + * larger than total size of MTU and Ether overhead. >>>>>>>>>> + */ >>>>>>>>> >>>>>>>>>> +if (frame_size > dev->data->dev_conf.rxmode.max_rx_pkt_len) >> { >>>>>>>>> >>>>>>>>> >>>>>>>>> Why we need this check? >>>>>>>>> Can we just call ice_mtu_set directly >>>>>>>> >>>>>>>> I think that without that check we can silently overwrite >>>>>>>> provided by user dev_conf.rxmode.max_rx_pkt_len value. >>>>>>> >>>>>>> OK, I see >>>>>>> >>>>>>> But still have one question >>>>>>> dev->data->mtu is initialized to 1518 as default , but if >>>>>>> dev->data->application set >>>>>>> dev_conf.rxmode.max_rx_pkt_len = 1000 in dev_configure. >>>>>>> does that mean we will still will set mtu to 1518, is this expected? >>>>>>> >>>>>> >>>>>> max_rx_pkt_len should be larger than mtu at least, so we should >>>>>> raise the max_rx_pkt_len (e.g.:1518) to hold expected mtu value (e.g.: >> 1500). >>>>> >>>>> Ok, this describe the problem more general and better to replace >>>>> exist >>>> code comment and commit log for easy understanding. >>>>> Please send a new version for reword >>>>> >>>> >>>> I didn't really get this set. >>>> >>>> Application explicitly sets 'max_rx_pkt_len' to '1518', and a frame >>>> bigger than this size is dropped. >>> >>> Sure, it is normal case for dropping oversize data. >>> >>>> Isn't this what should be, why we are trying to overwrite user >>>> configuration in PMD to prevent this? >>>> >>> >>> But it is a confliction that application/user sets mtu & max_rx_pkt_len at >> the same time. >>> This fix will make a decision when confliction occurred. >>> MTU value will come from user operation (e.g.: port config mtu 0 1500) >>> directly, so, the max_rx_pkt_len will resize itself to adapt expected MTU >> value if its size is smaller than MTU + Ether overhead. >>> >>>> During eth_dev allocation, mtu set to default '1500', by ethdev layer. >>>> And testpmd sets 'max_rx_pkt_len' by default to '1518'. >>>> I think Qi's concern above is valid, what is user set 'max_rx_pkt_len' to >> '1000' >>>> and mean it? PMD will not honor the user config. >>> >>> I'm not sure when set 'mtu' to '1500' and 'max_rx_pkt_len' to '1000', what's >> the behavior expected? >>> If still keep the 'max_rx_pkt_len' value, that means the larger 'mtu' will be >> invalid. >>> >>>> >>>> Why not simply increase the default 'max_rx_pkt_len' in testpmd? >>>> >>> The default 'max_rx_pkt_len' has been initialized to generical value >>> (1518) and default 'mtu' is '1500' in testpmd, But it isn't suitable to those >> NIC drivers which Ether overhead is larger than 18. (e.g.: ice, i40e) if 'mtu' >> value is preferable. >>> >>>> And I guess even better what we need is to tell to the application >>>> what the frame overhead PMD accepts. >>>> So the application can set proper 'max_rx_pkt_len' value per port for >>>> a given/requested MTU value. >>>> @Ian, cc'ed, was complaining almost same thing years ago, these PMD >>>> overhead macros and 'max_mtu'/'min_mtu' added because of that, >>>> perhaps he has a solution now? >>>> >>> >>>> >>>> And why this same thing can't happen to other PMDs? If this is a >>>> problem for all PMDs, we should solve in other level, not for only some >> PMDs. >>>> >>> No, all PMDs exist the same issue, another proposal: >>> - rte_ethdev provides the unique resize 'max_rx_pkt_len' in >> rte_eth_dev_configure(); >>> - provide the uniform API for fetching the NIC's supported Ether >>> Overhead size; Is it feasible? >>> >> >> overhead can be calculated as "dev_info.max_rx_pktlen - >> dev_info.max_mtu" >> >> What do you think update the testpmd 'init_config()', to update 'port- >>> dev_conf.rxmode.max_rx_pkt_len' as "RTE_ETHER_MTU + overhead"? > > If update the testpmd relative code, this fix will only impact testpmd application, > Need we make the change more common for other applications or DPDK clients? > This is something needs to be done in application level. Testpmd update can be a sample usage for them. > How about update 'max_rx_pkt_len' within 'rte_eth_dev_configure()' of rte_ethdev? > What is your proposal to do in the ethdev layer? >> >>>>> >>>>>> Generally, the mtu value can be adjustable from user (e.g.: ip link >>>>>> set ens801f0 mtu 1400), hence, we just adjust the max_rx_pkt_len to >>>>>> satisfy mtu requirement. >>>>>> >>>>>>> Should we just call ice_mtu_set(dev, >> dev_conf.rxmode.max_rx_pkt_len) >>>>>>> here? >>>>>> ice_mtu_set(dev, mtu) will append ether overhead to >>>>>> frame_size/max_rx_pkt_len, so we need pass the mtu value as the >> 2nd >>>>>> parameter, or not the max_rx_pkt_len. >>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>>> And please remove above comment, since ether overhead is >> already >>>>>>>> considered in ice_mtu_set. >>>>>> Ether overhead is already considered in ice_mtu_set, but it also >>>>>> should be considered as the adjustment condition that if ice_mtu_set >>>> need be invoked. >>>>>> So, it perhaps should remain this comment before this if() condition. >>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> +ret = ice_mtu_set(dev, dev->data->mtu); if (ret != 0) return >>>>>>>>>> +ret; } >>>>>>>>>> + >>>>>>>>>> ret = ice_init_rss(pf); >>>>>>>>>> if (ret) { >>>>>>>>>> PMD_DRV_LOG(ERR, "Failed to enable rss for PF"); >>>>>>>>>> -- >>>>>>>>>> 2.17.1 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>> >