[RFC] ethdev: introduce maximum Rx buffer size

[RFC] ethdev: introduce maximum Rx buffer size

[Huisong Li]

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. 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.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 1 +
 lib/ethdev/rte_ethdev.h | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 0840d2b594..6d1b92e607 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -3689,6 +3689,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..1f0ab9c5d8 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1779,8 +1779,8 @@ struct rte_eth_dev_info {
 	struct rte_eth_switch_info switch_info;
 	/** Supported error handling mode. */
 	enum rte_eth_err_handle_mode err_handle_mode;
-
-	uint64_t reserved_64s[2]; /**< Reserved for future fields */
+	uint32_t max_rx_bufsize; /**< Maximum size of Rx buffer. */
+	uint32_t reserved_32s[3]; /**< Reserved for future fields */
 	void *reserved_ptrs[2];   /**< Reserved for future fields */
 };
 
-- 
2.33.0

Re: [RFC] ethdev: introduce maximum Rx buffer size

[Andrew Rybchenko]

On 8/8/23 07:02, 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. 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.

I think that the field should be defined as for informational purposes
only (highlighted in comments). I.e. if application specifies larger Rx
buffer, driver should accept it and just pass smaller value value to HW.
Also I think it would be useful to log warning from Rx queue setup
if provided Rx buffer is larger than maximum reported by the driver.

> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 1 +
>   lib/ethdev/rte_ethdev.h | 4 ++--
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 0840d2b594..6d1b92e607 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3689,6 +3689,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..1f0ab9c5d8 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1779,8 +1779,8 @@ struct rte_eth_dev_info {
>   	struct rte_eth_switch_info switch_info;
>   	/** Supported error handling mode. */
>   	enum rte_eth_err_handle_mode err_handle_mode;
> -
> -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
> +	uint32_t max_rx_bufsize; /**< Maximum size of Rx buffer. */

IMHO, comment should be aligned similar to comments below.
Since the next release is ABI breaking, I think it should be put
nearby min_rx_bufsize to make it easier to notice it.

> +	uint32_t reserved_32s[3]; /**< Reserved for future fields */
>   	void *reserved_ptrs[2];   /**< Reserved for future fields */
>   };
>   

Re: [RFC] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

Hi Andrew,
Thanks for your review.


在 2023/8/11 20:07, Andrew Rybchenko 写道:
> On 8/8/23 07:02, 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. 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.
>
> I think that the field should be defined as for informational purposes
> only (highlighted in comments). I.e. if application specifies larger Rx
> buffer, driver should accept it and just pass smaller value value to HW.
Ok, will add it.
> Also I think it would be useful to log warning from Rx queue setup
> if provided Rx buffer is larger than maximum reported by the driver.
Ack
>
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   lib/ethdev/rte_ethdev.c | 1 +
>>   lib/ethdev/rte_ethdev.h | 4 ++--
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 0840d2b594..6d1b92e607 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -3689,6 +3689,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..1f0ab9c5d8 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -1779,8 +1779,8 @@ struct rte_eth_dev_info {
>>       struct rte_eth_switch_info switch_info;
>>       /** Supported error handling mode. */
>>       enum rte_eth_err_handle_mode err_handle_mode;
>> -
>> -    uint64_t reserved_64s[2]; /**< Reserved for future fields */
>> +    uint32_t max_rx_bufsize; /**< Maximum size of Rx buffer. */
>
> IMHO, comment should be aligned similar to comments below.
> Since the next release is ABI breaking, I think it should be put
> nearby min_rx_bufsize to make it easier to notice it.
Yes, let's put min/max_rx_bufsize together.
>
>> +    uint32_t reserved_32s[3]; /**< Reserved for future fields */
>>       void *reserved_ptrs[2];   /**< Reserved for future fields */
>>   };
>
> .

[PATCH v1 0/3] introduce maximum Rx buffer size

[Huisong Li]

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. Actually, network engines also have the maximum buffer.
So this series introduce maximum Rx buffer size in struct rte_eth_dev_info
and display it by testpmd.

---
v1:
 - move max_rx_bufsize to min_rx_bufsize closer in struct rte_eth_dev_info
 - add max_rx_bufsize display in testpmd.
 - hns3 reports maximum buffer size.

Huisong Li (3):
  ethdev: introduce maximum Rx buffer size
  app/testpmd: add maximum Rx buffer size display
  net/hns3: report maximum buffer size

 app/test-pmd/config.c          | 2 ++
 drivers/net/hns3/hns3_common.c | 1 +
 lib/ethdev/rte_ethdev.c        | 7 +++++++
 lib/ethdev/rte_ethdev.h        | 6 ++++++
 4 files changed, 16 insertions(+)

-- 
2.33.0

[PATCH v1 1/3] ethdev: introduce maximum Rx buffer size

[Huisong Li]

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. 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.

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);
 	} 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.
+	 */
+	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;
-- 
2.33.0

[PATCH v1 2/3] app/testpmd: add maximum Rx buffer size display

[Huisong Li]

Add maximum Rx buffer size display.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 11f3a22048..e0a11a35fe 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -880,6 +880,8 @@ port_infos_display(portid_t port_id)
 	}
 
 	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
+	if (dev_info.max_rx_bufsize != UINT32_MAX)
+		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
 	printf("Maximum configurable length of RX packet: %u\n",
 		dev_info.max_rx_pktlen);
 	printf("Maximum configurable size of LRO aggregated packet: %u\n",
-- 
2.33.0

[PATCH v1 3/3] net/hns3: report maximum buffer size

[Huisong Li]

This patch reports the maximum buffer size hardware supported.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/net/hns3/hns3_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/hns3/hns3_common.c b/drivers/net/hns3/hns3_common.c
index a11ea686fd..6a34df2d75 100644
--- a/drivers/net/hns3/hns3_common.c
+++ b/drivers/net/hns3/hns3_common.c
@@ -59,6 +59,7 @@ hns3_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *info)
 	info->max_tx_queues = hw->tqps_num;
 	info->max_rx_pktlen = HNS3_MAX_FRAME_LEN; /* CRC included */
 	info->min_rx_bufsize = HNS3_MIN_BD_BUF_SIZE;
+	info->max_rx_bufsize = HNS3_MAX_BD_BUF_SIZE;
 	info->max_mtu = info->max_rx_pktlen - HNS3_ETH_OVERHEAD;
 	info->max_lro_pkt_size = HNS3_MAX_LRO_SIZE;
 	info->rx_offload_capa = (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
-- 
2.33.0

Re: [PATCH v1 1/3] ethdev: introduce maximum Rx buffer size

[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,

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;

Re: [PATCH v1 1/3] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

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;
> .

[PATCH v2 0/3] introduce maximum Rx buffer size

[Huisong Li]

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.

If mbuf data room size in mempool is greater then the maximum Rx buffer
size supported by HW, the data size application used in each mbuf is just
as much as the maximum Rx buffer size supported by HW 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.

---
v2
 - fix some comments
 - fix the log level when data room size is greater than maximum Rx
   buffer size.

v1:
 - move max_rx_bufsize to min_rx_bufsize closer in struct rte_eth_dev_info
 - add max_rx_bufsize display in testpmd.
 - hns3 reports maximum buffer size.

Huisong Li (3):
  ethdev: introduce maximum Rx buffer size
  app/testpmd: add maximum Rx buffer size display
  net/hns3: report maximum buffer size

 app/test-pmd/config.c          | 2 ++
 drivers/net/hns3/hns3_common.c | 1 +
 lib/ethdev/rte_ethdev.c        | 7 +++++++
 lib/ethdev/rte_ethdev.h        | 9 ++++++++-
 4 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH v2 1/3] ethdev: introduce maximum Rx buffer size

[Huisong Li]

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. If mbuf data room size
in mempool is greater then the maximum Rx buffer size supported by HW,
the data size application used in each mbuf is just as much as the maximum
Rx buffer size supported by HW 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 <lihuisong@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 7 +++++++
 lib/ethdev/rte_ethdev.h | 9 ++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9dabcb5ae2..eb657a984e 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;
 
 	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,
+				"Ethdev port_id=%u, the data size application used in each mbuf is just %u instead of the whole data room(%u).\n",
+				port_id, dev_info.max_rx_bufsize, vld_bufsize);
 	} 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 2fd3cd808d..5dfb455b2c 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1723,7 +1723,14 @@ 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.
+	 */
+	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;
-- 
2.33.0

[PATCH v2 2/3] app/testpmd: add maximum Rx buffer size display

[Huisong Li]

