[02/11] log: define logtype register wrapper for drivers
Checks
Commit Message
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>
---
lib/librte_eal/common/include/rte_log.h | 12 ++++++++----
lib/librte_eal/rte_eal_version.map | 8 +++++++-
2 files changed, 15 insertions(+), 5 deletions(-)
Comments
On 8/19/19 2: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>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
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>
> ---
> lib/librte_eal/common/include/rte_log.h | 12 ++++++++----
> lib/librte_eal/rte_eal_version.map | 8 +++++++-
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index cbb4184..c3aff00 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -209,9 +209,6 @@ int rte_log_cur_msg_logtype(void);
> int rte_log_register(const char *name);
>
> /**
> - * @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_log_register() is called inside. If successful, the function tries
> @@ -227,9 +224,16 @@ int rte_log_register(const char *name);
> * - >=0: the newly registered log type
> * - <0: rte_log_register() error value
> */
> -__rte_experimental
> int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def);
+1 to remove experimental from the API.
>
> +#define RTE_LOG_REGISTER(token, name, level, fallback) \
> +RTE_INIT(token##_init) \
Does it still need to be an init time call?
Since it is dynamic now it can be during probe, even log name can be a paramter
to the "struct rte_driver" and log can be registered automatically during probe,
not sure how complex it becomes.
> +{ \
> + 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.
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?
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>
> > ---
> > lib/librte_eal/common/include/rte_log.h | 12 ++++++++----
> > lib/librte_eal/rte_eal_version.map | 8 +++++++-
> > 2 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> > index cbb4184..c3aff00 100644
> > --- a/lib/librte_eal/common/include/rte_log.h
> > +++ b/lib/librte_eal/common/include/rte_log.h
> > @@ -209,9 +209,6 @@ int rte_log_cur_msg_logtype(void);
> > int rte_log_register(const char *name);
> >
> > /**
> > - * @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_log_register() is called inside. If successful, the function tries
> > @@ -227,9 +224,16 @@ int rte_log_register(const char *name);
> > * - >=0: the newly registered log type
> > * - <0: rte_log_register() error value
> > */
> > -__rte_experimental
> > int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def);
>
> +1 to remove experimental from the API.
>
> >
> > +#define RTE_LOG_REGISTER(token, name, level, fallback) \
> > +RTE_INIT(token##_init) \
>
> Does it still need to be an init time call?
> Since it is dynamic now it can be during probe, even log name can be a paramter
> to the "struct rte_driver" and log can be registered automatically during probe,
> not sure how complex it becomes.
This would not work with non driver components built as shared
libraries (unless they have an explicit init symbol in the dpdk init
flow).
The drivers can register multiple log types so this would have to be handled.
We would touch the struct rte_driver which is embedded in other
objects like rte_pci_driver, breaking the abi.
>
> > +{ \
> > + 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?
Yes, I can raise a warning log (using RTE_LOGTYPE_EAL type), since
duplicates are not supposed to happen.
>
> > + 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.
>
> 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.
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).
--
David Marchand
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>
>>> ---
>>> lib/librte_eal/common/include/rte_log.h | 12 ++++++++----
>>> lib/librte_eal/rte_eal_version.map | 8 +++++++-
>>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
>>> index cbb4184..c3aff00 100644
>>> --- a/lib/librte_eal/common/include/rte_log.h
>>> +++ b/lib/librte_eal/common/include/rte_log.h
>>> @@ -209,9 +209,6 @@ int rte_log_cur_msg_logtype(void);
>>> int rte_log_register(const char *name);
>>>
>>> /**
>>> - * @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_log_register() is called inside. If successful, the function tries
>>> @@ -227,9 +224,16 @@ int rte_log_register(const char *name);
>>> * - >=0: the newly registered log type
>>> * - <0: rte_log_register() error value
>>> */
>>> -__rte_experimental
>>> int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def);
>>
>> +1 to remove experimental from the API.
>>
>>>
>>> +#define RTE_LOG_REGISTER(token, name, level, fallback) \
>>> +RTE_INIT(token##_init) \
>>
>> Does it still need to be an init time call?
>> Since it is dynamic now it can be during probe, even log name can be a paramter
>> to the "struct rte_driver" and log can be registered automatically during probe,
>> not sure how complex it becomes.
>
> This would not work with non driver components built as shared
> libraries (unless they have an explicit init symbol in the dpdk init
> flow).
Right.
>
> The drivers can register multiple log types so this would have to be handled.
> We would touch the struct rte_driver which is embedded in other
> objects like rte_pci_driver, breaking the abi.
Yes they may require multiple logs, +abi break, so forget about it.
>
>
>>
>>> +{ \
>>> + 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?
>
> Yes, I can raise a warning log (using RTE_LOGTYPE_EAL type), since
> duplicates are not supposed to happen.
I was checking if we can detect the error from duplication, there can be a
defect it that logic:
Call trace is:
rte_log_register_type_and_pick_level
type = rte_log_register(name);
id = rte_log_lookup(name);
if (id >= 0)
return id
if (type < 0)
return type
"type > 0" for the duplication case but error check only checks if "type < 0"
>
>
>>
>>> + 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.
>
>
>>
>> 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).
There is a gap between 'RTE_LOGTYPE_GSO' & 'RTE_LOGTYPE_USER1' ...
>
>
> --
> David Marchand
>
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.
On 9/4/19 8:45 PM, Thomas Monjalon wrote:
> 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?
See [1]
[1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
04/09/2019 21:21, Andrew Rybchenko:
> On 9/4/19 8:45 PM, Thomas Monjalon wrote:
> > 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?
>
> See [1]
>
> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
Sorry, it does not explain why we mix both operations in one function.
On September 4, 2019 22:42:12 Thomas Monjalon <thomas@monjalon.net> wrote:
> 04/09/2019 21:21, Andrew Rybchenko:
>> On 9/4/19 8:45 PM, Thomas Monjalon wrote:
>>> 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?
>>
>> See [1]
>>
>> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
>
> Sorry, it does not explain why we mix both operations in one function.
Exactly to have one function instead of repeating two function calls
everywhere. Log level should be set when log type is registered. Yes, it is
possible to factor out a function just to pick log level, but I'm not sure
it makes sense separately.
In fact may be it makes sense to substitute just register with this one
(I.e. remove simple register from piblic API and do not highlight that
level is picked up in function name).
04/09/2019 21:58, Andrew Rybchenko:
> On September 4, 2019 22:42:12 Thomas Monjalon <thomas@monjalon.net> wrote:
>
> > 04/09/2019 21:21, Andrew Rybchenko:
> >> On 9/4/19 8:45 PM, Thomas Monjalon wrote:
> >>> 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?
> >>
> >> See [1]
> >>
> >> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
> >
> > Sorry, it does not explain why we mix both operations in one function.
>
> Exactly to have one function instead of repeating two function calls
> everywhere. Log level should be set when log type is registered. Yes, it is
> possible to factor out a function just to pick log level, but I'm not sure
> it makes sense separately.
>
> In fact may be it makes sense to substitute just register with this one
> (I.e. remove simple register from piblic API and do not highlight that
> level is picked up in function name).
Given that we use it in a macro, we could keep two separate functions
for logtype register and minimum log level (note that "minimum" is
currently a missing word in these functions).
Anyway, we will always use the single macro in our libraries.
On 9/4/19 11:44 PM, Thomas Monjalon wrote:
> 04/09/2019 21:58, Andrew Rybchenko:
>> On September 4, 2019 22:42:12 Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>>> 04/09/2019 21:21, Andrew Rybchenko:
>>>> On 9/4/19 8:45 PM, Thomas Monjalon wrote:
>>>>> 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?
>>>> See [1]
>>>>
>>>> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
>>> Sorry, it does not explain why we mix both operations in one function.
>> Exactly to have one function instead of repeating two function calls
>> everywhere. Log level should be set when log type is registered. Yes, it is
>> possible to factor out a function just to pick log level, but I'm not sure
>> it makes sense separately.
>>
>> In fact may be it makes sense to substitute just register with this one
>> (I.e. remove simple register from piblic API and do not highlight that
>> level is picked up in function name).
> Given that we use it in a macro, we could keep two separate functions
> for logtype register and minimum log level (note that "minimum" is
> currently a missing word in these functions).
> Anyway, we will always use the single macro in our libraries.
I don't understand why "minimum" is missing. It assigns level specified
for the pattern if it matches, or use level_def if no match. No level
comparison, last match wins. If "minimum" refers to minimum logged
level, it matches rte_log_set_level() which has no "minimum" as well
in neither name nor description. Moreover, EMERG is smaller than
DEBUG and if we treat log levels as numbers, it sets maximum level
which is logged.
There are two usages right now without a macro, but it is not that
important. What I'm trying to say is that rte_log_register() plus setting
default after register is too error prone. It is the source of the bug
which we tried to workaround introducing the function.
If user sets log level (using eal command-line options), no special
efforts should be required to assign it on register. So, I think register
should always do it. The function name is not ideal since it is highlights
details which are not really interesting. It is logging support details that
log levels may be set before log types registration.
if we accept DPDK-wide default, it makes level_def parameter
unnecessary and the functionality to pick level may be simply
moved to rte_log_register() (using helper internal function) and
I see no good reasons why we really need type-specific defaults.
On Thu, Sep 5, 2019 at 8:29 AM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> On 9/4/19 11:44 PM, Thomas Monjalon wrote:
>
> 04/09/2019 21:58, Andrew Rybchenko:
>
> On September 4, 2019 22:42:12 Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 04/09/2019 21:21, Andrew Rybchenko:
>
> On 9/4/19 8:45 PM, Thomas Monjalon wrote:
>
> 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?
>
> See [1]
>
> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
>
> Sorry, it does not explain why we mix both operations in one function.
>
> Exactly to have one function instead of repeating two function calls
> everywhere. Log level should be set when log type is registered. Yes, it is
> possible to factor out a function just to pick log level, but I'm not sure
> it makes sense separately.
>
> In fact may be it makes sense to substitute just register with this one
> (I.e. remove simple register from piblic API and do not highlight that
> level is picked up in function name).
>
> Given that we use it in a macro, we could keep two separate functions
> for logtype register and minimum log level (note that "minimum" is
> currently a missing word in these functions).
> Anyway, we will always use the single macro in our libraries.
>
>
> I don't understand why "minimum" is missing. It assigns level specified
> for the pattern if it matches, or use level_def if no match. No level
> comparison, last match wins. If "minimum" refers to minimum logged
> level, it matches rte_log_set_level() which has no "minimum" as well
> in neither name nor description. Moreover, EMERG is smaller than
> DEBUG and if we treat log levels as numbers, it sets maximum level
> which is logged.
>
> There are two usages right now without a macro, but it is not that
> important. What I'm trying to say is that rte_log_register() plus setting
> default after register is too error prone. It is the source of the bug
> which we tried to workaround introducing the function.
>
> If user sets log level (using eal command-line options), no special
> efforts should be required to assign it on register. So, I think register
> should always do it. The function name is not ideal since it is highlights
> details which are not really interesting. It is logging support details that
> log levels may be set before log types registration.
>
> if we accept DPDK-wide default, it makes level_def parameter
> unnecessary and the functionality to pick level may be simply
> moved to rte_log_register() (using helper internal function) and
> I see no good reasons why we really need type-specific defaults.
Well this would require to touch quite some components that are not
aligned, but I can't disagree with you :-).
I'll prepare a v2 and identify which drivers are not ready for this.
I also have some additional changes in mind.
This should prevent the log register from failing (and no need for the
"fallback" stuff then).
05/09/2019 08:29, Andrew Rybchenko:
> On 9/4/19 11:44 PM, Thomas Monjalon wrote:
> > 04/09/2019 21:58, Andrew Rybchenko:
> >> On September 4, 2019 22:42:12 Thomas Monjalon <thomas@monjalon.net> wrote:
> >>
> >>> 04/09/2019 21:21, Andrew Rybchenko:
> >>>> On 9/4/19 8:45 PM, Thomas Monjalon wrote:
> >>>>> 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?
> >>>> See [1]
> >>>>
> >>>> [1] http://git.dpdk.org/dpdk/commit/?id=b22e77c026
> >>> Sorry, it does not explain why we mix both operations in one function.
> >> Exactly to have one function instead of repeating two function calls
> >> everywhere. Log level should be set when log type is registered. Yes, it is
> >> possible to factor out a function just to pick log level, but I'm not sure
> >> it makes sense separately.
> >>
> >> In fact may be it makes sense to substitute just register with this one
> >> (I.e. remove simple register from piblic API and do not highlight that
> >> level is picked up in function name).
> > Given that we use it in a macro, we could keep two separate functions
> > for logtype register and minimum log level (note that "minimum" is
> > currently a missing word in these functions).
> > Anyway, we will always use the single macro in our libraries.
>
> I don't understand why "minimum" is missing. It assigns level specified
> for the pattern if it matches, or use level_def if no match. No level
> comparison, last match wins. If "minimum" refers to minimum logged
> level, it matches rte_log_set_level() which has no "minimum" as well
> in neither name nor description. Moreover, EMERG is smaller than
> DEBUG and if we treat log levels as numbers, it sets maximum level
> which is logged.
Yes it is maximum.
I mean even in rte_log_set_level() we don't have such word.
It is a detail.
> There are two usages right now without a macro, but it is not that
> important. What I'm trying to say is that rte_log_register() plus setting
> default after register is too error prone. It is the source of the bug
> which we tried to workaround introducing the function.
>
> If user sets log level (using eal command-line options), no special
> efforts should be required to assign it on register. So, I think register
> should always do it. The function name is not ideal since it is highlights
> details which are not really interesting. It is logging support details that
> log levels may be set before log types registration.
>
> if we accept DPDK-wide default, it makes level_def parameter
> unnecessary and the functionality to pick level may be simply
> moved to rte_log_register() (using helper internal function) and
> I see no good reasons why we really need type-specific defaults.
I agree it can be merged in rte_log_register().
@@ -209,9 +209,6 @@ int rte_log_cur_msg_logtype(void);
int rte_log_register(const char *name);
/**
- * @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_log_register() is called inside. If successful, the function tries
@@ -227,9 +224,16 @@ int rte_log_register(const char *name);
* - >=0: the newly registered log type
* - <0: rte_log_register() error value
*/
-__rte_experimental
int rte_log_register_type_and_pick_level(const char *name, uint32_t level_def);
+#define RTE_LOG_REGISTER(token, name, level, fallback) \
+RTE_INIT(token##_init) \
+{ \
+ token = rte_log_register_type_and_pick_level(name, level); \
+ if (token < 0) \
+ token = fallback; \
+}
+
/**
* Dump log information.
*
@@ -312,6 +312,13 @@ DPDK_19.08 {
} DPDK_19.05;
+DPDK_19.11 {
+ global:
+
+ rte_log_register_type_and_pick_level;
+
+} DPDK_19.08;
+
EXPERIMENTAL {
global:
@@ -342,7 +349,6 @@ EXPERIMENTAL {
rte_fbarray_is_used;
rte_fbarray_set_free;
rte_fbarray_set_used;
- rte_log_register_type_and_pick_level;
rte_malloc_dump_heaps;
rte_mem_alloc_validator_register;
rte_mem_alloc_validator_unregister;