From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4D94BA0A02; Thu, 14 Jan 2021 21:44:43 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EC0BE140E0E; Thu, 14 Jan 2021 21:44:42 +0100 (CET) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id CC554140E0D for ; Thu, 14 Jan 2021 21:44:41 +0100 (CET) IronPort-SDR: 7CvyXo283v1wHmP+0uJfsLwKTdxzG2tQZJ9sCf1PwlePyCyzBhPN683tDnuNP+sbEg8MmrwU7S NijWELBHaFrA== X-IronPort-AV: E=McAfee;i="6000,8403,9864"; a="157621422" X-IronPort-AV: E=Sophos;i="5.79,347,1602572400"; d="scan'208";a="157621422" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jan 2021 12:44:38 -0800 IronPort-SDR: t96bP71+oCeUcllR7ZWweMA4VSeBgF195rKQiT34RFik3R6PhYMIYALxavwOXBK143pja2NlPC cZ/9MoADSc8g== X-IronPort-AV: E=Sophos;i="5.79,347,1602572400"; d="scan'208";a="382401902" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.31.191]) ([10.252.31.191]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jan 2021 12:44:36 -0800 To: Andrew Boyer Cc: Steve Yang , dev@dpdk.org, thomas@monjalon.net, andrew.rybchenko@oktetlabs.ru, oulijun@huawei.com, Konstantin Ananyev References: <20201217092312.27033-1-stevex.yang@intel.com> <20210114094537.13661-1-stevex.yang@intel.com> <53a37e85-fdba-ae58-1028-1facc33884dc@intel.com> <96dd6d7a-7af3-2c59-b25f-e9f0cad974d0@intel.com> <97F6D3C8-E222-407C-A523-1E5D2552442A@pensando.io> From: Ferruh Yigit Message-ID: <35b13b80-018d-fef3-9f43-14aff0a4b93d@intel.com> Date: Thu, 14 Jan 2021 20:44:32 +0000 MIME-Version: 1.0 In-Reply-To: <97F6D3C8-E222-407C-A523-1E5D2552442A@pensando.io> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 01/22] ethdev: fix MTU size exceeds max rx packet length X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 1/14/2021 5:29 PM, Andrew Boyer wrote: > > >> On Jan 14, 2021, at 12:13 PM, Ferruh Yigit > > wrote: >> >> On 1/14/2021 4:36 PM, Ferruh Yigit wrote: >>> On 1/14/2021 9:45 AM, Steve Yang wrote: >>>> Ethdev is using default Ethernet overhead to decide if provided >>>> 'max_rx_pkt_len' value is bigger than max (non jumbo) MTU value, >>>> and limits it to MAX if it is. >>>> >>>> Since the application/driver used Ethernet overhead is different than >>>> the ethdev one, check result is wrong. >>>> >>>> If the driver is using Ethernet overhead bigger than the default one, >>>> the provided 'max_rx_pkt_len' is trimmed down, and in the driver when >>>> correct Ethernet overhead is used to convert back, the resulting MTU is >>>> less than the intended one, causing some packets to be dropped. >>>> >>>> Like, >>>> app     -> max_rx_pkt_len = 1500/*mtu*/ + 22/*overhead*/ = 1522 >>>> ethdev  -> 1522 > 1518/*MAX*/; max_rx_pkt_len = 1518 >>>> driver  -> MTU = 1518 - 22 = 1496 >>>> Packets with size 1497-1500 are dropped although intention is to be able >>>> to send/receive them. >>>> >>>> The fix is to make ethdev use the correct Ethernet overhead for port, >>>> instead of default one. >>>> >>>> Fixes: 59d0ecdbf0e1 ("ethdev: MTU accessors") >>>> >>>> Signed-off-by: Steve Yang > >>> <...> >>>> @@ -1410,11 +1422,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t >>>> nb_rx_q, uint16_t nb_tx_q, >>>> goto rollback; >>>> } >>>> } else { >>>> -        if (dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN || >>>> -            dev_conf->rxmode.max_rx_pkt_len > RTE_ETHER_MAX_LEN) >>>> +        uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len; >>>> +        if (pktlen < RTE_ETHER_MIN_MTU + overhead_len || >>>> +            pktlen > RTE_ETHER_MTU + overhead_len) >>>> /* Use default value */ >>>> dev->data->dev_conf.rxmode.max_rx_pkt_len = >>>> -                            RTE_ETHER_MAX_LEN; >>>> +                        RTE_ETHER_MTU + overhead_len; >>> What do you think removing the above check, the else block, completely? >>> Since the 'max_rx_pkt_len' should not be used when jumbo frame is not set. >> >> As I tested removing this check is causing problem because some PMDs are using >> the 'max_rx_pkt_len' even jumbo frame is not set. >> >> Perhaps better to keep it, and make a separate patch later to remove this >> check, after PMDs fixed. > > Hello Ferruh - > Working on fixing our PMD here. Do you want PMDs to update the JUMBO_FRAME flag > based on the mtu value in dev_set_mtu(), or do you want the application to be > solely responsible for it? > Hi Andrew, Technically JUMBO_FRAME flag is an user config and application should set it. It is application's responsibility to check the capability and set the flag when necessary. But, after above said, many PMDs set it based on provided MTU value, if the explicitly requested MTU is bigger than the RTE_ETHER_MTU, this means user implied the JUMBO_FRAME support, for this case PMDs set the flag implicitly instead of failing. In another thread Andrew R. & Konstantin suggested to remove the JUMBO_FRAME flag, since it is redundant and causing this kind of confusion, instead driver can decide based on requested MTU value, and driver reported 'max_mtu' value can be used by application to detect the capability. We will probably do this change, but it can be done only in the ABI break release, v21.11. For now, PMD can set the flag itself if requested MTU > RTE_ETHER_MTU and driver supports jumbo frames. > Thanks, > Andrew > >>>> +    } >>>> + >>>> +    /* Scale the MTU size to adapt max_rx_pkt_len */ >>>> +    if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) { >>>> +        dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len - >>>> +                overhead_len; >>>> } >>> Above if block has exact same check, why not move it above block? >> >> Can you still send a new version for above change please? >