Add maximum Rx buffer size display.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b9fdb7e8f1..2ac6f15773 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
 	}
 
 	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
+	if (dev_info.max_rx_bufsize != UINT32_MAX)
+		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
 	printf("Maximum configurable length of RX packet: %u\n",
 		dev_info.max_rx_pktlen);
 	printf("Maximum configurable size of LRO aggregated packet: %u\n",
-- 
2.33.0

[PATCH v2 3/3] net/hns3: report maximum buffer size

[Huisong Li]

This patch reports the maximum buffer size hardware supported.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/net/hns3/hns3_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/hns3/hns3_common.c b/drivers/net/hns3/hns3_common.c
index c4d47f43fe..cfce9ddd0f 100644
--- a/drivers/net/hns3/hns3_common.c
+++ b/drivers/net/hns3/hns3_common.c
@@ -59,6 +59,7 @@ hns3_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *info)
 	info->max_tx_queues = hw->tqps_num;
 	info->max_rx_pktlen = HNS3_MAX_FRAME_LEN; /* CRC included */
 	info->min_rx_bufsize = HNS3_MIN_BD_BUF_SIZE;
+	info->max_rx_bufsize = HNS3_MAX_BD_BUF_SIZE;
 	info->max_mtu = info->max_rx_pktlen - HNS3_ETH_OVERHEAD;
 	info->max_lro_pkt_size = HNS3_MAX_LRO_SIZE;
 	info->rx_offload_capa = (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
-- 
2.33.0

Re: [PATCH v2 3/3] net/hns3: report maximum buffer size

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>


On 2023/10/27 12:15, Huisong Li wrote:
> This patch reports the maximum buffer size hardware supported.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  drivers/net/hns3/hns3_common.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/hns3/hns3_common.c b/drivers/net/hns3/hns3_common.c
> index c4d47f43fe..cfce9ddd0f 100644
> --- a/drivers/net/hns3/hns3_common.c
> +++ b/drivers/net/hns3/hns3_common.c
> @@ -59,6 +59,7 @@ hns3_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *info)
>  	info->max_tx_queues = hw->tqps_num;
>  	info->max_rx_pktlen = HNS3_MAX_FRAME_LEN; /* CRC included */
>  	info->min_rx_bufsize = HNS3_MIN_BD_BUF_SIZE;
> +	info->max_rx_bufsize = HNS3_MAX_BD_BUF_SIZE;
>  	info->max_mtu = info->max_rx_pktlen - HNS3_ETH_OVERHEAD;
>  	info->max_lro_pkt_size = HNS3_MAX_LRO_SIZE;
>  	info->rx_offload_capa = (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
> 

Re: [PATCH v2 1/3] ethdev: introduce maximum Rx buffer size

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>


On 2023/10/27 12:15, 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. If mbuf data room size
> in mempool is greater then the maximum Rx buffer size supported by HW,
> the data size application used in each mbuf is just as much as the maximum
> Rx buffer size supported by HW 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 <lihuisong@huawei.com>

...

Re: [PATCH v2 2/3] app/testpmd: add maximum Rx buffer size display

[fengchengwen]

Acked-by: Chengwen Feng <fengchengwen@huawei.com>


On 2023/10/27 12:15, Huisong Li wrote:
> Add maximum Rx buffer size display.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  app/test-pmd/config.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index b9fdb7e8f1..2ac6f15773 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
>  	}
>  
>  	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
> +	if (dev_info.max_rx_bufsize != UINT32_MAX)
> +		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
>  	printf("Maximum configurable length of RX packet: %u\n",
>  		dev_info.max_rx_pktlen);
>  	printf("Maximum configurable size of LRO aggregated packet: %u\n",
> 

RE: [PATCH v2 1/3] ethdev: introduce maximum Rx buffer size

[Morten Brørup]

> From: Huisong Li [mailto:lihuisong@huawei.com]
> Sent: Friday, 27 October 2023 06.15
> 
> 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. If mbuf data room size
> in mempool is greater then the maximum Rx buffer size supported by HW,
> the data size application used in each mbuf is just as much as the
> maximum
> Rx buffer size supported by HW 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 <lihuisong@huawei.com>
> ---

[...]

> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1723,7 +1723,14 @@ 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;

The comment was moved above min_rx_bufsize, so you must use "/** " instead of "/**< ".

> +	/**
> +	 * Maximum Rx buffer size per descriptor supported by HW.
> +	 * The value is not enforced, information only to application to
> +	 * optimize mbuf size.
> +	 */
> +	uint32_t max_rx_bufsize;

The comment should mention that the value is UINT32_MAX when not specified by the driver.

With those to changes,

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2 1/3] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

在 2023/10/27 15:40, Morten Brørup 写道:
>> From: Huisong Li [mailto:lihuisong@huawei.com]
>> Sent: Friday, 27 October 2023 06.15
>>
>> 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. If mbuf data room size
>> in mempool is greater then the maximum Rx buffer size supported by HW,
>> the data size application used in each mbuf is just as much as the
>> maximum
>> Rx buffer size supported by HW 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 <lihuisong@huawei.com>
>> ---
> [...]
>
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -1723,7 +1723,14 @@ 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;
> The comment was moved above min_rx_bufsize, so you must use "/** " instead of "/**< ".
You are right. will fix it in next version.
>
>> +	/**
>> +	 * Maximum Rx buffer size per descriptor supported by HW.
>> +	 * The value is not enforced, information only to application to
>> +	 * optimize mbuf size.
>> +	 */
>> +	uint32_t max_rx_bufsize;
> The comment should mention that the value is UINT32_MAX when not specified by the driver.
Ack
>
> With those to changes,
>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>
>
> .

[PATCH v3 0/3] introduce maximum Rx buffer size

[Huisong Li]

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.

If mbuf data room size in mempool is greater then the maximum Rx buffer
size supported by HW, the data size application used in each mbuf is just
as much as the maximum Rx buffer size supported by HW 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.

---
v3:
 - fix some comments for Morten's advice.

v2:
 - fix some comments
 - fix the log level when data room size is greater than maximum Rx
   buffer size.

v1:
 - move max_rx_bufsize to min_rx_bufsize closer in struct rte_eth_dev_info
 - add max_rx_bufsize display in testpmd.
 - hns3 reports maximum buffer size.


Huisong Li (3):
  ethdev: introduce maximum Rx buffer size
  app/testpmd: add maximum Rx buffer size display
  net/hns3: report maximum buffer size

 app/test-pmd/config.c          |  2 ++
 drivers/net/hns3/hns3_common.c |  1 +
 lib/ethdev/rte_ethdev.c        |  7 +++++++
 lib/ethdev/rte_ethdev.h        | 10 +++++++++-
 4 files changed, 19 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH v3 1/3] ethdev: introduce maximum Rx buffer size

[Huisong Li]

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. If mbuf data room size
in mempool is greater then the maximum Rx buffer size supported by HW,
the data size application used in each mbuf is just as much as the maximum
Rx buffer size supported by HW 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 <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ethdev/rte_ethdev.c |  7 +++++++
 lib/ethdev/rte_ethdev.h | 10 +++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9dabcb5ae2..eb657a984e 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;
 
 	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,
+				"Ethdev port_id=%u, the data size application used in each mbuf is just %u instead of the whole data room(%u).\n",
+				port_id, dev_info.max_rx_bufsize, vld_bufsize);
 	} 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 2fd3cd808d..b8f5dcae00 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;
-- 
2.33.0

[PATCH v3 2/3] app/testpmd: add maximum Rx buffer size display

[Huisong Li]

Add maximum Rx buffer size display.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b9fdb7e8f1..2ac6f15773 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
 	}
 
 	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
+	if (dev_info.max_rx_bufsize != UINT32_MAX)
+		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
 	printf("Maximum configurable length of RX packet: %u\n",
 		dev_info.max_rx_pktlen);
 	printf("Maximum configurable size of LRO aggregated packet: %u\n",
-- 
2.33.0

[PATCH v3 3/3] net/hns3: report maximum buffer size

[Huisong Li]

This patch reports the maximum buffer size hardware supported.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/hns3/hns3_common.c b/drivers/net/hns3/hns3_common.c
index c4d47f43fe..cfce9ddd0f 100644
--- a/drivers/net/hns3/hns3_common.c
+++ b/drivers/net/hns3/hns3_common.c
@@ -59,6 +59,7 @@ hns3_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *info)
 	info->max_tx_queues = hw->tqps_num;
 	info->max_rx_pktlen = HNS3_MAX_FRAME_LEN; /* CRC included */
 	info->min_rx_bufsize = HNS3_MIN_BD_BUF_SIZE;
