From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Huisong Li <lihuisong@huawei.com>, dev@dpdk.org
Cc: thomas@monjalon.net, andrew.rybchenko@oktetlabs.ru,
liuyonglong@huawei.com
Subject: Re: [PATCH v1 1/3] ethdev: introduce maximum Rx buffer size
Date: Thu, 28 Sep 2023 16:56:46 +0100 [thread overview]
Message-ID: <f596c5bf-b73a-4e80-82f9-bfb33e1e5dbf@amd.com> (raw)
In-Reply-To: <20230815111034.22887-2-lihuisong@huawei.com>
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,
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.
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.
> 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?
No objection device sharing this information, but we should make it
clear what it is to not confuse users, please check below comments.
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> 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 do you think to move this log to testpmd, it can be also a sample
how to use this new field in application level?
> } 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.
"
> + uint32_t max_rx_bufsize;
>
What about renaming variable, to something like:
"max_desc_bufsize" /**< Maximum buffer size per Rx 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;
next prev parent reply other threads:[~2023-09-28 15:56 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-08 4:02 [RFC] " Huisong Li
2023-08-11 12:07 ` Andrew Rybchenko
2023-08-15 8:16 ` lihuisong (C)
2023-08-15 11:10 ` [PATCH v1 0/3] " Huisong Li
2023-08-15 11:10 ` [PATCH v1 1/3] ethdev: " Huisong Li
2023-09-28 15:56 ` Ferruh Yigit [this message]
2023-10-24 12:21 ` lihuisong (C)
2023-08-15 11:10 ` [PATCH v1 2/3] app/testpmd: add maximum Rx buffer size display Huisong Li
2023-08-15 11:10 ` [PATCH v1 3/3] net/hns3: report maximum buffer size Huisong Li
2023-10-27 4:15 ` [PATCH v2 0/3] introduce maximum Rx " Huisong Li
2023-10-27 4:15 ` [PATCH v2 1/3] ethdev: " Huisong Li
2023-10-27 6:27 ` fengchengwen
2023-10-27 7:40 ` Morten Brørup
2023-10-28 1:23 ` lihuisong (C)
2023-10-27 4:15 ` [PATCH v2 2/3] app/testpmd: add maximum Rx buffer size display Huisong Li
2023-10-27 6:28 ` fengchengwen
2023-10-27 4:15 ` [PATCH v2 3/3] net/hns3: report maximum buffer size Huisong Li
2023-10-27 6:17 ` fengchengwen
2023-10-28 1:48 ` [PATCH v3 0/3] introduce maximum Rx " Huisong Li
2023-10-28 1:48 ` [PATCH v3 1/3] ethdev: " Huisong Li
2023-10-29 15:43 ` Stephen Hemminger
2023-10-30 3:08 ` lihuisong (C)
2023-10-28 1:48 ` [PATCH v3 2/3] app/testpmd: add maximum Rx buffer size display Huisong Li
2023-10-28 1:48 ` [PATCH v3 3/3] net/hns3: report maximum buffer size Huisong Li
2023-10-29 15:48 ` [PATCH v3 0/3] introduce maximum Rx " Stephen Hemminger
2023-10-30 1:25 ` lihuisong (C)
2023-10-30 18:48 ` Stephen Hemminger
2023-10-31 2:57 ` lihuisong (C)
2023-10-31 7:48 ` Morten Brørup
2023-10-31 15:40 ` Stephen Hemminger
2023-11-01 2:36 ` lihuisong (C)
2023-11-01 16:08 ` Stephen Hemminger
2023-11-02 1:59 ` lihuisong (C)
2023-11-02 16:23 ` Ferruh Yigit
2023-11-02 16:51 ` Morten Brørup
2023-11-02 17:05 ` Ferruh Yigit
2023-11-02 17:12 ` Morten Brørup
2023-11-02 17:35 ` Ferruh Yigit
2023-11-03 2:13 ` lihuisong (C)
2023-11-02 12:16 ` [PATCH v4 " Huisong Li
2023-11-02 12:16 ` [PATCH v4 1/3] ethdev: " Huisong Li
2023-11-02 16:35 ` Ferruh Yigit
2023-11-03 2:21 ` lihuisong (C)
2023-11-03 3:30 ` Ferruh Yigit
2023-11-03 6:27 ` lihuisong (C)
2023-11-02 12:16 ` [PATCH v4 2/3] app/testpmd: add maximum Rx buffer size display Huisong Li
2023-11-02 16:42 ` Ferruh Yigit
2023-11-03 2:39 ` lihuisong (C)
2023-11-03 3:53 ` Ferruh Yigit
2023-11-03 6:37 ` lihuisong (C)
2023-11-02 12:16 ` [PATCH v4 3/3] net/hns3: report maximum buffer size Huisong Li
2023-11-03 10:27 ` [PATCH v5 0/3] introduce maximum Rx " Huisong Li
2023-11-03 10:27 ` [PATCH v5 1/3] ethdev: " Huisong Li
2023-11-03 12:37 ` Ivan Malov
2023-11-03 10:27 ` [PATCH v5 2/3] app/testpmd: add maximum Rx buffer size display Huisong Li
2023-11-03 10:27 ` [PATCH v5 3/3] net/hns3: report maximum buffer size Huisong Li
2023-11-03 11:53 ` [PATCH v5 0/3] introduce maximum Rx " Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f596c5bf-b73a-4e80-82f9-bfb33e1e5dbf@amd.com \
--to=ferruh.yigit@amd.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=lihuisong@huawei.com \
--cc=liuyonglong@huawei.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).