Hi Matan
On 2020/9/6 21:45, Matan Azrad wrote:
>
> Hi Chengchang
>
> From: Chengchang Tang:
>> Hi, Matan
>>
>> On 2020/9/2 18:30, Matan Azrad wrote:
>>> Hi Chengchang
>>>
>>> From: Chengchang Tang
>>>> Hi, Matan
>>>>
>>>> On 2020/9/2 15:19, Matan Azrad wrote:
>>>>>
>>>>> Hi Chengchang
>>>>>
>>>>> From: Chengchang Tang
>>>>>> 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 <tangchengch...@huawei.com>
>>>>>>>> Reviewed-by: Wei Hu (Xavier) <xavier.hu...@huawei.com>
>>>>>>>> Acked-by: Andrew Rybchenko <arybche...@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.
>>>
>>>>> Doesn't it defined by the user? Using Rx queue mem-pool mbuf room
>> size?
>>>>>
>>>>> And it may be different per Rx queue....
>>>>
>>>> Yes, it is defined by user using the Rx queue mem-pool mbuf room size.
>>>> When different queues are bound to different mempools, different
>>>> queues may have different value.
>>>>>
>>>>>> 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.
>>>>>
>>>>> It looks me more like capabilities...
>>>>> The one which define the current configuration is the user by the
>>>> configuration APIs(after reading the capabilities).
>>>>
>>>> I prefer to consider the structure rte_eth_dev_info as the capabilities.
>>>
>>> Yes.
>>>
>>>> Because rxq_info and associated APIs are not available until the
>>>> queue is configured. And the max rx_buf_size is already exists in
>> dev_info.
>>>>>
>>>>> I don't think we have here all the current configurations, so what
>>>>> is special
>>>> in this one?
>>>>
>>>> I think the structure is used to store the queue-related
>>>> configuration, especially the final HW configuration that may be
>>>> different from user configuration and some configurations that are
>>>> not mandatory for the user(PMDs will use a default configuration).
>>>> Such as the scatterred_rx and rx_drop_en in rte_eth_rxconf, some PMDs
>>>> will adjust it in some cases based on their HW constraints.
>>>
>>> Ok, this struct(struct rte_eth_rxq_info) is new for me.
>>> Thanks for the explanation.
>>>
>>>> This configuration item meets the above criteria. The value range of
>>>> rx_buf_size varies according to HW. Some HW may require 1k-alignment,
>>>> while others may require several fixed values. So, the PMDs will
>>>> configure it based on their HW constraints. This results in
>>>> difference between the user configuration and the actual
>>>> configuration and this value affects the memory usage and performance.
>>>> I think there's a need for a way to expose that information.
>>>
>>> So the user can configure X and the driver will use Y!=X?
>>
>> Yes, it depends on the HW. In the queue setup API, it just checks whether
>> the input is greater than the required minimum value. But HW usually has
>> requirements for alignment and so on.
>> So when X does not meet these requirements, PMDs will calculate a new
>> value Y that meets these requirements to configure the hardware (Y <= X, to
>> ensure no memory overflow occurs).
>>> Should the application validate its own configurations after setting them
>> successfully?
>>
>> It depends on their own needs. The application should not be forced to
>> verify it to avoid affecting the ease of use of PMDs. For some applications,
>> they don't care about this value.
>
> I understand,
> It looks me like a bad ping-pong between app and PMD (for all the fields in
> the struct),
> And we should avoid adding fields to this structure if we can.
>
> What's about adding field in rte_eth_dev_info to expose the rx buffer
> alignment supported by the PMD?
> Then, application has all the knowledge you want to expose before the
> configuration.
This may not work because there may be other restrictions besides alignment,
which are
related to the hardware design. Therefore, it is difficult to describe all
constraints
in a single field. Moreover, this approach seems to constrain the PMDs and HW
to some
extent.
>
>>>
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>> 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)...
>>>>>
>>>>> Can you explain more what is the issue you came to solve?
>>>>
>>>> This HW information may be useful when there is some problems running
>>>> a application. This structure and related APIs can be used to expose
>>>> it at run time.
>>>>>
>>> OK
>>>
>>> .
>>>
>
>
> .
>