+	info->max_rx_bufsize = HNS3_MAX_BD_BUF_SIZE;
 	info->max_mtu = info->max_rx_pktlen - HNS3_ETH_OVERHEAD;
 	info->max_lro_pkt_size = HNS3_MAX_LRO_SIZE;
 	info->rx_offload_capa = (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
-- 
2.33.0

Re: [PATCH v3 1/3] ethdev: introduce maximum Rx buffer size

[Stephen Hemminger]

On Sat, 28 Oct 2023 09:48:45 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> +				"Ethdev port_id=%u, the data size application used in each mbuf is just %u instead of the whole data room(%u)

The message should be shortened. 

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Stephen Hemminger]

On Sat, 28 Oct 2023 09:48:44 +0800
Huisong Li <lihuisong@huawei.com> 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.
> 
> If mbuf data room size in mempool is greater then the maximum Rx buffer
> size supported by HW, the data size application used in each mbuf is just
> as much as the maximum Rx buffer size supported by HW 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.

I am not convinced this is really necessary.
Your device will use up to 4K of buffer size, not sure why an application
would want to use much larger than that because it would be wasting
a lot of buffer space (most packets are smaller) anyway.

The only case where it might be useful is if application is using jumbo
frames (9K) and the application was not able to handle multi segment packets.
Not handling multi segment packets in SW is just programmer laziness.

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[lihuisong (C)]

在 2023/10/29 23:48, Stephen Hemminger 写道:
> On Sat, 28 Oct 2023 09:48:44 +0800
> Huisong Li <lihuisong@huawei.com> 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.
>>
>> If mbuf data room size in mempool is greater then the maximum Rx buffer
>> size supported by HW, the data size application used in each mbuf is just
>> as much as the maximum Rx buffer size supported by HW 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.
> I am not convinced this is really necessary.
> Your device will use up to 4K of buffer size, not sure why an application
> would want to use much larger than that because it would be wasting
> a lot of buffer space (most packets are smaller) anyway.
>
> The only case where it might be useful is if application is using jumbo
> frames (9K) and the application was not able to handle multi segment packets.
Yeah, it is useful if user want a large packet (like, 6K) is in a mbuf.
But, in current layer, user don't know what the maximum buffer size per 
descriptor supported by HW is.
> Not handling multi segment packets in SW is just programmer laziness.
User do decide their implement based on their cases in project.
May it be a point for this that user don't want to do memcpy for multi 
segment packets and just use the first mbuf memory.

Now that there is the "min_rx_bufsize" to report in ethdev layer.
Anyway, DPDK is indeed the lack of the way to report the maximum Rx 
buffer size per hw descriptor.
>
> .

Re: [PATCH v3 1/3] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

Hi Stephen,

Thanks for your review.


在 2023/10/29 23:43, Stephen Hemminger 写道:
> On Sat, 28 Oct 2023 09:48:45 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> +				"Ethdev port_id=%u, the data size application used in each mbuf is just %u instead of the whole data room(%u)
> The message should be shortened.
What do you think of the following log?
"The data size user used in mbuf is just %u instead of the whole data 
room(%u)"
I find that a shortest log.

/Huisong

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Stephen Hemminger]

On Mon, 30 Oct 2023 09:25:34 +0800
"lihuisong (C)" <lihuisong@huawei.com> 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.
> >>
> >> If mbuf data room size in mempool is greater then the maximum Rx
> >> buffer size supported by HW, the data size application used in
> >> each mbuf is just as much as the maximum Rx buffer size supported
> >> by HW 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.  
> > I am not convinced this is really necessary.
> > Your device will use up to 4K of buffer size, not sure why an
> > application would want to use much larger than that because it
> > would be wasting a lot of buffer space (most packets are smaller)
> > anyway.
> >
> > The only case where it might be useful is if application is using
> > jumbo frames (9K) and the application was not able to handle multi
> > segment packets.  
> Yeah, it is useful if user want a large packet (like, 6K) is in a
> mbuf. But, in current layer, user don't know what the maximum buffer
> size per descriptor supported by HW is.
> > Not handling multi segment packets in SW is just programmer
> > laziness.  
> User do decide their implement based on their cases in project.
> May it be a point for this that user don't want to do memcpy for
> multi segment packets and just use the first mbuf memory.
> 
> Now that there is the "min_rx_bufsize" to report in ethdev layer.
> Anyway, DPDK is indeed the lack of the way to report the maximum Rx 
> buffer size per hw descriptor.

My concern is that you are creating a special case for one driver.
And other drivers probably have similar upper bound.

