diff mbox

[dpdk-dev,2/6] rte_log.h: add detailed log levels RTE_LOG{FINE, FINER, FINEST}

Message ID 1447397258-27233-3-git-send-email-mhall@mhcomputing.net (mailing list archive)
State Rejected, archived
Headers

Commit Message

Matthew Hall Nov. 13, 2015, 6:47 a.m. UTC 
  Signed-off-by: Matthew Hall <mhall@mhcomputing.net>
---
 lib/librte_eal/common/include/rte_log.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Comments

Bruce Richardson Nov. 13, 2015, 11:44 a.m. UTC | #1 
On Fri, Nov 13, 2015 at 06:47:34AM +0000, Matthew Hall wrote:
> Signed-off-by: Matthew Hall <mhall@mhcomputing.net>
> ---
>  lib/librte_eal/common/include/rte_log.h | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index 9dad24e..7dc19e1 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -90,14 +90,17 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOGTYPE_USER8   0x80000000 /**< User-defined log type 8. */
>  
>  /* Can't use 0, as it gives compiler warnings */
> -#define RTE_LOG_EMERG    1U  /**< System is unusable.               */
> -#define RTE_LOG_ALERT    2U  /**< Action must be taken immediately. */
> -#define RTE_LOG_CRIT     3U  /**< Critical conditions.              */
> -#define RTE_LOG_ERR      4U  /**< Error conditions.                 */
> -#define RTE_LOG_WARNING  5U  /**< Warning conditions.               */
> -#define RTE_LOG_NOTICE   6U  /**< Normal but significant condition. */
> -#define RTE_LOG_INFO     7U  /**< Informational.                    */
> -#define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
> +#define RTE_LOG_EMERG     1U  /**< System is unusable.               */
> +#define RTE_LOG_ALERT     2U  /**< Action must be taken immediately. */
> +#define RTE_LOG_CRIT      3U  /**< Critical conditions.              */
> +#define RTE_LOG_ERR       4U  /**< Error conditions.                 */
> +#define RTE_LOG_WARNING   5U  /**< Warning conditions.               */
> +#define RTE_LOG_NOTICE    6U  /**< Normal but significant condition. */
> +#define RTE_LOG_INFO      7U  /**< Informational.                    */
> +#define RTE_LOG_DEBUG     8U  /**< Debug-level messages.             */
> +#define RTE_LOG_FINE      9U  /**< Fine-level messages.              */
> +#define RTE_LOG_FINER    10U  /**< Finer-level messages.             */
> +#define RTE_LOG_FINEST   11U  /**< Finest-level messages.            */
>  
>  /** The default log stream. */
>  extern FILE *eal_default_log_stream;
> -- 
> 1.9.1
> 
Why 11 log levels - it seems an odd number?
Also, not sure about the {fine, finer, finest} names. My thinking would be to
just start numbering them after DEBUG, so RTE_LOG_L9, RTE_LOG_L10 etc., which
would allow us to add on an arbitrary number of extra log levels as needed.

