Re: [dpdk-dev] [PATCH 02/11] log: define logtype register wrapper for drivers

[Thomas Monjalon <thomas@monjalon.net>]

03/09/2019 10:47, Ferruh Yigit:
> On 9/3/2019 9:06 AM, David Marchand wrote:
> > On Mon, Sep 2, 2019 at 4:29 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >> On 8/19/2019 12:41 PM, David Marchand wrote:
> >>> The function rte_log_register_type_and_pick_level() fills a gap for
> >>> dynamically loaded code (especially drivers) who would not pick up
> >>> the log level passed at startup.
> >>>
> >>> Let's promote it to stable and export it for use by drivers via
> >>> a wrapper.
> >>>
> >>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >>> ---
[...]
> >>>  /**
> >>> - * @warning
> >>> - * @b EXPERIMENTAL: this API may change without prior notice
> >>> - *
> >>>   * Register a dynamic log type and try to pick its level from EAL options
[...]
> >>> -__rte_experimental
> >>>  int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def);
> >>
> >> +1 to remove experimental from the API.

I am not sure about this function API.
Why we combined register and level setting in one function?

> >>> +#define RTE_LOG_REGISTER(token, name, level, fallback) \

You really need to document this macro with doxygen.

> >>> +{ \
> >>> +     token = rte_log_register_type_and_pick_level(name, level); \
> >>> +     if (token < 0) \
> >>
> >> The failure can be because component can try to register existing log name, or
> >> there is no enough memory, do you think does it worth to do log, or some
> >> additional work if component is trying to register existing log name?
[...]
> >>> +             token = fallback; \
> >>
> >> Does the 'fallback' needs to be provided by user, it looks like everyone will
> >> just copy/paste 'RTE_LOGTYPE_PMD' for drivers, and does it really needs to be
> >> configurable since it is fallback.
> > 
> > This series only touches drivers, but I expected other components
> > would use this macro later.
> > I can add a RTE_PMD_REGISTER_LOG macro that hides the RTE_LOGTYPE_PMD
> > fallback value.

I agree we don't need to configure the fallback log.
If there is an error during log setup,
we can log everything next (at debug level).
Let's make fallback hardcoded.

> >> Why not provide a hardcoded type for the failure case? And for that case perhaps
> >> create a more generic logtype, something like "RTE_LOGTYPE_FALLBACK" so that it
> >> can be as it is from all components?
> > 
> > I prefer to map all drivers to a logtype that means something, like
> > RTE_LOGTYPE_PMD.
> 
> In that manner it make sense agreed, but previously drivers were using
> 'RTE_LOGTYPE_PMD' instead of having their own log types, Stephen did some work
> to replace the 'RTE_LOGTYPE_PMD' so that it can be deprecated,
> 
> starting to use it again as fallback may lead drivers using it again as log type
> in their drivers, may they wouldn't but this is what I concern. Something with
> name 'RTE_LOGTYPE_FALLBACK' clear to not use as default logtype in drivers.
> 
> > Having a "fallback" could be used for all components, but this would
> > have to be a static logtype and adding one is not possible without
> > breaking the abi (static entries are < 32 and all values are used).

RTE_LOGTYPE_PMD can be renamed to RTE_LOGTYPE_FALLBACK.

> There is a gap between 'RTE_LOGTYPE_GSO' & 'RTE_LOGTYPE_USER1' ...

Yes, there is room here. But I prefer to rename and re-use
RTE_LOGTYPE_PMD which is not used anymore.
It is part of the EAL API but it is not supposed to be used externally.
For out-of-tree PMDs, we are not supposed to provide such compat.
So I would say don't care with deprecation here.