Could the warning be better handled in the driver specific configure
routine rather than updating the ethdev API.  Something like:

   if (multi-segment-flag off) {
       if (mtu > driver max buf size) {
               return error;
   } else {
       if (mtu > driver max buf size && 
           mtu < mempool_buf_size(mp)) {
         warn that packet maybe segmented ??
       }
   }

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[lihuisong (C)]

在 2023/10/31 2:48, Stephen Hemminger 写道:
> On Mon, 30 Oct 2023 09:25:34 +0800
> "lihuisong (C)" <lihuisong@huawei.com> 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.
>>>>
>>>> If mbuf data room size in mempool is greater then the maximum Rx
>>>> buffer size supported by HW, the data size application used in
>>>> each mbuf is just as much as the maximum Rx buffer size supported
>>>> by HW 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.
>>> I am not convinced this is really necessary.
>>> Your device will use up to 4K of buffer size, not sure why an
>>> application would want to use much larger than that because it
>>> would be wasting a lot of buffer space (most packets are smaller)
>>> anyway.
>>>
>>> The only case where it might be useful is if application is using
>>> jumbo frames (9K) and the application was not able to handle multi
>>> segment packets.
>> Yeah, it is useful if user want a large packet (like, 6K) is in a
>> mbuf. But, in current layer, user don't know what the maximum buffer
>> size per descriptor supported by HW is.
>>> Not handling multi segment packets in SW is just programmer
>>> laziness.
>> User do decide their implement based on their cases in project.
>> May it be a point for this that user don't want to do memcpy for
>> multi segment packets and just use the first mbuf memory.
>>
>> Now that there is the "min_rx_bufsize" to report in ethdev layer.
>> Anyway, DPDK is indeed the lack of the way to report the maximum Rx
>> buffer size per hw descriptor.
> My concern is that you are creating a special case for one driver.
understand your concern.
> And other drivers probably have similar upper bound.
Yes, they also have similar upper bound.
 From the codes, the max buffer size of Most PMDs are 16K and bnxt is 
9600Byte.
Do we need to report this size? It's a common feature for all PMDs.
>
> Could the warning be better handled in the driver specific configure
> routine rather than updating the ethdev API.  Something like:
>
>     if (multi-segment-flag off) {
>         if (mtu > driver max buf size) {
>                 return error;
>     } else {
>         if (mtu > driver max buf size &&
>             mtu < mempool_buf_size(mp)) {
>           warn that packet maybe segmented ??
>         }
>     }
>
>
> .

RE: [PATCH v3 0/3] introduce maximum Rx buffer size

[Morten Brørup]

> From: lihuisong (C) [mailto:lihuisong@huawei.com]
> Sent: Tuesday, 31 October 2023 03.58
> 
> 在 2023/10/31 2:48, Stephen Hemminger 写道:
> > On Mon, 30 Oct 2023 09:25:34 +0800
> > "lihuisong (C)" <lihuisong@huawei.com> 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.
> >>>>
> >>>> If mbuf data room size in mempool is greater then the maximum Rx
> >>>> buffer size supported by HW, the data size application used in
> >>>> each mbuf is just as much as the maximum Rx buffer size supported
> >>>> by HW 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.
> >>> I am not convinced this is really necessary.
> >>> Your device will use up to 4K of buffer size, not sure why an
> >>> application would want to use much larger than that because it
> >>> would be wasting a lot of buffer space (most packets are smaller)
> >>> anyway.
> >>>
> >>> The only case where it might be useful is if application is using
> >>> jumbo frames (9K) and the application was not able to handle multi
> >>> segment packets.
> >> Yeah, it is useful if user want a large packet (like, 6K) is in a
> >> mbuf. But, in current layer, user don't know what the maximum buffer
> >> size per descriptor supported by HW is.
> >>> Not handling multi segment packets in SW is just programmer
> >>> laziness.
> >> User do decide their implement based on their cases in project.
> >> May it be a point for this that user don't want to do memcpy for
> >> multi segment packets and just use the first mbuf memory.
> >>
> >> Now that there is the "min_rx_bufsize" to report in ethdev layer.
> >> Anyway, DPDK is indeed the lack of the way to report the maximum Rx
> >> buffer size per hw descriptor.
> > My concern is that you are creating a special case for one driver.
> understand your concern.
> > And other drivers probably have similar upper bound.
> Yes, they also have similar upper bound.
>  From the codes, the max buffer size of Most PMDs are 16K and bnxt is
> 9600Byte.
> Do we need to report this size? It's a common feature for all PMDs.

I think this could be a useful feature for applications not wanting to deal with scattered packets. I don't consider such applications exotic, so I support adding this feature.

> >
> > Could the warning be better handled in the driver specific configure
> > routine rather than updating the ethdev API.  Something like:
> >
> >     if (multi-segment-flag off) {

Many drivers ignore the RTE_ETH_RX_OFFLOAD_SCATTER configuration, and only use RTE_ETH_RX_OFFLOAD_SCATTER for capabilities reporting. Perhaps this should be reported in Bugzilla?

> >         if (mtu > driver max buf size) {
> >                 return error;
> >     } else {
> >         if (mtu > driver max buf size &&
> >             mtu < mempool_buf_size(mp)) {
> >           warn that packet maybe segmented ??
> >         }
> >     }

Such a log message would also be useful.

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Stephen Hemminger]

On Tue, 31 Oct 2023 10:57:45 +0800
"lihuisong (C)" <lihuisong@huawei.com> wrote:

> >> User do decide their implement based on their cases in project.
> >> May it be a point for this that user don't want to do memcpy for
> >> multi segment packets and just use the first mbuf memory.
> >>
> >> Now that there is the "min_rx_bufsize" to report in ethdev layer.
> >> Anyway, DPDK is indeed the lack of the way to report the maximum Rx
> >> buffer size per hw descriptor.  
> > My concern is that you are creating a special case for one driver.  
> understand your concern.
> > And other drivers probably have similar upper bound.  
> Yes, they also have similar upper bound.
>  From the codes, the max buffer size of Most PMDs are 16K and bnxt is 
> 9600Byte.
> Do we need to report this size? It's a common feature for all PMDs.

It would make sense then to have max_rx_bufsize set to 16K by default
in ethdev, and PMD could then raise/lower based on hardware.

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[lihuisong (C)]

Hi Stephen,

在 2023/10/31 23:40, Stephen Hemminger 写道:
> On Tue, 31 Oct 2023 10:57:45 +0800
> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>
>>>> User do decide their implement based on their cases in project.
>>>> May it be a point for this that user don't want to do memcpy for
>>>> multi segment packets and just use the first mbuf memory.
>>>>
>>>> Now that there is the "min_rx_bufsize" to report in ethdev layer.
>>>> Anyway, DPDK is indeed the lack of the way to report the maximum Rx
>>>> buffer size per hw descriptor.
>>> My concern is that you are creating a special case for one driver.
>> understand your concern.
>>> And other drivers probably have similar upper bound.
>> Yes, they also have similar upper bound.
>>   From the codes, the max buffer size of Most PMDs are 16K and bnxt is
>> 9600Byte.
>> Do we need to report this size? It's a common feature for all PMDs.
> It would make sense then to have max_rx_bufsize set to 16K by default
> in ethdev, and PMD could then raise/lower based on hardware.
It is not appropriate to set to 16K by default in ethdev layer.
Because I don't see any check for the upper bound in some driver, like 
axgbe, enetc and so on.
I'm not sure if they have no upper bound.
And some driver's maximum buffer size is "16384(16K) - 128"
So it's better to set to UINT32_MAX by default.
what do you think?
>
> .

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Stephen Hemminger]

On Wed, 1 Nov 2023 10:36:07 +0800
"lihuisong (C)" <lihuisong@huawei.com> wrote:

> > Do we need to report this size? It's a common feature for all PMDs.  
> > It would make sense then to have max_rx_bufsize set to 16K by default
> > in ethdev, and PMD could then raise/lower based on hardware.  
> It is not appropriate to set to 16K by default in ethdev layer.
> Because I don't see any check for the upper bound in some driver, like 
> axgbe, enetc and so on.
> I'm not sure if they have no upper bound.
> And some driver's maximum buffer size is "16384(16K) - 128"
> So it's better to set to UINT32_MAX by default.
> what do you think?

The goal is always giving application a working upper bound, and enforcing
that as much as possible in ethdev layer. It doesnt matter which pattern
does that.  Fortunately, telling application an incorrect answer is not fatal.
If over estimated, application pool would be wasting space.
If under estimated, application will get more fragmented packets.

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[lihuisong (C)]

在 2023/11/2 0:08, Stephen Hemminger 写道:
> On Wed, 1 Nov 2023 10:36:07 +0800
> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>
>>> Do we need to report this size? It's a common feature for all PMDs.
>>> It would make sense then to have max_rx_bufsize set to 16K by default
>>> in ethdev, and PMD could then raise/lower based on hardware.
>> It is not appropriate to set to 16K by default in ethdev layer.
>> Because I don't see any check for the upper bound in some driver, like
>> axgbe, enetc and so on.
>> I'm not sure if they have no upper bound.
>> And some driver's maximum buffer size is "16384(16K) - 128"
>> So it's better to set to UINT32_MAX by default.
>> what do you think?
> The goal is always giving application a working upper bound, and enforcing
> that as much as possible in ethdev layer. It doesnt matter which pattern
> does that.  Fortunately, telling application an incorrect answer is not fatal.
> If over estimated, application pool would be wasting space.
> If under estimated, application will get more fragmented packets.
I know what you mean.
If we set UINT32_MAX, it just means that driver don't report this upper 
bound.
This is also a very common way of handling. And it has no effect on the 
drivers that doesn't report this value.
On the contrary, if we set a default value (like 16K) in ethdev, user 
may be misunderstood and confused by that, right?
After all, this isn't the real upper bound of all drivers. And this 
fixed default value may affect the behavior of some driver that I didn't 
find their upper bound.
So I'd like to keep it as UINT32_MAX.
>
> .

[PATCH v4 0/3] introduce maximum Rx buffer size

[Huisong Li]

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.

If mbuf data room size in mempool is greater then the maximum Rx buffer
size supported by HW, the data size application used in each mbuf is just
as much as the maximum Rx buffer size supported by HW 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.

---
v4:
 - fix the log in rte_eth_rx_queue_setup.
 - add a comment in rel_notes.

v3:
 - fix some comments for Morten's advice.

v2:
 - fix some comments
 - fix the log level when data room size is greater than maximum Rx
   buffer size.

v1:
 - move max_rx_bufsize to min_rx_bufsize closer in struct rte_eth_dev_info
 - add max_rx_bufsize display in testpmd.
 - hns3 reports maximum buffer size.

Huisong Li (3):
  ethdev: introduce maximum Rx buffer size
  app/testpmd: add maximum Rx buffer size display
  net/hns3: report maximum buffer size

 app/test-pmd/config.c                  |  2 ++
 doc/guides/rel_notes/release_23_11.rst |  7 +++++++
 drivers/net/hns3/hns3_common.c         |  1 +
 lib/ethdev/rte_ethdev.c                |  7 +++++++
 lib/ethdev/rte_ethdev.h                | 10 +++++++++-
 5 files changed, 26 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH v4 1/3] ethdev: introduce maximum Rx buffer size

[Huisong Li]

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 <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 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;
 
 	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);
 	} 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;
-- 
2.33.0

[PATCH v4 2/3] app/testpmd: add maximum Rx buffer size display

[Huisong Li]

Add maximum Rx buffer size display.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b9fdb7e8f1..2ac6f15773 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
 	}
 
 	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
+	if (dev_info.max_rx_bufsize != UINT32_MAX)
+		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
 	printf("Maximum configurable length of RX packet: %u\n",
 		dev_info.max_rx_pktlen);
 	printf("Maximum configurable size of LRO aggregated packet: %u\n",
-- 
2.33.0

[PATCH v4 3/3] net/hns3: report maximum buffer size

[Huisong Li]

This patch reports the maximum buffer size hardware supported.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/hns3/hns3_common.c b/drivers/net/hns3/hns3_common.c
index 9327adbdc1..4b0e38cf67 100644
--- a/drivers/net/hns3/hns3_common.c
+++ b/drivers/net/hns3/hns3_common.c
@@ -59,6 +59,7 @@ hns3_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *info)
 	info->max_tx_queues = hw->tqps_num;
 	info->max_rx_pktlen = HNS3_MAX_FRAME_LEN; /* CRC included */
 	info->min_rx_bufsize = HNS3_MIN_BD_BUF_SIZE;
