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

[Matan Azrad <matan@nvidia.com>]

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

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