From: Thomas Monjalon <thomas@monjalon.net>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
David Marchand <david.marchand@redhat.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 02/11] log: define logtype register wrapper for drivers
Date: Wed, 04 Sep 2019 19:45:55 +0200 [thread overview]
Message-ID: <3044851.BoT7FmBCyV@xps> (raw)
In-Reply-To: <6fd3bec6-15e4-33ed-4bd2-88d2ecaf6706@intel.com>
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.
next prev parent reply other threads:[~2019-09-04 17:46 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-19 11:41 [dpdk-dev] [PATCH 00/11] Fixing log levels for dynamically loaded drivers David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 01/11] log: fix plugin level restore with patterns David Marchand
2019-08-19 12:30 ` Andrew Rybchenko
2019-08-19 11:41 ` [dpdk-dev] [PATCH 02/11] log: define logtype register wrapper for drivers David Marchand
2019-08-19 12:27 ` Andrew Rybchenko
2019-09-02 14:29 ` Ferruh Yigit
2019-09-03 8:06 ` David Marchand
2019-09-03 8:47 ` Ferruh Yigit
2019-09-04 17:45 ` Thomas Monjalon [this message]
2019-09-04 19:21 ` Andrew Rybchenko
2019-09-04 19:41 ` Thomas Monjalon
2019-09-04 19:58 ` Andrew Rybchenko
2019-09-04 20:44 ` Thomas Monjalon
2019-09-05 6:29 ` Andrew Rybchenko
2019-09-05 7:13 ` David Marchand
2019-09-05 7:45 ` Thomas Monjalon
2019-08-19 11:41 ` [dpdk-dev] [PATCH 03/11] drivers/baseband: use new logtype wrapper David Marchand
2019-08-19 15:39 ` Chautru, Nicolas
2019-08-19 11:41 ` [dpdk-dev] [PATCH 04/11] drivers/bus: " David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 05/11] drivers/common: " David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 06/11] drivers/compress: " David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 07/11] drivers/crypto: " David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 08/11] drivers/event: " David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 09/11] drivers/mempool: " David Marchand
2019-08-19 11:41 ` [dpdk-dev] [PATCH 10/11] drivers/net: " David Marchand
2019-08-19 14:55 ` Legacy, Allain
2019-09-02 16:11 ` Ferruh Yigit
2019-09-03 8:06 ` David Marchand
2019-09-03 15:03 ` Stephen Hemminger
2019-08-19 11:41 ` [dpdk-dev] [PATCH 11/11] drivers/raw: " David Marchand
2019-09-02 14:17 ` [dpdk-dev] [PATCH 00/11] Fixing log levels for dynamically loaded drivers Ferruh Yigit
2019-09-03 8:06 ` David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3044851.BoT7FmBCyV@xps \
--to=thomas@monjalon.net \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).