+	info->max_rx_bufsize = HNS3_MAX_BD_BUF_SIZE;
 	info->max_mtu = info->max_rx_pktlen - HNS3_ETH_OVERHEAD;
 	info->max_lro_pkt_size = HNS3_MAX_LRO_SIZE;
 	info->rx_offload_capa = (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
-- 
2.33.0

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Ferruh Yigit]

On 11/2/2023 1:59 AM, lihuisong (C) wrote:
> 
> 在 2023/11/2 0:08, Stephen Hemminger 写道:
>> On Wed, 1 Nov 2023 10:36:07 +0800
>> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>>
>>>> Do we need to report this size? It's a common feature for all PMDs.
>>>> It would make sense then to have max_rx_bufsize set to 16K by default
>>>> in ethdev, and PMD could then raise/lower based on hardware.
>>> It is not appropriate to set to 16K by default in ethdev layer.
>>> Because I don't see any check for the upper bound in some driver, like
>>> axgbe, enetc and so on.
>>> I'm not sure if they have no upper bound.
>>> And some driver's maximum buffer size is "16384(16K) - 128"
>>> So it's better to set to UINT32_MAX by default.
>>> what do you think?
>> The goal is always giving application a working upper bound, and
>> enforcing
>> that as much as possible in ethdev layer. It doesnt matter which pattern
>> does that.  Fortunately, telling application an incorrect answer is
>> not fatal.
>> If over estimated, application pool would be wasting space.
>> If under estimated, application will get more fragmented packets.
> I know what you mean.
> If we set UINT32_MAX, it just means that driver don't report this upper
> bound.
> This is also a very common way of handling. And it has no effect on the
> drivers that doesn't report this value.
> On the contrary, if we set a default value (like 16K) in ethdev, user
> may be misunderstood and confused by that, right?
> After all, this isn't the real upper bound of all drivers. And this
> fixed default value may affect the behavior of some driver that I didn't
> find their upper bound.
> So I'd like to keep it as UINT32_MAX.
> 


Hi Stephen, Morten,

I saw scattered Rx mentioned, there may be some misalignment,
the purpose of the patch is not to enable application to set as big as
possible mbuf size, so that application can escape from parsing
multi-segment mbufs.
Indeed application can provide a large mbuf anyway, to have same result,
without knowing this information.

Main motivation is other way around, device may have restriction on
buffer size that a single descriptor can address, independent from
scattered Rx used, if mbuf size is bigger than this device limit, each
mbuf will have some unused space.
Patch has intention to inform this max per mbuf/descriptor buffer size,
so that application doesn't allocate bigger mbuf and waste memory.

Re: [PATCH v4 1/3] ethdev: introduce maximum Rx buffer size

[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 <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  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'?

>  	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'?

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."



>  	} 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;

Re: [PATCH v4 2/3] app/testpmd: add maximum Rx buffer size display

[Ferruh Yigit]

On 11/2/2023 12:16 PM, Huisong Li wrote:
> Add maximum Rx buffer size display.
> 

I think there is a value to show what is the intended usage of this new
field in application level,
that is why what do you think to use testpmd?

Testpmd can be updated to check if mbuf data size is bigger than device
buffer size, and just log, similar to done in ethdev layer, just to
showcase how application should use it.

For many drivers, this won't have anything functional, because of
UINT32_MAX size, but it is still valuable as sample.


> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  app/test-pmd/config.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index b9fdb7e8f1..2ac6f15773 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
>  	}
>  
>  	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
> +	if (dev_info.max_rx_bufsize != UINT32_MAX)
> +		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
>  	printf("Maximum configurable length of RX packet: %u\n",
>  		dev_info.max_rx_pktlen);
>  	printf("Maximum configurable size of LRO aggregated packet: %u\n",

RE: [PATCH v3 0/3] introduce maximum Rx buffer size

[Morten Brørup]

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Thursday, 2 November 2023 17.24
> 
> On 11/2/2023 1:59 AM, lihuisong (C) wrote:
> >
> > 在 2023/11/2 0:08, Stephen Hemminger 写道:
> >> On Wed, 1 Nov 2023 10:36:07 +0800
> >> "lihuisong (C)" <lihuisong@huawei.com> wrote:
> >>
> >>>> Do we need to report this size? It's a common feature for all
> PMDs.
> >>>> It would make sense then to have max_rx_bufsize set to 16K by
> default
> >>>> in ethdev, and PMD could then raise/lower based on hardware.
> >>> It is not appropriate to set to 16K by default in ethdev layer.
> >>> Because I don't see any check for the upper bound in some driver,
> like
> >>> axgbe, enetc and so on.
> >>> I'm not sure if they have no upper bound.
> >>> And some driver's maximum buffer size is "16384(16K) - 128"
> >>> So it's better to set to UINT32_MAX by default.
> >>> what do you think?
> >> The goal is always giving application a working upper bound, and
> >> enforcing
> >> that as much as possible in ethdev layer. It doesnt matter which
> pattern
> >> does that.  Fortunately, telling application an incorrect answer is
> >> not fatal.
> >> If over estimated, application pool would be wasting space.
> >> If under estimated, application will get more fragmented packets.
> > I know what you mean.
> > If we set UINT32_MAX, it just means that driver don't report this
> upper
> > bound.
> > This is also a very common way of handling. And it has no effect on
> the
> > drivers that doesn't report this value.
> > On the contrary, if we set a default value (like 16K) in ethdev, user
> > may be misunderstood and confused by that, right?
> > After all, this isn't the real upper bound of all drivers. And this
> > fixed default value may affect the behavior of some driver that I
> didn't
> > find their upper bound.
> > So I'd like to keep it as UINT32_MAX.
> >
> 
> 
> Hi Stephen, Morten,
> 
> I saw scattered Rx mentioned, there may be some misalignment,
> the purpose of the patch is not to enable application to set as big as
> possible mbuf size, so that application can escape from parsing
> multi-segment mbufs.
> Indeed application can provide a large mbuf anyway, to have same
> result,
> without knowing this information.
> 
> Main motivation is other way around, device may have restriction on
> buffer size that a single descriptor can address, independent from
> scattered Rx used, if mbuf size is bigger than this device limit, each
> mbuf will have some unused space.
> Patch has intention to inform this max per mbuf/descriptor buffer size,
> so that application doesn't allocate bigger mbuf and waste memory.

Good point!

Let's categorize this patch series as a memory optimization for applications that support jumbo frames, but are trying to avoid (or reduce) scattered RX. :-)

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Ferruh Yigit]

On 11/2/2023 4:51 PM, Morten Brørup wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
>> Sent: Thursday, 2 November 2023 17.24
>>
>> On 11/2/2023 1:59 AM, lihuisong (C) wrote:
>>>
>>> 在 2023/11/2 0:08, Stephen Hemminger 写道:
>>>> On Wed, 1 Nov 2023 10:36:07 +0800
>>>> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>>>>
>>>>>> Do we need to report this size? It's a common feature for all
>> PMDs.
>>>>>> It would make sense then to have max_rx_bufsize set to 16K by
>> default
>>>>>> in ethdev, and PMD could then raise/lower based on hardware.
>>>>> It is not appropriate to set to 16K by default in ethdev layer.
>>>>> Because I don't see any check for the upper bound in some driver,
>> like
>>>>> axgbe, enetc and so on.
>>>>> I'm not sure if they have no upper bound.
>>>>> And some driver's maximum buffer size is "16384(16K) - 128"
>>>>> So it's better to set to UINT32_MAX by default.
>>>>> what do you think?
>>>> The goal is always giving application a working upper bound, and
>>>> enforcing
>>>> that as much as possible in ethdev layer. It doesnt matter which
>> pattern
>>>> does that.  Fortunately, telling application an incorrect answer is
>>>> not fatal.
>>>> If over estimated, application pool would be wasting space.
>>>> If under estimated, application will get more fragmented packets.
>>> I know what you mean.
>>> If we set UINT32_MAX, it just means that driver don't report this
>> upper
>>> bound.
>>> This is also a very common way of handling. And it has no effect on
>> the
>>> drivers that doesn't report this value.
>>> On the contrary, if we set a default value (like 16K) in ethdev, user
>>> may be misunderstood and confused by that, right?
>>> After all, this isn't the real upper bound of all drivers. And this
>>> fixed default value may affect the behavior of some driver that I
>> didn't
>>> find their upper bound.
>>> So I'd like to keep it as UINT32_MAX.
>>>
>>
>>
>> Hi Stephen, Morten,
>>
>> I saw scattered Rx mentioned, there may be some misalignment,
>> the purpose of the patch is not to enable application to set as big as
>> possible mbuf size, so that application can escape from parsing
>> multi-segment mbufs.
>> Indeed application can provide a large mbuf anyway, to have same
>> result,
>> without knowing this information.
>>
>> Main motivation is other way around, device may have restriction on
>> buffer size that a single descriptor can address, independent from
>> scattered Rx used, if mbuf size is bigger than this device limit, each
>> mbuf will have some unused space.
>> Patch has intention to inform this max per mbuf/descriptor buffer size,
>> so that application doesn't allocate bigger mbuf and waste memory.
> 
> Good point!
> 
> Let's categorize this patch series as a memory optimization for applications that support jumbo frames, but are trying to avoid (or reduce) scattered RX. :-)
> 

