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

  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(+)
  

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;
  

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