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 CBA86431F0; Tue, 24 Oct 2023 14:21:39 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5ABA6402E3; Tue, 24 Oct 2023 14:21:39 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 58B7A40285 for ; Tue, 24 Oct 2023 14:21:36 +0200 (CEST) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.55]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4SFB1F6pJGz15Ncf; Tue, 24 Oct 2023 20:18:41 +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; Tue, 24 Oct 2023 20:21:32 +0800 Message-ID: <29c828de-baf0-32fc-5fb9-1e440e6b7ba7@huawei.com> Date: Tue, 24 Oct 2023 20:21:17 +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 v1 1/3] ethdev: introduce maximum Rx buffer size To: Ferruh Yigit , CC: , , References: <20230808040234.12947-1-lihuisong@huawei.com> <20230815111034.22887-1-lihuisong@huawei.com> <20230815111034.22887-2-lihuisong@huawei.com> From: "lihuisong (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) 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 Hi Ferruh, Very very sorry for my delay reply. I missed your email.🙁 在 2023/9/28 23:56, Ferruh Yigit 写道: > On 8/15/2023 12:10 PM, Huisong Li wrote: >> The Rx buffer size stands for the size hardware supported to receive >> packets in one mbuf. The "min_rx_bufsize" is the minimum buffer hardware >> supported in Rx. > Hi Huisong, > > I guess I understand the intention, but above description is not accurate, Yes, the mbuf data room size in mempool can be bigger than the buffer in HW descriptor. But it seems that Rx buffer size is different from the mbuf data room size. Their relationship is as below: mbuf_data_room_size = rx_buffer_size + RTE_PKTMBUF_HEADROOM. rx_buffer_size is equal to the Rx buffer in HW descriptor. That's why I said that. > > ethdev Rx buffer sizes are not per mbuf. Device internal Rx buffer can > be bigger and received packet can be represented by multiple > descriptors. Each descriptor maps to an mbuf. Yeah > Like device can support 8K packets, this is informed with > 'max_rx_pktlen', and if you provide 1K mbufs, device can receives a 8K > buffer and chains 8 mbufs to represent packet. Yes, you are right. The number of segments that a packet, like 8K length, needs to be received depends on the size of the buffer size corresponding to each HW descriptor. And this buffer size set to hw is calculated based on the mbuf_data_room_size in mempool from rx buffer size. > > >> Actually, some engines also have the maximum buffer >> specification, like, hns3. For these engines, the available data size >> of one mbuf in Rx also depends on the maximum buffer hardware supported. >> So introduce maximum Rx buffer size in struct rte_eth_dev_info to report >> user to avoid memory waste. And driver should accept it and just pass >> maximum buffer hardware supported to hardware if application specifies >> the Rx buffer size is greater than the maximum buffer. >> > If I understand correctly, you want to notify user about per descriptor > max buffer size, and I can see that is 4K for hns3. > Which means even if device can receive larger packets, each > descriptor/mbuf can't have more than 4K. > So user allocating pktmbuf pool with mbuf bigger than 4K is wasting > memory, and you are trying to prevent it, is this understanding correct? Yes, your are correct. > > No objection device sharing this information, but we should make it ok, thanks. > clear what it is to not confuse users, please check below comments. Ok, let's make it more clear. > > > >> Signed-off-by: Huisong Li >> --- >> lib/ethdev/rte_ethdev.c | 7 +++++++ >> lib/ethdev/rte_ethdev.h | 6 ++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index 0840d2b594..9985bd3049 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -2068,6 +2068,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; >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> @@ -2105,6 +2106,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(WARNING, >> + "Ethdev port_id=%u Rx buffer size (%u) is greater than the maximum buffer size (%u) driver supported.\n", >> + port_id, vld_bufsize, dev_info.max_rx_bufsize); >> > I am not sure about this check in the ethdev level, application already > getting this value, intention is optimization but a warning message can > be confusing to the user, > > even if we keep the log, at least log level shouldn't be warning, > nothing wrong to warn, maybe info or debug level, and I think message > should be updated, this is not about Rx buffer size but max size per mbuf. What about info log level? What do you think to use "data room size in mbuf" to replace rx bufer size here? > > > What do you think to move this log to testpmd, it can be also a sample > how to use this new field in application level? IMO, it is more better in ethdev layer because it is a common issue for all PMDs and applications. If user know this point, they also actually don't check this size. right? > > >> } else if (rx_conf != NULL && rx_conf->rx_nseg > 0) { >> const struct rte_eth_rxseg_split *rx_seg; >> uint16_t n_seg; >> @@ -3689,6 +3695,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 04a2564f22..9fdf2c75ee 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -1724,6 +1724,12 @@ struct rte_eth_dev_info { >> uint16_t max_mtu; /**< Maximum MTU allowed */ >> const uint32_t *dev_flags; /**< Device flags */ >> uint32_t min_rx_bufsize; /**< Minimum size of Rx buffer. */ >> + /** >> + * Maximum size of Rx buffer. Driver should accept it and just pass >> + * this value to HW if application specifies the Rx buffer size is >> + * greater than this value. >> + */ > Above comment can be simplified, as something like: > " > Maximum buffer size supported per Rx descriptor by HW. > The value is not enforced, information only to application to optimize > mbuf size. > " Above comment is just intented to express the behavior in driver according to Andrew's suggestion. Yes no need to explain the behavior.  All right, I will fix it. >> + uint32_t max_rx_bufsize; >> > What about renaming variable, to something like: > "max_desc_bufsize" /**< Maximum buffer size per Rx descriptor. */ It's really more accurate if we do like this. But how should we address the "min_rx_bufsize" field? They are similar. The "min_rx_bufsize" field already stood for the minimum Rx buffer size in Rx hw descriptor. > > >> uint32_t max_rx_pktlen; /**< Maximum configurable length of Rx pkt. */ >> /** Maximum configurable size of LRO aggregated packet. */ >> uint32_t max_lro_pkt_size; > .