It is a memory optimization patch, but again nothing to do with jumbo
frames or scattered Rx.

RE: [PATCH v3 0/3] introduce maximum Rx buffer size

[Morten Brørup]

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Thursday, 2 November 2023 18.06
> 
> On 11/2/2023 4:51 PM, Morten Brørup wrote:
> >> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> >> Sent: Thursday, 2 November 2023 17.24
> >>
> >> On 11/2/2023 1:59 AM, lihuisong (C) wrote:
> >>>
> >>> 在 2023/11/2 0:08, Stephen Hemminger 写道:
> >>>> On Wed, 1 Nov 2023 10:36:07 +0800
> >>>> "lihuisong (C)" <lihuisong@huawei.com> wrote:
> >>>>
> >>>>>> Do we need to report this size? It's a common feature for all
> >> PMDs.
> >>>>>> It would make sense then to have max_rx_bufsize set to 16K by
> >> default
> >>>>>> in ethdev, and PMD could then raise/lower based on hardware.
> >>>>> It is not appropriate to set to 16K by default in ethdev layer.
> >>>>> Because I don't see any check for the upper bound in some driver,
> >> like
> >>>>> axgbe, enetc and so on.
> >>>>> I'm not sure if they have no upper bound.
> >>>>> And some driver's maximum buffer size is "16384(16K) - 128"
> >>>>> So it's better to set to UINT32_MAX by default.
> >>>>> what do you think?
> >>>> The goal is always giving application a working upper bound, and
> >>>> enforcing
> >>>> that as much as possible in ethdev layer. It doesnt matter which
> >> pattern
> >>>> does that.  Fortunately, telling application an incorrect answer
> is
> >>>> not fatal.
> >>>> If over estimated, application pool would be wasting space.
> >>>> If under estimated, application will get more fragmented packets.
> >>> I know what you mean.
> >>> If we set UINT32_MAX, it just means that driver don't report this
> >> upper
> >>> bound.
> >>> This is also a very common way of handling. And it has no effect on
> >> the
> >>> drivers that doesn't report this value.
> >>> On the contrary, if we set a default value (like 16K) in ethdev,
> user
> >>> may be misunderstood and confused by that, right?
> >>> After all, this isn't the real upper bound of all drivers. And this
> >>> fixed default value may affect the behavior of some driver that I
> >> didn't
> >>> find their upper bound.
> >>> So I'd like to keep it as UINT32_MAX.
> >>>
> >>
> >>
> >> Hi Stephen, Morten,
> >>
> >> I saw scattered Rx mentioned, there may be some misalignment,
> >> the purpose of the patch is not to enable application to set as big
> as
> >> possible mbuf size, so that application can escape from parsing
> >> multi-segment mbufs.
> >> Indeed application can provide a large mbuf anyway, to have same
> >> result,
> >> without knowing this information.
> >>
> >> Main motivation is other way around, device may have restriction on
> >> buffer size that a single descriptor can address, independent from
> >> scattered Rx used, if mbuf size is bigger than this device limit,
> each
> >> mbuf will have some unused space.
> >> Patch has intention to inform this max per mbuf/descriptor buffer
> size,
> >> so that application doesn't allocate bigger mbuf and waste memory.
> >
> > Good point!
> >
> > Let's categorize this patch series as a memory optimization for
> applications that support jumbo frames, but are trying to avoid (or
> reduce) scattered RX. :-)
> >
> 
> It is a memory optimization patch, but again nothing to do with jumbo
> frames or scattered Rx.

I expect all NICs to support standard Ethernet frames without scattered RX.

So I consider this patch related to jumbo frames (and non-scattered RX). Is there any other use case?

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[Ferruh Yigit]

On 11/2/2023 5:12 PM, Morten Brørup wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
>> Sent: Thursday, 2 November 2023 18.06
>>
>> On 11/2/2023 4:51 PM, Morten Brørup wrote:
>>>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
>>>> Sent: Thursday, 2 November 2023 17.24
>>>>
>>>> On 11/2/2023 1:59 AM, lihuisong (C) wrote:
>>>>>
>>>>> 在 2023/11/2 0:08, Stephen Hemminger 写道:
>>>>>> On Wed, 1 Nov 2023 10:36:07 +0800
>>>>>> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>>>>>>
>>>>>>>> Do we need to report this size? It's a common feature for all
>>>> PMDs.
>>>>>>>> It would make sense then to have max_rx_bufsize set to 16K by
>>>> default
>>>>>>>> in ethdev, and PMD could then raise/lower based on hardware.
>>>>>>> It is not appropriate to set to 16K by default in ethdev layer.
>>>>>>> Because I don't see any check for the upper bound in some driver,
>>>> like
>>>>>>> axgbe, enetc and so on.
>>>>>>> I'm not sure if they have no upper bound.
>>>>>>> And some driver's maximum buffer size is "16384(16K) - 128"
>>>>>>> So it's better to set to UINT32_MAX by default.
>>>>>>> what do you think?
>>>>>> The goal is always giving application a working upper bound, and
>>>>>> enforcing
>>>>>> that as much as possible in ethdev layer. It doesnt matter which
>>>> pattern
>>>>>> does that.  Fortunately, telling application an incorrect answer
>> is
>>>>>> not fatal.
>>>>>> If over estimated, application pool would be wasting space.
>>>>>> If under estimated, application will get more fragmented packets.
>>>>> I know what you mean.
>>>>> If we set UINT32_MAX, it just means that driver don't report this
>>>> upper
>>>>> bound.
>>>>> This is also a very common way of handling. And it has no effect on
>>>> the
>>>>> drivers that doesn't report this value.
>>>>> On the contrary, if we set a default value (like 16K) in ethdev,
>> user
>>>>> may be misunderstood and confused by that, right?
>>>>> After all, this isn't the real upper bound of all drivers. And this
>>>>> fixed default value may affect the behavior of some driver that I
>>>> didn't
>>>>> find their upper bound.
>>>>> So I'd like to keep it as UINT32_MAX.
>>>>>
>>>>
>>>>
>>>> Hi Stephen, Morten,
>>>>
>>>> I saw scattered Rx mentioned, there may be some misalignment,
>>>> the purpose of the patch is not to enable application to set as big
>> as
>>>> possible mbuf size, so that application can escape from parsing
>>>> multi-segment mbufs.
>>>> Indeed application can provide a large mbuf anyway, to have same
>>>> result,
>>>> without knowing this information.
>>>>
>>>> Main motivation is other way around, device may have restriction on
>>>> buffer size that a single descriptor can address, independent from
>>>> scattered Rx used, if mbuf size is bigger than this device limit,
>> each
>>>> mbuf will have some unused space.
>>>> Patch has intention to inform this max per mbuf/descriptor buffer
>> size,
>>>> so that application doesn't allocate bigger mbuf and waste memory.
>>>
>>> Good point!
>>>
>>> Let's categorize this patch series as a memory optimization for
>> applications that support jumbo frames, but are trying to avoid (or
>> reduce) scattered RX. :-)
>>>
>>
>> It is a memory optimization patch, but again nothing to do with jumbo
>> frames or scattered Rx.
> 
> I expect all NICs to support standard Ethernet frames without scattered RX.
> 
> So I consider this patch related to jumbo frames (and non-scattered RX). Is there any other use case?
> 

I was thinking this is mainly for miss configuration by the application,
but if done intentionally yes intention of the application can be to
receive jumbo frames.

Re: [PATCH v3 0/3] introduce maximum Rx buffer size

[lihuisong (C)]

