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

Should the application validate its own configurations after setting them successfully?

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