Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add a field for rxq info structure

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/2/2020 4:52 AM, Chengchang Tang wrote:
> Hi, Matan
> 
> On 2020/9/1 23:33, Matan Azrad wrote:
>>
>> Hi Chengchang
>>
>> Please see some question below.
>>
>> From: Chengchang Tang
>>> Add a field named rx_buf_size in rte_eth_rxq_info to indicate the buffer size
>>> used in receiving packets for HW.
>>>
>>> In this way, upper-layer users can get this information by calling
>>> rte_eth_rx_queue_info_get.
>>>
>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>> Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>  lib/librte_ethdev/rte_ethdev.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index 70295d7..9fed5cb 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -1420,6 +1420,8 @@ struct rte_eth_rxq_info {
>>>         struct rte_eth_rxconf conf; /**< queue config parameters. */
>>>         uint8_t scattered_rx;       /**< scattered packets RX supported. */
>>>         uint16_t nb_desc;           /**< configured number of RXDs. */
>>> +       /**< buffer size used for hardware when receive packets. */
>>> +       uint16_t rx_buf_size;
>>
>> Is it the maximum supported Rx buffer by the HW?
>> If yes, maybe max_rx_buf_size is better name?
> 
> No, it is the Rx buffer size currently used by HW.
> 
> IMHO, the structure rte_eth_rxq_info and associated query API are mainly used to query HW configurations
> at runtime or after queue is configured/setup. Therefore, the content of this structure should be the
> current HW configuration.
> 
>> Maybe document that 0 means - no limitation by HW?
> 
> Yes, there is no need to fill this filed for HW that has no restrictions on it.
> I'll add it in v4.

This field in not for max Rx buffer size, so HW restriction doesn't really apply
here.
But as far as I remember it has been decided that the field will be optional for
PMDs, agree to document that part.

> 
>> Must application read it in order to know if its datapath should handle multi-segment buffers?
> 
> I think it's more appropriate to use scattered_rx to determine if multi-segment buffers should be handled.
> 
>>
>> Maybe it will be good to force application to configure scatter when this field is valid and smaller than max_rx_pkt_len\max_lro.. (<= room size)...
>>
>>
>>>  } __rte_cache_min_aligned;
>>>
>>>  /**
> 
>