在 2023/11/3 0:51, Morten Brørup 写道:
>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
>> Sent: Thursday, 2 November 2023 17.24
>>
>> On 11/2/2023 1:59 AM, lihuisong (C) wrote:
>>> 在 2023/11/2 0:08, Stephen Hemminger 写道:
>>>> On Wed, 1 Nov 2023 10:36:07 +0800
>>>> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>>>>
>>>>>> Do we need to report this size? It's a common feature for all
>> PMDs.
>>>>>> It would make sense then to have max_rx_bufsize set to 16K by
>> default
>>>>>> in ethdev, and PMD could then raise/lower based on hardware.
>>>>> It is not appropriate to set to 16K by default in ethdev layer.
>>>>> Because I don't see any check for the upper bound in some driver,
>> like
>>>>> axgbe, enetc and so on.
>>>>> I'm not sure if they have no upper bound.
>>>>> And some driver's maximum buffer size is "16384(16K) - 128"
>>>>> So it's better to set to UINT32_MAX by default.
>>>>> what do you think?
>>>> The goal is always giving application a working upper bound, and
>>>> enforcing
>>>> that as much as possible in ethdev layer. It doesnt matter which
>> pattern
>>>> does that.  Fortunately, telling application an incorrect answer is
>>>> not fatal.
>>>> If over estimated, application pool would be wasting space.
>>>> If under estimated, application will get more fragmented packets.
>>> I know what you mean.
>>> If we set UINT32_MAX, it just means that driver don't report this
>> upper
>>> bound.
>>> This is also a very common way of handling. And it has no effect on
>> the
>>> drivers that doesn't report this value.
>>> On the contrary, if we set a default value (like 16K) in ethdev, user
>>> may be misunderstood and confused by that, right?
>>> After all, this isn't the real upper bound of all drivers. And this
>>> fixed default value may affect the behavior of some driver that I
>> didn't
>>> find their upper bound.
>>> So I'd like to keep it as UINT32_MAX.
>>>
>>
>> Hi Stephen, Morten,
>>
>> I saw scattered Rx mentioned, there may be some misalignment,
>> the purpose of the patch is not to enable application to set as big as
>> possible mbuf size, so that application can escape from parsing
>> multi-segment mbufs.
>> Indeed application can provide a large mbuf anyway, to have same
>> result,
>> without knowing this information.
>>
>> Main motivation is other way around, device may have restriction on
>> buffer size that a single descriptor can address, independent from
>> scattered Rx used, if mbuf size is bigger than this device limit, each
>> mbuf will have some unused space.
>> Patch has intention to inform this max per mbuf/descriptor buffer size,
>> so that application doesn't allocate bigger mbuf and waste memory.
> Good point!
+1
>
> Let's categorize this patch series as a memory optimization for applications that support jumbo frames, but are trying to avoid (or reduce) scattered RX. :-)
User can select to receive jumbo frames in one mbuf or descriptor rather 
then multi-mbuf even if no this patch.
In other words, using big Rx buffer size is just a way to receive jumbo 
frames.
>

Re: [PATCH v4 1/3] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

在 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 <lihuisong@huawei.com>
>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>> ---
>>   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",
>
>
>
>>   	} 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;
> .

Re: [PATCH v4 2/3] app/testpmd: add maximum Rx buffer size display

[lihuisong (C)]

在 2023/11/3 0:42, Ferruh Yigit 写道:
> On 11/2/2023 12:16 PM, Huisong Li wrote:
>> Add maximum Rx buffer size display.
>>
> I think there is a value to show what is the intended usage of this new
> field in application level,
> that is why what do you think to use testpmd?
>
> Testpmd can be updated to check if mbuf data size is bigger than device
> buffer size, and just log, similar to done in ethdev layer, just to
> showcase how application should use it.
Just to check "--mbuf-size" in testpmd cmdline, right?
But we already did it in ethdev layer, is it necessary for testpmd?
Or we can add a usecase in test_ethdev_api.c.
what do you think?
>
> For many drivers, this won't have anything functional, because of
> UINT32_MAX size, but it is still valuable as sample.
>
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>   app/test-pmd/config.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index b9fdb7e8f1..2ac6f15773 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
>>   	}
>>   
>>   	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
>> +	if (dev_info.max_rx_bufsize != UINT32_MAX)
>> +		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
>>   	printf("Maximum configurable length of RX packet: %u\n",
>>   		dev_info.max_rx_pktlen);
>>   	printf("Maximum configurable size of LRO aggregated packet: %u\n",
> .

Re: [PATCH v4 1/3] ethdev: introduce maximum Rx buffer size

[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 <lihuisong@huawei.com>
>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>> ---
>>>   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"

>>
>>
>>
>>>       } 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;
>> .

Re: [PATCH v4 2/3] app/testpmd: add maximum Rx buffer size display

[Ferruh Yigit]

On 11/3/2023 2:39 AM, lihuisong (C) wrote:
> 
> 在 2023/11/3 0:42, Ferruh Yigit 写道:
>> On 11/2/2023 12:16 PM, Huisong Li wrote:
>>> Add maximum Rx buffer size display.
>>>
>> I think there is a value to show what is the intended usage of this new
>> field in application level,
>> that is why what do you think to use testpmd?
>>
>> Testpmd can be updated to check if mbuf data size is bigger than device
>> buffer size, and just log, similar to done in ethdev layer, just to
>> showcase how application should use it.
> Just to check "--mbuf-size" in testpmd cmdline, right?
> But we already did it in ethdev layer, is it necessary for testpmd?
>

What is the expected behavior from the application?

I assumed it should first get the max_rx_bufsize, and if the existing
pktmbuf_pool size bigger than max_rx_bufsize, it can free the mempool
and create a new one with reduced size.

I just thought it can be good to have a sample for this usage, instead
of creating a new mempool, since it will complicate things unnecessary,
testpmd can just log, but yes when it just logs, it will be duplication
of the ethdev.

And I think this is not suitable for the unit test, since this is not
API verification issue.


Thinking twice, lets scratch the request, to not duplicate or increase
complexity more, keep testpmd as it is.


> Or we can add a usecase in test_ethdev_api.c.
> what do you think?
>>
>> For many drivers, this won't have anything functional, because of
>> UINT32_MAX size, but it is still valuable as sample.
>>
>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>> ---
>>>   app/test-pmd/config.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index b9fdb7e8f1..2ac6f15773 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
>>>       }
>>>         printf("Minimum size of RX buffer: %u\n",
>>> dev_info.min_rx_bufsize);
>>> +    if (dev_info.max_rx_bufsize != UINT32_MAX)
>>> +        printf("Maximum size of RX buffer: %u\n",
>>> dev_info.max_rx_bufsize);
>>>       printf("Maximum configurable length of RX packet: %u\n",
>>>           dev_info.max_rx_pktlen);
>>>       printf("Maximum configurable size of LRO aggregated packet: %u\n",
>> .

Re: [PATCH v4 1/3] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

在 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 <lihuisong@huawei.com>
>>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>>> ---
>>>>    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;
>>> .
> .

Re: [PATCH v4 2/3] app/testpmd: add maximum Rx buffer size display

[lihuisong (C)]

在 2023/11/3 11:53, Ferruh Yigit 写道:
> On 11/3/2023 2:39 AM, lihuisong (C) wrote:
>> 在 2023/11/3 0:42, Ferruh Yigit 写道:
>>> On 11/2/2023 12:16 PM, Huisong Li wrote:
>>>> Add maximum Rx buffer size display.
>>>>
>>> I think there is a value to show what is the intended usage of this new
>>> field in application level,
>>> that is why what do you think to use testpmd?
>>>
>>> Testpmd can be updated to check if mbuf data size is bigger than device
>>> buffer size, and just log, similar to done in ethdev layer, just to
>>> showcase how application should use it.
>> Just to check "--mbuf-size" in testpmd cmdline, right?
>> But we already did it in ethdev layer, is it necessary for testpmd?
>>
> What is the expected behavior from the application?
>
> I assumed it should first get the max_rx_bufsize, and if the existing
> pktmbuf_pool size bigger than max_rx_bufsize, it can free the mempool
> and create a new one with reduced size.
>
> I just thought it can be good to have a sample for this usage, instead
> of creating a new mempool, since it will complicate things unnecessary,
> testpmd can just log, but yes when it just logs, it will be duplication
> of the ethdev.
I understand you.
Yeah, it will be duplicate with ethdev.
>
> And I think this is not suitable for the unit test, since this is not
> API verification issue.
Agreed.
>
>
> Thinking twice, lets scratch the request, to not duplicate or increase
> complexity more, keep testpmd as it is.
ok, keep testpmd as it is.
will send next version.
>
>
>> Or we can add a usecase in test_ethdev_api.c.
>> what do you think?
>>> For many drivers, this won't have anything functional, because of
>>> UINT32_MAX size, but it is still valuable as sample.
>>>
>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> ---
>>>>    app/test-pmd/config.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>> index b9fdb7e8f1..2ac6f15773 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
>>>>        }
>>>>          printf("Minimum size of RX buffer: %u\n",
>>>> dev_info.min_rx_bufsize);
>>>> +    if (dev_info.max_rx_bufsize != UINT32_MAX)
>>>> +        printf("Maximum size of RX buffer: %u\n",
>>>> dev_info.max_rx_bufsize);
>>>>        printf("Maximum configurable length of RX packet: %u\n",
>>>>            dev_info.max_rx_pktlen);
>>>>        printf("Maximum configurable size of LRO aggregated packet: %u\n",
>>> .
> .

[PATCH v5 0/3] introduce maximum Rx buffer size

[Huisong Li]

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.

If mbuf data room size in mempool is greater then the maximum Rx buffer
size supported by HW, the data size application used in each mbuf is just
as much as the maximum Rx buffer size supported by HW 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.

---
v5:
 - fix a valiable name
 - fix the log level and context in rte_eth_rx_queue_setup.

v4:
 - fix the log in rte_eth_rx_queue_setup.
 - add a comment in rel_notes.

v3:
 - fix some comments for Morten's advice.

v2:
 - fix some comments
 - fix the log level when data room size is greater than maximum Rx
   buffer size.

v1:
 - move max_rx_bufsize to min_rx_bufsize closer in struct rte_eth_dev_info
 - add max_rx_bufsize display in testpmd.
 - hns3 reports maximum buffer size.

Huisong Li (3):
  ethdev: introduce maximum Rx buffer size
  app/testpmd: add maximum Rx buffer size display
  net/hns3: report maximum buffer size

 app/test-pmd/config.c                  |  2 ++
 doc/guides/rel_notes/release_23_11.rst |  7 +++++++
 drivers/net/hns3/hns3_common.c         |  1 +
 lib/ethdev/rte_ethdev.c                |  8 ++++++++
 lib/ethdev/rte_ethdev.h                | 10 +++++++++-
 5 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH v5 1/3] ethdev: introduce maximum Rx buffer size