/Bruce
Ananyev, Konstantin Nov. 13, 2015, 11:48 a.m. UTC | #2 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Friday, November 13, 2015 11:44 AM
> To: Matthew Hall
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/6] rte_log.h: add detailed log levels RTE_LOG{FINE, FINER, FINEST}
> 
> On Fri, Nov 13, 2015 at 06:47:34AM +0000, Matthew Hall wrote:
> > Signed-off-by: Matthew Hall <mhall@mhcomputing.net>
> > ---
> >  lib/librte_eal/common/include/rte_log.h | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> > index 9dad24e..7dc19e1 100644
> > --- a/lib/librte_eal/common/include/rte_log.h
> > +++ b/lib/librte_eal/common/include/rte_log.h
> > @@ -90,14 +90,17 @@ extern struct rte_logs rte_logs;
> >  #define RTE_LOGTYPE_USER8   0x80000000 /**< User-defined log type 8. */
> >
> >  /* Can't use 0, as it gives compiler warnings */
> > -#define RTE_LOG_EMERG    1U  /**< System is unusable.               */
> > -#define RTE_LOG_ALERT    2U  /**< Action must be taken immediately. */
> > -#define RTE_LOG_CRIT     3U  /**< Critical conditions.              */
> > -#define RTE_LOG_ERR      4U  /**< Error conditions.                 */
> > -#define RTE_LOG_WARNING  5U  /**< Warning conditions.               */
> > -#define RTE_LOG_NOTICE   6U  /**< Normal but significant condition. */
> > -#define RTE_LOG_INFO     7U  /**< Informational.                    */
> > -#define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
> > +#define RTE_LOG_EMERG     1U  /**< System is unusable.               */
> > +#define RTE_LOG_ALERT     2U  /**< Action must be taken immediately. */
> > +#define RTE_LOG_CRIT      3U  /**< Critical conditions.              */
> > +#define RTE_LOG_ERR       4U  /**< Error conditions.                 */
> > +#define RTE_LOG_WARNING   5U  /**< Warning conditions.               */
> > +#define RTE_LOG_NOTICE    6U  /**< Normal but significant condition. */
> > +#define RTE_LOG_INFO      7U  /**< Informational.                    */
> > +#define RTE_LOG_DEBUG     8U  /**< Debug-level messages.             */
> > +#define RTE_LOG_FINE      9U  /**< Fine-level messages.              */
> > +#define RTE_LOG_FINER    10U  /**< Finer-level messages.             */
> > +#define RTE_LOG_FINEST   11U  /**< Finest-level messages.            */
> >
> >  /** The default log stream. */
> >  extern FILE *eal_default_log_stream;
> > --
> > 1.9.1
> >
> Why 11 log levels - it seems an odd number?
> Also, not sure about the {fine, finer, finest} names. My thinking would be to
> just start numbering them after DEBUG, so RTE_LOG_L9, RTE_LOG_L10 etc., which
> would allow us to add on an arbitrary number of extra log levels as needed.

Actually another question: are existing 8 levels not enough?
Konstantin

 

> 
> /Bruce
Stephen Hemminger Nov. 13, 2015, 4:07 p.m. UTC | #3 
On Fri, 13 Nov 2015 06:47:34 +0000
Matthew Hall <mhall@mhcomputing.net> wrote:

> Signed-off-by: Matthew Hall <mhall@mhcomputing.net>
> ---
>  lib/librte_eal/common/include/rte_log.h | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index 9dad24e..7dc19e1 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -90,14 +90,17 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOGTYPE_USER8   0x80000000 /**< User-defined log type 8. */
>  
>  /* Can't use 0, as it gives compiler warnings */
> -#define RTE_LOG_EMERG    1U  /**< System is unusable.               */
> -#define RTE_LOG_ALERT    2U  /**< Action must be taken immediately. */
> -#define RTE_LOG_CRIT     3U  /**< Critical conditions.              */
> -#define RTE_LOG_ERR      4U  /**< Error conditions.                 */
> -#define RTE_LOG_WARNING  5U  /**< Warning conditions.               */
> -#define RTE_LOG_NOTICE   6U  /**< Normal but significant condition. */
> -#define RTE_LOG_INFO     7U  /**< Informational.                    */
> -#define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
> +#define RTE_LOG_EMERG     1U  /**< System is unusable.               */
> +#define RTE_LOG_ALERT     2U  /**< Action must be taken immediately. */
> +#define RTE_LOG_CRIT      3U  /**< Critical conditions.              */
> +#define RTE_LOG_ERR       4U  /**< Error conditions.                 */
> +#define RTE_LOG_WARNING   5U  /**< Warning conditions.               */
> +#define RTE_LOG_NOTICE    6U  /**< Normal but significant condition. */
> +#define RTE_LOG_INFO      7U  /**< Informational.                    */
> +#define RTE_LOG_DEBUG     8U  /**< Debug-level messages.             */
> +#define RTE_LOG_FINE      9U  /**< Fine-level messages.              */
> +#define RTE_LOG_FINER    10U  /**< Finer-level messages.             */
> +#define RTE_LOG_FINEST   11U  /**< Finest-level messages.            */
>  
>  /** The default log stream. */
>  extern FILE *eal_default_log_stream;

I understand the motivation but the existing levels match syslog
which are what you want for a production application.

The new levels are only for developer logs. I don't think we want all
the developer levels beyond debug in the upstream tree anyway.
Matthew Hall Nov. 13, 2015, 7:14 p.m. UTC | #4 
On Fri, Nov 13, 2015 at 11:48:41AM +0000, Ananyev, Konstantin wrote:
> Actually another question: are existing 8 levels not enough?
> Konstantin

