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 69354A0C4E; Thu, 22 Jul 2021 12:15:09 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EA64F4014D; Thu, 22 Jul 2021 12:15:08 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 46D0940040 for ; Thu, 22 Jul 2021 12:15:06 +0200 (CEST) Received: from [192.168.100.116] (unknown [37.139.99.76]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id B81567F53F; Thu, 22 Jul 2021 13:15:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru B81567F53F DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1626948905; bh=pxorIWWrwXOZeWy+PkaOm8z16D32JxroJRxH8kQ/lFE=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=xK/Sb323QueaG8gwxfPiVG0JmRD1oKTvqAhsmHUnc+La0u9HtMe9zNs5ZFAUZCKAf ZCKPtrG8DDKa4+Tesc8GK/PDQ0ncEUT26HbFu7ksyQOqwzAo5tFkNPRy9vaMx1HXNn UWKBlWUpLvSrasDjqdLw4F+os63mS4dCoHWut7fU= To: Ferruh Yigit , Huisong Li Cc: "dev@dpdk.org" References: <20210709172923.3369846-1-ferruh.yigit@intel.com> <1a891390-1122-6dcf-03e8-1f0b147b30ec@huawei.com> <4e1ae26c-197b-ea45-0860-66c195d1f820@intel.com> <44ea055b-4f43-5cd5-9911-662b6df51623@huawei.com> <78252f7e-3170-344c-fba9-85fcacc36026@intel.com> From: Andrew Rybchenko Message-ID: <00fb2449-cdf1-7af2-1ad9-f16d80def53f@oktetlabs.ru> Date: Thu, 22 Jul 2021 13:15:04 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <78252f7e-3170-344c-fba9-85fcacc36026@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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" On 7/22/21 1:12 PM, Ferruh Yigit wrote: > On 7/22/2021 8:21 AM, Huisong Li wrote: >> >> 在 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? >> > > I don't think we care about type of transmission in this level, I assume we > define min MTU mainly for the HW limitation and configuration. That is why it > makes sense to me to use Ethernet frame lenght limitation (not IPv4 one). +1 >> 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。 >> > > I am planning to move the MTU checks into common function and use it for both > 'rte_eth_dev_configure()' & 'rte_eth_dev_set_mtu()' in a seperate patch. Please > check v2. >