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 A8F8143275; Fri, 3 Nov 2023 07:28:01 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F373542DCA; Fri, 3 Nov 2023 07:27:45 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 2D53442DCA for ; Fri, 3 Nov 2023 07:27:44 +0100 (CET) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.57]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4SM9h35TqPz1P7tn; Fri, 3 Nov 2023 14:24:35 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm000004.china.huawei.com (7.193.23.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Fri, 3 Nov 2023 14:27:38 +0800 Message-ID: Date: Fri, 3 Nov 2023 14:27:38 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v4 1/3] ethdev: introduce maximum Rx buffer size To: Ferruh Yigit , , Thomas Monjalon , Andrew Rybchenko , =?UTF-8?Q?Morten_Br=c3=b8rup?= , Stephen Hemminger CC: References: <20230808040234.12947-1-lihuisong@huawei.com> <20231102121608.10170-1-lihuisong@huawei.com> <20231102121608.10170-2-lihuisong@huawei.com> <85d5dacf-4cb2-3eb9-87e0-29da61726b80@huawei.com> <769abd53-d93b-440e-b542-aee3e244f147@amd.com> From: "lihuisong (C)" In-Reply-To: <769abd53-d93b-440e-b542-aee3e244f147@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemm000004.china.huawei.com (7.193.23.18) X-CFilter-Loop: Reflected 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 在 2023/11/3 11:30, Ferruh Yigit 写道: > On 11/3/2023 2:21 AM, lihuisong (C) wrote: >> 在 2023/11/3 0:35, Ferruh Yigit 写道: >>> On 11/2/2023 12:16 PM, Huisong Li wrote: >>>> The "min_rx_bufsize" in struct rte_eth_dev_info stands for the minimum >>>> Rx buffer size supported by hardware. Actually, some engines also have >>>> the maximum Rx buffer specification, like, hns3, i40e and so on. If mbuf >>>> data room size in mempool is greater then the maximum Rx buffer size >>>> per descriptor supported by HW, the data size application used in each >>>> mbuf is just as much as the maximum Rx buffer size instead of the whole >>>> data room size. >>>> >>>> So introduce maximum Rx buffer size which is not enforced just to >>>> report user to avoid memory waste. In addition, fix the comment for >>>> the "min_rx_bufsize" to make it be more specific. >>>> >>>> Signed-off-by: Huisong Li >>>> Acked-by: Chengwen Feng >>>> Acked-by: Morten Brørup >>>> --- >>>>   doc/guides/rel_notes/release_23_11.rst |  7 +++++++ >>>>   lib/ethdev/rte_ethdev.c                |  7 +++++++ >>>>   lib/ethdev/rte_ethdev.h                | 10 +++++++++- >>>>   3 files changed, 23 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/doc/guides/rel_notes/release_23_11.rst >>>> b/doc/guides/rel_notes/release_23_11.rst >>>> index 95db98d098..d4f7d5b266 100644 >>>> --- a/doc/guides/rel_notes/release_23_11.rst >>>> +++ b/doc/guides/rel_notes/release_23_11.rst >>>> @@ -122,6 +122,13 @@ New Features >>>>     a group's miss actions, which are the actions to be performed on >>>> packets >>>>     that didn't match any of the flow rules in the group. >>>>   +* **Added maximum Rx buffer size to report.** >>>> + >>>> +  Introduced the ``max_rx_bufsize`` field representing the maximum Rx >>>> +  buffer size per descriptor supported by HW in structure >>>> ``rte_eth_dev_info`` >>>> +  to report user and to avoid wasting space of mempool. >>>> +  Its value is UINT32_MAX if driver doesn't report it. >>>> + >>>>   * **Updated Amazon ena (Elastic Network Adapter) net driver.** >>>>       * Upgraded ENA HAL to latest version. >>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >>>> index af23ac0ad0..03539bb6c3 100644 >>>> --- a/lib/ethdev/rte_ethdev.c >>>> +++ b/lib/ethdev/rte_ethdev.c >>>> @@ -2112,6 +2112,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, >>>> uint16_t rx_queue_id, >>>>       struct rte_eth_dev *dev; >>>>       struct rte_eth_dev_info dev_info; >>>>       struct rte_eth_rxconf local_conf; >>>> +    uint32_t vld_bufsize; >>>> >>> I guess 'vld' is valid, but that is not accurate. Can we rename it >>> something like 'buf_data_size'? >> Ack >>>>       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>>>       dev = &rte_eth_devices[port_id]; >>>> @@ -2158,6 +2159,11 @@ rte_eth_rx_queue_setup(uint16_t port_id, >>>> uint16_t rx_queue_id, >>>>               return ret; >>>>             mbp_buf_size = rte_pktmbuf_data_room_size(mp); >>>> +        vld_bufsize = mbp_buf_size - RTE_PKTMBUF_HEADROOM; >>>> +        if (vld_bufsize > dev_info.max_rx_bufsize) >>>> +            RTE_ETHDEV_LOG(INFO, >>>> +                "For port_id=%u the max data size per mbuf in Rx is >>>> %u instead of the whole data room(%u).\n", >>>> +                port_id, dev_info.max_rx_bufsize, vld_bufsize); >>>> >>> This log gives some information, but it is not clear what user should >>> do. My concern is this can confuse end users (not developers but users >>> of dpdk application). >>> >>> Since audience of this message is DPDK application developer, most of >>> the times end user don't have anything to do with this log, what do you >>> think to change log level to 'debug'? >> Ack >>> Also can we add some action to it, something like: >>> "mbuf data buffer size is bigger than what device can utilize, consider >>> reducing mbuf size." >> What do you think of this log, Ferruh? >> "The mbuf data buffer size (%u) is bigger than the max size (%u) device >> with port_id=%u can utilize, consider reducing mbuf size.\n", >> > Can move port_id to the beginning, and can make wording passive, what about: > > "For port_id=%u, the mbuf data buffer size (%u) is bigger than max > buffer size (%u) device can utilize, mbuf size can be reduced.\n" Ok, thanks for you advice. > >>> >>> >>>>       } else if (rx_conf != NULL && rx_conf->rx_nseg > 0) { >>>>           const struct rte_eth_rxseg_split *rx_seg; >>>>           uint16_t n_seg; >>>> @@ -3757,6 +3763,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct >>>> rte_eth_dev_info *dev_info) >>>>       dev_info->min_mtu = RTE_ETHER_MIN_LEN - RTE_ETHER_HDR_LEN - >>>>           RTE_ETHER_CRC_LEN; >>>>       dev_info->max_mtu = UINT16_MAX; >>>> +    dev_info->max_rx_bufsize = UINT32_MAX; >>>>         if (*dev->dev_ops->dev_infos_get == NULL) >>>>           return -ENOTSUP; >>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >>>> index a53dd5a1ef..7133b73d26 100644 >>>> --- a/lib/ethdev/rte_ethdev.h >>>> +++ b/lib/ethdev/rte_ethdev.h >>>> @@ -1723,7 +1723,15 @@ struct rte_eth_dev_info { >>>>       uint16_t min_mtu;    /**< Minimum MTU allowed */ >>>>       uint16_t max_mtu;    /**< Maximum MTU allowed */ >>>>       const uint32_t *dev_flags; /**< Device flags */ >>>> -    uint32_t min_rx_bufsize; /**< Minimum size of Rx buffer. */ >>>> +    /** Minimum Rx buffer size per descriptor supported by HW. */ >>>> +    uint32_t min_rx_bufsize; >>>> +    /** >>>> +     * Maximum Rx buffer size per descriptor supported by HW. >>>> +     * The value is not enforced, information only to application to >>>> +     * optimize mbuf size. Its value is UINT32_MAX when not specified >>>> +     * by the driver. >>>> +     */ >>>> +    uint32_t max_rx_bufsize; >>>>       uint32_t max_rx_pktlen; /**< Maximum configurable length of Rx >>>> pkt. */ >>>>       /** Maximum configurable size of LRO aggregated packet. */ >>>>       uint32_t max_lro_pkt_size; >>> . > .