Depends who you ask. I was modeling it based upon the following:

https://docs.oracle.com/javase/7/docs/api/java/util/logging/Level.html

The reason I added a previously-merged patch for a new function 
rte_get_log_level() and all these extra levels is that it allows me to do 
crazy stuff like this:

    if (rte_get_log_level() >= RTE_LOG_FINEST) {
        rte_hexdump(stderr, "header_bytes", packet->header.bytes, packet->header.header_size);
    }

It is extremely valuable to have levels which go below INFO, so you can make 
detailed trace code compiled directly into your app. Which can be readily 
enabled or disabled by setting the log_level. WAY easier to debug the early 
green-field code in your app when it crashes somehow.

Obviously the first thing you guys will say is... "BUT THAT RUNS WAY TOO 
SLOW!!!!!" and you might be right if you are processing 100 gbits/sec. But I 
am using DPDK for security not for SDN switching or core routing. And my patch 
will not introduce any performance change for the guys doing the other stuff 
either.

So if this patch is not accepted in some form, I'd be stuck with my DPDK fork 
just for this feature, and the pre-existing patches I sent to Bruce & Co. 
regarding rte_lpm and rte_lpm6 expansion and so on.

Matthew.
Matthew Hall Nov. 13, 2015, 7:15 p.m. UTC | #5 
On Fri, Nov 13, 2015 at 08:07:41AM -0800, Stephen Hemminger wrote:
> I understand the motivation but the existing levels match syslog
> which are what you want for a production application.
> 
> The new levels are only for developer logs. I don't think we want all
> the developer levels beyond debug in the upstream tree anyway.

I figured everybody would say this.

So then I have to fork DPDK for a trivial aid to coders which doesn't affect 
performance.

This is silly to me.

Matthew.
Matthew Hall Nov. 13, 2015, 7:16 p.m. UTC | #6 
On Fri, Nov 13, 2015 at 11:44:03AM +0000, Bruce Richardson wrote:
> Why 11 log levels - it seems an odd number?
> Also, not sure about the {fine, finer, finest} names. My thinking would be to
> just start numbering them after DEBUG, so RTE_LOG_L9, RTE_LOG_L10 etc., which
> would allow us to add on an arbitrary number of extra log levels as needed.
> 
> /Bruce

I based it on this, it worked very well for me in the past.

https://docs.oracle.com/javase/7/docs/api/java/util/logging/Level.html

But happy to name them anything people want and have any number of them people 
want. I am not religious about the details, just want the feature.

Matthew.
diff mbox

Patch

diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 9dad24e..7dc19e1 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -90,14 +90,17 @@  extern struct rte_logs rte_logs;
 #define RTE_LOGTYPE_USER8   0x80000000 /**< User-defined log type 8. */
 
 /* Can't use 0, as it gives compiler warnings */
-#define RTE_LOG_EMERG    1U  /**< System is unusable.               */
-#define RTE_LOG_ALERT    2U  /**< Action must be taken immediately. */
-#define RTE_LOG_CRIT     3U  /**< Critical conditions.              */
-#define RTE_LOG_ERR      4U  /**< Error conditions.                 */
-#define RTE_LOG_WARNING  5U  /**< Warning conditions.               */
-#define RTE_LOG_NOTICE   6U  /**< Normal but significant condition. */
-#define RTE_LOG_INFO     7U  /**< Informational.                    */
-#define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
+#define RTE_LOG_EMERG     1U  /**< System is unusable.               */
+#define RTE_LOG_ALERT     2U  /**< Action must be taken immediately. */
+#define RTE_LOG_CRIT      3U  /**< Critical conditions.              */
+#define RTE_LOG_ERR       4U  /**< Error conditions.                 */
+#define RTE_LOG_WARNING   5U  /**< Warning conditions.               */
+#define RTE_LOG_NOTICE    6U  /**< Normal but significant condition. */
+#define RTE_LOG_INFO      7U  /**< Informational.                    */
+#define RTE_LOG_DEBUG     8U  /**< Debug-level messages.             */
+#define RTE_LOG_FINE      9U  /**< Fine-level messages.              */
+#define RTE_LOG_FINER    10U  /**< Finer-level messages.             */
+#define RTE_LOG_FINEST   11U  /**< Finest-level messages.            */
 
 /** The default log stream. */
 extern FILE *eal_default_log_stream;