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 D2765A0C4E; Thu, 22 Jul 2021 09:22:02 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4F5EA4014D; Thu, 22 Jul 2021 09:22:02 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 5760340040 for ; Thu, 22 Jul 2021 09:22:00 +0200 (CEST) Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.54]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4GVkMG106HzZrVY for ; Thu, 22 Jul 2021 15:18:34 +0800 (CST) Received: from dggema767-chm.china.huawei.com (10.1.198.209) by dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2176.2; Thu, 22 Jul 2021 15:21:56 +0800 Received: from [10.66.74.184] (10.66.74.184) by dggema767-chm.china.huawei.com (10.1.198.209) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Thu, 22 Jul 2021 15:21:56 +0800 To: Ferruh Yigit References: <20210709172923.3369846-1-ferruh.yigit@intel.com> <1a891390-1122-6dcf-03e8-1f0b147b30ec@huawei.com> <4e1ae26c-197b-ea45-0860-66c195d1f820@intel.com> CC: "dev@dpdk.org" From: Huisong Li Message-ID: <44ea055b-4f43-5cd5-9911-662b6df51623@huawei.com> Date: Thu, 22 Jul 2021 15:21:55 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <4e1ae26c-197b-ea45-0860-66c195d1f820@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.66.74.184] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggema767-chm.china.huawei.com (10.1.198.209) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH 1/4] ethdev: fix 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" 在 2021/7/21 23:29, Ferruh Yigit 写道: > On 7/19/2021 4:35 AM, Huisong Li wrote: >> Hi, Ferruh >> > Hi Huisong, > > Thanks for the review. > >> 在 2021/7/10 1:29, Ferruh Yigit 写道: >>> There is a confusion on setting max Rx packet length, this patch aims to >>> clarify it. >>> >>> 'rte_eth_dev_configure()' API accepts max Rx packet size via >>> 'uint32_t max_rx_pkt_len' filed of the config struct 'struct >>> rte_eth_conf'. >>> >>> Also 'rte_eth_dev_set_mtu()' API can be used to set the MTU, and result >>> stored into '(struct rte_eth_dev)->data->mtu'. >>> >>> These two APIs are related but they work in a disconnected way, they >>> store the set values in different variables which makes hard to figure >>> out which one to use, also two different related method is confusing for >>> the users. >>> >>> Other issues causing confusion is: >>> * maximum transmission unit (MTU) is payload of the Ethernet frame. And >>>    'max_rx_pkt_len' is the size of the Ethernet frame. Difference is >>>    Ethernet frame overhead, but this may be different from device to >>>    device based on what device supports, like VLAN and QinQ. >>> * 'max_rx_pkt_len' is only valid when application requested jumbo frame, >>>    which adds additional confusion and some APIs and PMDs already >>>    discards this documented behavior. >>> * For the jumbo frame enabled case, 'max_rx_pkt_len' is an mandatory >>>    field, this adds configuration complexity for application. >>> >>> As solution, both APIs gets MTU as parameter, and both saves the result >>> in same variable '(struct rte_eth_dev)->data->mtu'. For this >>> 'max_rx_pkt_len' updated as 'mtu', and it is always valid independent >>> from jumbo frame. >>> >>> For 'rte_eth_dev_configure()', 'dev->data->dev_conf.rxmode.mtu' is user >>> request and it should be used only within configure function and result >>> should be stored to '(struct rte_eth_dev)->data->mtu'. After that point >>> both application and PMD uses MTU from this variable. >>> >>> When application doesn't provide an MTU during 'rte_eth_dev_configure()' >>> default 'RTE_ETHER_MTU' value is used. >>> >>> As additional clarification, MTU is used to configure the device for >>> physical Rx/Tx limitation. Other related issue is size of the buffer to >>> store Rx packets, many PMDs use mbuf data buffer size as Rx buffer size. >>> And compares MTU against Rx buffer size to decide enabling scattered Rx >>> or not, if PMD supports it. If scattered Rx is not supported by device, >>> MTU bigger than Rx buffer size should fail. >>> >>> Signed-off-by: Ferruh Yigit > <...> > >>> diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c >>> index e51512560e15..8bccdeddb2f7 100644 >>> --- a/drivers/net/hns3/hns3_ethdev.c >>> +++ b/drivers/net/hns3/hns3_ethdev.c >>> @@ -2379,20 +2379,11 @@ hns3_refresh_mtu(struct rte_eth_dev *dev, struct >>> rte_eth_conf *conf) >>>   { >>>       struct hns3_adapter *hns = dev->data->dev_private; >>>       struct hns3_hw *hw = &hns->hw; >>> -    uint32_t max_rx_pkt_len; >>> -    uint16_t mtu; >>> -    int ret; >>> - >>> -    if (!(conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME)) >>> -        return 0; >>> +    uint32_t max_rx_pktlen; >>>   -    /* >>> -     * If jumbo frames are enabled, MTU needs to be refreshed >>> -     * according to the maximum RX packet length. >>> -     */ >>> -    max_rx_pkt_len = conf->rxmode.max_rx_pkt_len; >>> -    if (max_rx_pkt_len > HNS3_MAX_FRAME_LEN || >>> -        max_rx_pkt_len <= HNS3_DEFAULT_FRAME_LEN) { >>> +    max_rx_pktlen = conf->rxmode.mtu + HNS3_ETH_OVERHEAD; >>> +    if (max_rx_pktlen > HNS3_MAX_FRAME_LEN || >>> +        max_rx_pktlen <= HNS3_DEFAULT_FRAME_LEN) { >>>           hns3_err(hw, "maximum Rx packet length must be greater than %u " >>>                "and no more than %u when jumbo frame enabled.", >>>                (uint16_t)HNS3_DEFAULT_FRAME_LEN, >> The preceding check for the maximum frame length was based on the scenario where >> jumbo frames are enabled. >> >> Since there is no offload of jumbo frames in this patchset, the maximum frame >> length does not need to be checked and only ensure conf->rxmode.mtu is valid. >> >> These should be guaranteed by dev_configure() in the framework . >> > Got it, agree that 'HNS3_DEFAULT_FRAME_LEN' check is now wrong, and as you said > these checks are becoming redundant, so I will remove them. > > In that case 'hns3_refresh_mtu()' becomes just wrapper to 'hns3_dev_mtu_set()', > I will remove function too. > > <...> ok > >>> diff --git a/drivers/net/hns3/hns3_ethdev_vf.c >>> b/drivers/net/hns3/hns3_ethdev_vf.c >>> index e582503f529b..ca839fa55fa0 100644 >>> --- a/drivers/net/hns3/hns3_ethdev_vf.c >>> +++ b/drivers/net/hns3/hns3_ethdev_vf.c >>> @@ -784,8 +784,7 @@ hns3vf_dev_configure(struct rte_eth_dev *dev) >>>       uint16_t nb_rx_q = dev->data->nb_rx_queues; >>>       uint16_t nb_tx_q = dev->data->nb_tx_queues; >>>       struct rte_eth_rss_conf rss_conf; >>> -    uint32_t max_rx_pkt_len; >>> -    uint16_t mtu; >>> +    uint32_t max_rx_pktlen; >>>       bool gro_en; >>>       int ret; >>>   @@ -825,29 +824,21 @@ hns3vf_dev_configure(struct rte_eth_dev *dev) >>>               goto cfg_err; >>>       } >>>   -    /* >>> -     * If jumbo frames are enabled, MTU needs to be refreshed >>> -     * according to the maximum RX packet length. >>> -     */ >>> -    if (conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) { >>> -        max_rx_pkt_len = conf->rxmode.max_rx_pkt_len; >>> -        if (max_rx_pkt_len > HNS3_MAX_FRAME_LEN || >>> -            max_rx_pkt_len <= HNS3_DEFAULT_FRAME_LEN) { >>> -            hns3_err(hw, "maximum Rx packet length must be greater " >>> -                 "than %u and less than %u when jumbo frame enabled.", >>> -                 (uint16_t)HNS3_DEFAULT_FRAME_LEN, >>> -                 (uint16_t)HNS3_MAX_FRAME_LEN); >>> -            ret = -EINVAL; >>> -            goto cfg_err; >>> -        } >>> - >>> -        mtu = (uint16_t)HNS3_PKTLEN_TO_MTU(max_rx_pkt_len); >>> -        ret = hns3vf_dev_mtu_set(dev, mtu); >>> -        if (ret) >>> -            goto cfg_err; >>> -        dev->data->mtu = mtu; >>> +    max_rx_pktlen = conf->rxmode.mtu + HNS3_ETH_OVERHEAD; >>> +    if (max_rx_pktlen > HNS3_MAX_FRAME_LEN || >>> +        max_rx_pktlen <= HNS3_DEFAULT_FRAME_LEN) { >>> +        hns3_err(hw, "maximum Rx packet length must be greater " >>> +             "than %u and less than %u when jumbo frame enabled.", >>> +             (uint16_t)HNS3_DEFAULT_FRAME_LEN, >>> +             (uint16_t)HNS3_MAX_FRAME_LEN); >>> +        ret = -EINVAL; >>> +        goto cfg_err; >>>       } >> Please remove this check now, thanks! > ack > > <...> > >>> diff --git a/examples/ip_reassembly/main.c b/examples/ip_reassembly/main.c >>> index ce8882a45883..f868e5d906c7 100644 >>> --- a/examples/ip_reassembly/main.c >>> +++ b/examples/ip_reassembly/main.c >>> @@ -162,7 +162,7 @@ static struct lcore_queue_conf >>> lcore_queue_conf[RTE_MAX_LCORE]; >>>   static struct rte_eth_conf port_conf = { >>>       .rxmode = { >>>           .mq_mode        = ETH_MQ_RX_RSS, >>> -        .max_rx_pkt_len = JUMBO_FRAME_MAX_SIZE, >>> +        .mtu = JUMBO_FRAME_MAX_SIZE, >> It feel likes that the replacement of max_rx_pkt_len with MTU is inappropriate. >> >> Because "max_rx_pkt_len " is the sum of  "mtu" and "overhead_len". > You are right, it is not same thing. I will update it to remove overhead. > > <...> > >>> @@ -1448,49 +1459,45 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t >>> nb_rx_q, uint16_t nb_tx_q, >>>       } >>>         /* >>> -     * If jumbo frames are enabled, check that the maximum RX packet >>> -     * length is supported by the configured device. >>> +     * Check that the maximum RX packet length is supported by the >>> +     * configured device. >>>        */ >>> -    if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) { >>> -        if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) { >>> -            RTE_ETHDEV_LOG(ERR, >>> -                "Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n", >>> -                port_id, dev_conf->rxmode.max_rx_pkt_len, >>> -                dev_info.max_rx_pktlen); >>> -            ret = -EINVAL; >>> -            goto rollback; >>> -        } else if (dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN) { >>> -            RTE_ETHDEV_LOG(ERR, >>> -                "Ethdev port_id=%u max_rx_pkt_len %u < min valid value %u\n", >>> -                port_id, dev_conf->rxmode.max_rx_pkt_len, >>> -                (unsigned int)RTE_ETHER_MIN_LEN); >>> -            ret = -EINVAL; >>> -            goto rollback; >>> -        } >>> +    if (dev_conf->rxmode.mtu == 0) >>> +        dev->data->dev_conf.rxmode.mtu = RTE_ETHER_MTU; >> Here, it will cause a case that the user configuration is inconsistent with the >> configuration saved in the framework . > What is the framework you mentioned? > > Previously 'max_rx_pkt_len' was mandatory when jumbo frame is configured even > user doesn't really case about it and it was causing additional complexity in > the configuration. > This check is required to use defaults when application doesn't need a specific > value, I believe this is a good usability improvement. > Application who cares about a specific value can set it explicitly and it will > be in sync with application. > >> Is it more reasonable to provide a prompt message? > Not sure about it. We are not changing a user configured value, but using > default value when application doesn't set it, and that kind of log will be > printed by most of the applications, this may cause noise. This is a good reason. > >>> +    max_rx_pktlen = dev->data->dev_conf.rxmode.mtu + overhead_len; >>> +    if (max_rx_pktlen > dev_info.max_rx_pktlen) { >>> +        RTE_ETHDEV_LOG(ERR, >>> +            "Ethdev port_id=%u max_rx_pktlen %u > max valid value %u\n", >>> +            port_id, max_rx_pktlen, dev_info.max_rx_pktlen); >>> +        ret = -EINVAL; >>> +        goto rollback; >>> +    } else if (max_rx_pktlen < RTE_ETHER_MIN_LEN) { >>> +        RTE_ETHDEV_LOG(ERR, >>> +            "Ethdev port_id=%u max_rx_pktlen %u < min valid value %u\n", >>> +            port_id, max_rx_pktlen, RTE_ETHER_MIN_LEN); >>> +        ret = -EINVAL; >>> +        goto rollback; >>> +    } >> Above "max_rx_pktlen < RTE_ETHER_MIN_LEN " case will be  inconsistent with >> dev_set_mtu() API. >> >> The reasons are as follows: >> >> The value of RTE_ETHER_MIN_LEN is 64. If "overhead_len" is 26 caculated by >> eth_dev_get_overhead_len(), it means >> >> that dev->data->dev_conf.rxmode.mtu equal to 38 is reasonable. >> >> But, in  dev_set_mtu() API, the check for mtu is: >> >> @@ -3643,12 +3644,27 @@ rte_eth_dev_set_mtu(uint16_t port_id, uint16_t mtu) >> >>          if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu) >>              return -EINVAL; >> >> It should be noted that dev_info.min_mtu is RTE_ETHER_MIN_MTU (68). >> > Agree on the inconsistency. > > RTE_ETHER_MIN_MTU is 68, that is min MTU for IPv4 > RTE_ETHER_MIN_LEN is 64, and min MTU for Ethernet frame > > Although we are talking about MTU, we are mainly concerned about Ethernet frame > payload, not IPv4. > > I suggest only using RTE_ETHER_MIN_LEN. > Since this inconsistency was already there before this patch, I will update it > in seperate patch instead of fixing in this one. > . Got it. Since the MTU value depends on the type of transmission link. Why does we define a minimum MTU? True, we don't have to break the current restrictions in this patch. But it is an indirect check on the MTU. Now that "mtu" in Rxmode is an entry for configuring MTU for driver, I prefer to keep the same MTU check in ethdev layer. If there's a better way to handle it, perhaps it would be more appropriate to do it in this patchset. I'd like to know how you're going to adjust。