From: "lihuisong (C)" <lihuisong@huawei.com>
To: Ferruh Yigit <ferruh.yigit@amd.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: Tue, 24 Oct 2023 20:21:17 +0800 [thread overview]
Message-ID: <29c828de-baf0-32fc-5fb9-1e440e6b7ba7@huawei.com> (raw)
In-Reply-To: <f596c5bf-b73a-4e80-82f9-bfb33e1e5dbf@amd.com>
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 <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 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;
> .
next prev parent reply other threads:[~2023-10-24 12:21 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
2023-10-24 12:21 ` lihuisong (C) [this message]
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=29c828de-baf0-32fc-5fb9-1e440e6b7ba7@huawei.com \
--to=lihuisong@huawei.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.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).