[Huisong Li]

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 <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 doc/guides/rel_notes/release_23_11.rst |  7 +++++++
 lib/ethdev/rte_ethdev.c                |  8 ++++++++
 lib/ethdev/rte_ethdev.h                | 10 +++++++++-
 3 files changed, 24 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..24b708f772 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 buf_data_size;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
@@ -2158,6 +2159,12 @@ 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);
+		buf_data_size = mbp_buf_size - RTE_PKTMBUF_HEADROOM;
+		if (buf_data_size > dev_info.max_rx_bufsize)
+			RTE_ETHDEV_LOG(DEBUG,
+				"For port_id=%u, the mbuf data buffer size (%u) is bigger than "
+				"max buffer size (%u) device can utilize, so mbuf size can be reduced.\n",
+				port_id, buf_data_size, dev_info.max_rx_bufsize);
 	} else if (rx_conf != NULL && rx_conf->rx_nseg > 0) {
 		const struct rte_eth_rxseg_split *rx_seg;
 		uint16_t n_seg;
@@ -3757,6 +3764,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;
-- 
2.33.0

[PATCH v5 2/3] app/testpmd: add maximum Rx buffer size display

[Huisong Li]

Add maximum Rx buffer size display.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 app/test-pmd/config.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b9fdb7e8f1..2ac6f15773 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -881,6 +881,8 @@ port_infos_display(portid_t port_id)
 	}
 
 	printf("Minimum size of RX buffer: %u\n", dev_info.min_rx_bufsize);
+	if (dev_info.max_rx_bufsize != UINT32_MAX)
+		printf("Maximum size of RX buffer: %u\n", dev_info.max_rx_bufsize);
 	printf("Maximum configurable length of RX packet: %u\n",
 		dev_info.max_rx_pktlen);
 	printf("Maximum configurable size of LRO aggregated packet: %u\n",
-- 
2.33.0

[PATCH v5 3/3] net/hns3: report maximum buffer size

[Huisong Li]

This patch reports the maximum buffer size hardware supported.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/hns3/hns3_common.c b/drivers/net/hns3/hns3_common.c
index 9327adbdc1..4b0e38cf67 100644
--- a/drivers/net/hns3/hns3_common.c
+++ b/drivers/net/hns3/hns3_common.c
@@ -59,6 +59,7 @@ hns3_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *info)
 	info->max_tx_queues = hw->tqps_num;
 	info->max_rx_pktlen = HNS3_MAX_FRAME_LEN; /* CRC included */
 	info->min_rx_bufsize = HNS3_MIN_BD_BUF_SIZE;
+	info->max_rx_bufsize = HNS3_MAX_BD_BUF_SIZE;
 	info->max_mtu = info->max_rx_pktlen - HNS3_ETH_OVERHEAD;
 	info->max_lro_pkt_size = HNS3_MAX_LRO_SIZE;
 	info->rx_offload_capa = (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM |
-- 
2.33.0

Re: [PATCH v5 0/3] introduce maximum Rx buffer size

[Ferruh Yigit]

On 11/3/2023 10:27 AM, 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.
> 
> If mbuf data room size in mempool is greater then the maximum Rx buffer
> size supported by HW, the data size application used in each mbuf is just
> as much as the maximum Rx buffer size supported by HW 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.
> 
> ---
> v5:
>  - fix a valiable name
>  - fix the log level and context in rte_eth_rx_queue_setup.
> 
> v4:
>  - fix the log in rte_eth_rx_queue_setup.
>  - add a comment in rel_notes.
> 
> v3:
>  - fix some comments for Morten's advice.
> 
> v2:
>  - fix some comments
>  - fix the log level when data room size is greater than maximum Rx
>    buffer size.
> 
> v1:
>  - move max_rx_bufsize to min_rx_bufsize closer in struct rte_eth_dev_info
>  - add max_rx_bufsize display in testpmd.
>  - hns3 reports maximum buffer size.
> 
> Huisong Li (3):
>   ethdev: introduce maximum Rx buffer size
>   app/testpmd: add maximum Rx buffer size display
>   net/hns3: report maximum buffer size
> 

For series,
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>


Series applied to dpdk-next-net/main, thanks.

Re: [PATCH v5 1/3] ethdev: introduce maximum Rx buffer size

[Ivan Malov]

[-- Attachment #1: Type: text/plain, Size: 4858 bytes --]

Hi,

(minor notes below)

On Fri, 3 Nov 2023, 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.

Consider:
So introduce maximum Rx buffer size. It does not enforce anything;
its sole purpose is to inform the user of possible memory waste.

>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> doc/guides/rel_notes/release_23_11.rst |  7 +++++++
> lib/ethdev/rte_ethdev.c                |  8 ++++++++
> lib/ethdev/rte_ethdev.h                | 10 +++++++++-
> 3 files changed, 24 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.**

Consider: Added maximum Rx buffer size reporting.

> +
> +  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..24b708f772 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 buf_data_size;
>
> 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 	dev = &rte_eth_devices[port_id];
> @@ -2158,6 +2159,12 @@ 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);
> +		buf_data_size = mbp_buf_size - RTE_PKTMBUF_HEADROOM;
> +		if (buf_data_size > dev_info.max_rx_bufsize)
> +			RTE_ETHDEV_LOG(DEBUG,
> +				"For port_id=%u, the mbuf data buffer size (%u) is bigger than "

Consider: is larger than

> +				"max buffer size (%u) device can utilize, so mbuf size can be reduced.\n",
> +				port_id, buf_data_size, dev_info.max_rx_bufsize);
> 	} else if (rx_conf != NULL && rx_conf->rx_nseg > 0) {
> 		const struct rte_eth_rxseg_split *rx_seg;
> 		uint16_t n_seg;
> @@ -3757,6 +3764,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.

Consider: The value is not enforced. It's a hint for the application
to optimise mbuf size on mempool allocation. If the driver does not
restrict the maximum buffer size, reported value will be UINT32_MAX.

> +	 */
> +	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;
> -- 
> 2.33.0
>
>
Other than that,

Acked-by: Ivan Malov <ivan.malov@arknetworks.am>

Thank you.