[dpdk-dev] [PATCH] eal:Fix log messages always being printed from rte_eal_cpu_init

[dpdk-dev] [PATCH] eal:Fix log messages always being printed from rte_eal_cpu_init

[Keith Wiles]

The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
even when the log level on the command line was set to INFO or lower.

The problem is the rte_eal_cpu_init() routine was called before
the command line args are scanned. Setting --log-level=7 now
correctly does not print the messages from the rte_eal_cpu_init() routine.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 43 ++++++++++++++++++++++++++++++++++-----
 lib/librte_eal/linuxapp/eal/eal.c | 43 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 76 insertions(+), 10 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 43e8a47..ca10f2c 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -306,6 +306,38 @@ eal_get_hugepage_mem_size(void)
 	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
 }
 
+/* Parse the arguments for --log-level only */
+static void
+eal_log_level_parse(int argc, char **argv)
+{
+	int opt;
+	char **argvopt;
+	int option_index;
+
+	argvopt = argv;
+
+	eal_reset_internal_config(&internal_config);
+
+	while ((opt = getopt_long(argc, argvopt, eal_short_options,
+				  eal_long_options, &option_index)) != EOF) {
+
+		int ret;
+
+		/* getopt is not happy, stop right now */
+		if (opt == '?')
+			break;
+
+		ret = (opt == OPT_LOG_LEVEL_NUM)?
+			eal_parse_common_option(opt, optarg, &internal_config) : 0;
+
+		/* common parser is not happy */
+		if (ret < 0)
+			break;
+	}
+
+	optind = 0; /* reset getopt lib */
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -317,8 +349,6 @@ eal_parse_args(int argc, char **argv)
 
 	argvopt = argv;
 
-	eal_reset_internal_config(&internal_config);
-
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
@@ -447,6 +477,12 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_log_early_init() < 0)
 		rte_panic("Cannot init early logs\n");
 
+	eal_log_level_parse(argc, argv);
+
+	/* set log level as early as possible */
+	rte_set_log_level(internal_config.log_level);
+
+	RTE_LOG(INFO, EAL,  "DPDK Version %s\n", rte_version());
 	if (rte_eal_cpu_init() < 0)
 		rte_panic("Cannot detect lcores\n");
 
@@ -454,9 +490,6 @@ rte_eal_init(int argc, char **argv)
 	if (fctret < 0)
 		exit(1);
 
-	/* set log level as early as possible */
-	rte_set_log_level(internal_config.log_level);
-
 	if (internal_config.no_hugetlbfs == 0 &&
 			internal_config.process_type != RTE_PROC_SECONDARY &&
 			eal_hugepage_info_init() < 0)
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index bd770cf..090ec99 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -499,6 +499,38 @@ eal_get_hugepage_mem_size(void)
 	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
 }
 
+/* Parse the arguments for --log-level only */
+static void
+eal_log_level_parse(int argc, char **argv)
+{
+	int opt;
+	char **argvopt;
+	int option_index;
+
+	argvopt = argv;
+
+	eal_reset_internal_config(&internal_config);
+
+	while ((opt = getopt_long(argc, argvopt, eal_short_options,
+				  eal_long_options, &option_index)) != EOF) {
+
+		int ret;
+
+		/* getopt is not happy, stop right now */
+		if (opt == '?')
+			break;
+
+		ret = (opt == OPT_LOG_LEVEL_NUM)?
+			eal_parse_common_option(opt, optarg, &internal_config) : 0;
+
+		/* common parser is not happy */
+		if (ret < 0)
+			break;
+	}
+
+	optind = 0; /* reset getopt lib */
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -511,8 +543,6 @@ eal_parse_args(int argc, char **argv)
 
 	argvopt = argv;
 
-	eal_reset_internal_config(&internal_config);
-
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
@@ -717,6 +747,12 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_log_early_init() < 0)
 		rte_panic("Cannot init early logs\n");
 
+	eal_log_level_parse(argc, argv);
+
+	/* set log level as early as possible */
+	rte_set_log_level(internal_config.log_level);
+
+	RTE_LOG(INFO, EAL, "DPDK Version %s\n", rte_version());
 	if (rte_eal_cpu_init() < 0)
 		rte_panic("Cannot detect lcores\n");
 
@@ -724,9 +760,6 @@ rte_eal_init(int argc, char **argv)
 	if (fctret < 0)
 		exit(1);
 
-	/* set log level as early as possible */
-	rte_set_log_level(internal_config.log_level);
-
 	if (internal_config.no_hugetlbfs == 0 &&
 			internal_config.process_type != RTE_PROC_SECONDARY &&
 			internal_config.xen_dom0_support == 0 &&
-- 
2.3.0

[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Keith Wiles]

Config files used RTE_LOG_LEVEL=8 to set log level to DEBUG. Using
a the RTE_LOG_XXXX is easier to maintain.

Converted the RTE_LOG_XXXX defines into a enum of values with
the same names for to reduct maintaining the values and allow
debuggers to print the name of the value.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
 config/common_bsdapp                    |  8 +++++++-
 config/common_linuxapp                  |  8 +++++++-
 lib/librte_eal/common/eal_common_log.c  |  4 ++--
 lib/librte_eal/common/include/rte_log.h | 19 +++++++++++--------
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index 0b169c8..97bbcbd 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -93,12 +93,18 @@ CONFIG_RTE_MAX_NUMA_NODES=8
 CONFIG_RTE_MAX_MEMSEG=256
 CONFIG_RTE_MAX_MEMZONE=2560
 CONFIG_RTE_MAX_TAILQ=32
-CONFIG_RTE_LOG_LEVEL=8
 CONFIG_RTE_LOG_HISTORY=256
 CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
 CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
 
 #
+# Log level use: RTE_LOG_XXX
+#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEBUG
+#   Look in rte_log.h for others if any.
+#
+CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
+
+#
 # FreeBSD contiguous memory driver settings
 #
 CONFIG_RTE_CONTIGMEM_MAX_NUM_BUFS=64
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 5deb55a..886fc66 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -93,7 +93,6 @@ CONFIG_RTE_MAX_NUMA_NODES=8
 CONFIG_RTE_MAX_MEMSEG=256
 CONFIG_RTE_MAX_MEMZONE=2560
 CONFIG_RTE_MAX_TAILQ=32
-CONFIG_RTE_LOG_LEVEL=8
 CONFIG_RTE_LOG_HISTORY=256
 CONFIG_RTE_LIBEAL_USE_HPET=n
 CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
@@ -102,6 +101,13 @@ CONFIG_RTE_EAL_IGB_UIO=y
 CONFIG_RTE_EAL_VFIO=y
 
 #
+# Log level use: RTE_LOG_XXX
+#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEBUG
+#   Look in rte_log.h for others if any.
+#
+CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
+
+#
 # Special configurations in PCI Config Space for high performance
 #
 CONFIG_RTE_PCI_CONFIG=n
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index fe3d7d5..3dcceab 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -82,7 +82,7 @@ static struct log_history_list log_history;
 /* global log structure */
 struct rte_logs rte_logs = {
 	.type = ~0,
-	.level = RTE_LOG_DEBUG,
+	.level = RTE_LOG_LEVEL,
 	.file = NULL,
 };
 
@@ -93,7 +93,7 @@ static int history_enabled = 1;
 
 /**
  * This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
  */
 struct log_cur_msg {
 	uint32_t loglevel; /**< log level - see rte_log.h */
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 3b467c1..e7e893e 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -89,14 +89,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.             */
+enum {
+	RTE_LOG_NOOP = 0,   /**< Noop not used (zero entry)        */
+	RTE_LOG_EMERG,      /**< System is unusable.               */
+	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
+	RTE_LOG_CRIT,       /**< Critical conditions.              */
+	RTE_LOG_ERR,        /**< Error conditions.                 */
+	RTE_LOG_WARNING,    /**< Warning conditions.               */
+	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
+	RTE_LOG_INFO,       /**< Informational.                    */
+	RTE_LOG_DEBUG       /**< Debug-level messages.             */
+};
 
 /** The default log stream. */
 extern FILE *eal_default_log_stream;
-- 
2.3.0

Re: [dpdk-dev] [PATCH] eal:Fix log messages always being printed from rte_eal_cpu_init

[Bruce Richardson]

On Sat, Jun 06, 2015 at 07:04:05PM -0500, Keith Wiles wrote:
> The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
> even when the log level on the command line was set to INFO or lower.
> 
> The problem is the rte_eal_cpu_init() routine was called before
> the command line args are scanned. Setting --log-level=7 now
> correctly does not print the messages from the rte_eal_cpu_init() routine.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>

This seems a good idea - make it easy to reduce the verbosity on startup if
so desired. Some comments below.

> ---
>  lib/librte_eal/bsdapp/eal/eal.c   | 43 ++++++++++++++++++++++++++++++++++-----
>  lib/librte_eal/linuxapp/eal/eal.c | 43 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 76 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 43e8a47..ca10f2c 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -306,6 +306,38 @@ eal_get_hugepage_mem_size(void)
>  	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
>  }
>  
> +/* Parse the arguments for --log-level only */
> +static void
> +eal_log_level_parse(int argc, char **argv)
> +{
> +	int opt;
> +	char **argvopt;
> +	int option_index;
> +
> +	argvopt = argv;
> +
> +	eal_reset_internal_config(&internal_config);
> +
> +	while ((opt = getopt_long(argc, argvopt, eal_short_options,
> +				  eal_long_options, &option_index)) != EOF) {
> +
> +		int ret;
> +
> +		/* getopt is not happy, stop right now */
> +		if (opt == '?')
> +			break;
> +
> +		ret = (opt == OPT_LOG_LEVEL_NUM)?
> +			eal_parse_common_option(opt, optarg, &internal_config) : 0;
> +
> +		/* common parser is not happy */
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	optind = 0; /* reset getopt lib */
> +}
> +
>  /* Parse the argument given in the command line of the application */
>  static int
>  eal_parse_args(int argc, char **argv)
> @@ -317,8 +349,6 @@ eal_parse_args(int argc, char **argv)
>  
>  	argvopt = argv;
>  
> -	eal_reset_internal_config(&internal_config);
> -
>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>  				  eal_long_options, &option_index)) != EOF) {
>  
> @@ -447,6 +477,12 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_log_early_init() < 0)
>  		rte_panic("Cannot init early logs\n");
>  
> +	eal_log_level_parse(argc, argv);
> +
> +	/* set log level as early as possible */
> +	rte_set_log_level(internal_config.log_level);
> +
> +	RTE_LOG(INFO, EAL,  "DPDK Version %s\n", rte_version());

There is already the -v option to the EAL to print the DPDK version. Just add
that flag to any command, as it has no other effects. I don't think we need to
increase the verbosity of startup by always printing it.

>  	if (rte_eal_cpu_init() < 0)
>  		rte_panic("Cannot detect lcores\n");
>  
> @@ -454,9 +490,6 @@ rte_eal_init(int argc, char **argv)
>  	if (fctret < 0)
>  		exit(1);
>  
> -	/* set log level as early as possible */
> -	rte_set_log_level(internal_config.log_level);
> -
>  	if (internal_config.no_hugetlbfs == 0 &&
>  			internal_config.process_type != RTE_PROC_SECONDARY &&
>  			eal_hugepage_info_init() < 0)
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index bd770cf..090ec99 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -499,6 +499,38 @@ eal_get_hugepage_mem_size(void)
>  	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
>  }
>  
> +/* Parse the arguments for --log-level only */
> +static void
> +eal_log_level_parse(int argc, char **argv)
> +{
> +	int opt;
> +	char **argvopt;
> +	int option_index;
> +
> +	argvopt = argv;
> +
> +	eal_reset_internal_config(&internal_config);
> +
> +	while ((opt = getopt_long(argc, argvopt, eal_short_options,
> +				  eal_long_options, &option_index)) != EOF) {
> +
> +		int ret;
> +
> +		/* getopt is not happy, stop right now */
> +		if (opt == '?')
> +			break;
> +
> +		ret = (opt == OPT_LOG_LEVEL_NUM)?
> +			eal_parse_common_option(opt, optarg, &internal_config) : 0;
> +
> +		/* common parser is not happy */
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	optind = 0; /* reset getopt lib */
> +}
> +

This function looks duplicated for linux and bsd. Can we move it to one of the
common files instead?

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] eal:Fix log messages always being printed from rte_eal_cpu_init

[Wiles, Keith]

On 6/8/15, 6:09 AM, "Richardson, Bruce" <bruce.richardson@intel.com> wrote:

>On Sat, Jun 06, 2015 at 07:04:05PM -0500, Keith Wiles wrote:
>> The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
>> even when the log level on the command line was set to INFO or lower.
>> 
>> The problem is the rte_eal_cpu_init() routine was called before
>> the command line args are scanned. Setting --log-level=7 now
>> correctly does not print the messages from the rte_eal_cpu_init()
>>routine.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>
>This seems a good idea - make it easy to reduce the verbosity on startup
>if
>so desired. Some comments below.
>
>> ---
>>  lib/librte_eal/bsdapp/eal/eal.c   | 43
>>++++++++++++++++++++++++++++++++++-----
>>  lib/librte_eal/linuxapp/eal/eal.c | 43
>>++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 76 insertions(+), 10 deletions(-)
>> 
>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
>>b/lib/librte_eal/bsdapp/eal/eal.c
>> index 43e8a47..ca10f2c 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal.c
>> @@ -306,6 +306,38 @@ eal_get_hugepage_mem_size(void)
>>  	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
>>  }
>>  
>> +/* Parse the arguments for --log-level only */
>> +static void
>> +eal_log_level_parse(int argc, char **argv)
>> +{
>> +	int opt;
>> +	char **argvopt;
>> +	int option_index;
>> +
>> +	argvopt = argv;
>> +
>> +	eal_reset_internal_config(&internal_config);
>> +
>> +	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>> +				  eal_long_options, &option_index)) != EOF) {
>> +
>> +		int ret;
>> +
>> +		/* getopt is not happy, stop right now */
>> +		if (opt == '?')
>> +			break;
>> +
>> +		ret = (opt == OPT_LOG_LEVEL_NUM)?
>> +			eal_parse_common_option(opt, optarg, &internal_config) : 0;
>> +
>> +		/* common parser is not happy */
>> +		if (ret < 0)
>> +			break;
>> +	}
>> +
>> +	optind = 0; /* reset getopt lib */
>> +}
>> +
>>  /* Parse the argument given in the command line of the application */
>>  static int
>>  eal_parse_args(int argc, char **argv)
>> @@ -317,8 +349,6 @@ eal_parse_args(int argc, char **argv)
>>  
>>  	argvopt = argv;
>>  
>> -	eal_reset_internal_config(&internal_config);
>> -
>>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>>  				  eal_long_options, &option_index)) != EOF) {
>>  
>> @@ -447,6 +477,12 @@ rte_eal_init(int argc, char **argv)
>>  	if (rte_eal_log_early_init() < 0)
>>  		rte_panic("Cannot init early logs\n");
>>  
>> +	eal_log_level_parse(argc, argv);
>> +
>> +	/* set log level as early as possible */
>> +	rte_set_log_level(internal_config.log_level);
>> +
>> +	RTE_LOG(INFO, EAL,  "DPDK Version %s\n", rte_version());
>
>There is already the -v option to the EAL to print the DPDK version. Just
>add
>that flag to any command, as it has no other effects. I don't think we
>need to
>increase the verbosity of startup by always printing it.

OK will remove, but it is one of the things you always need to know when
someone submits the startup messages. This way you do not have to put it
in the email or ask them to tell you.
>
>>  	if (rte_eal_cpu_init() < 0)
>>  		rte_panic("Cannot detect lcores\n");
>>  
>> @@ -454,9 +490,6 @@ rte_eal_init(int argc, char **argv)
>>  	if (fctret < 0)
>>  		exit(1);
>>  
>> -	/* set log level as early as possible */
>> -	rte_set_log_level(internal_config.log_level);
>> -
>>  	if (internal_config.no_hugetlbfs == 0 &&
>>  			internal_config.process_type != RTE_PROC_SECONDARY &&
>>  			eal_hugepage_info_init() < 0)
>> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
>>b/lib/librte_eal/linuxapp/eal/eal.c
>> index bd770cf..090ec99 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -499,6 +499,38 @@ eal_get_hugepage_mem_size(void)
>>  	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
>>  }
>>  
>> +/* Parse the arguments for --log-level only */
>> +static void
>> +eal_log_level_parse(int argc, char **argv)
>> +{
>> +	int opt;
>> +	char **argvopt;
>> +	int option_index;
>> +
>> +	argvopt = argv;
>> +
>> +	eal_reset_internal_config(&internal_config);
>> +
>> +	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>> +				  eal_long_options, &option_index)) != EOF) {
>> +
>> +		int ret;
>> +
>> +		/* getopt is not happy, stop right now */
>> +		if (opt == '?')
>> +			break;
>> +
>> +		ret = (opt == OPT_LOG_LEVEL_NUM)?
>> +			eal_parse_common_option(opt, optarg, &internal_config) : 0;
>> +
>> +		/* common parser is not happy */
>> +		if (ret < 0)
>> +			break;
>> +	}
>> +
>> +	optind = 0; /* reset getopt lib */
>> +}
>> +
>
>This function looks duplicated for linux and bsd. Can we move it to one
>of the
>common files instead?

I looked at moving this function to the common location, but the problem
is all of the routines of this type are all static functions. I did not
want to add yet another global function to the mix. I will look at it and
see what I can do.
>
>Regards,
>/Bruce

Re: [dpdk-dev] [PATCH] eal:Fix log messages always being printed from rte_eal_cpu_init

[Wiles, Keith]

On 6/8/15, 8:33 AM, "Wiles, Keith" <keith.wiles@intel.com> wrote:

>
>
>On 6/8/15, 6:09 AM, "Richardson, Bruce" <bruce.richardson@intel.com>
>wrote:
>
>>On Sat, Jun 06, 2015 at 07:04:05PM -0500, Keith Wiles wrote:
>>> The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
>>> even when the log level on the command line was set to INFO or lower.
>>> 
>>> The problem is the rte_eal_cpu_init() routine was called before
>>> the command line args are scanned. Setting --log-level=7 now
>>> correctly does not print the messages from the rte_eal_cpu_init()
>>>routine.
>>> 
>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>
>>This seems a good idea - make it easy to reduce the verbosity on startup
>>if
>>so desired. Some comments below.
>>
>>> ---
>>>  lib/librte_eal/bsdapp/eal/eal.c   | 43
>>>++++++++++++++++++++++++++++++++++-----
>>>  lib/librte_eal/linuxapp/eal/eal.c | 43
>>>++++++++++++++++++++++++++++++++++-----
>>>  2 files changed, 76 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
>>>b/lib/librte_eal/bsdapp/eal/eal.c
>>> index 43e8a47..ca10f2c 100644
>>> --- a/lib/librte_eal/bsdapp/eal/eal.c
>>> +++ b/lib/librte_eal/bsdapp/eal/eal.c
>>> @@ -306,6 +306,38 @@ eal_get_hugepage_mem_size(void)
>>>  	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
>>>  }
>>>  
>>> +/* Parse the arguments for --log-level only */
>>> +static void
>>> +eal_log_level_parse(int argc, char **argv)
>>> +{
>>> +	int opt;
>>> +	char **argvopt;
>>> +	int option_index;
>>> +
>>> +	argvopt = argv;
>>> +
>>> +	eal_reset_internal_config(&internal_config);
>>> +
>>> +	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>>> +				  eal_long_options, &option_index)) != EOF) {
>>> +
>>> +		int ret;
>>> +
>>> +		/* getopt is not happy, stop right now */
>>> +		if (opt == '?')
>>> +			break;
>>> +
>>> +		ret = (opt == OPT_LOG_LEVEL_NUM)?
>>> +			eal_parse_common_option(opt, optarg, &internal_config) : 0;
>>> +
>>> +		/* common parser is not happy */
>>> +		if (ret < 0)
>>> +			break;
>>> +	}
>>> +
>>> +	optind = 0; /* reset getopt lib */
>>> +}
>>> +
>>>  /* Parse the argument given in the command line of the application */
>>>  static int
>>>  eal_parse_args(int argc, char **argv)
>>> @@ -317,8 +349,6 @@ eal_parse_args(int argc, char **argv)
>>>  
>>>  	argvopt = argv;
>>>  
>>> -	eal_reset_internal_config(&internal_config);
>>> -
>>>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>>>  				  eal_long_options, &option_index)) != EOF) {
>>>  
>>> @@ -447,6 +477,12 @@ rte_eal_init(int argc, char **argv)
>>>  	if (rte_eal_log_early_init() < 0)
>>>  		rte_panic("Cannot init early logs\n");
>>>  
>>> +	eal_log_level_parse(argc, argv);
>>> +
>>> +	/* set log level as early as possible */
>>> +	rte_set_log_level(internal_config.log_level);
>>> +
>>> +	RTE_LOG(INFO, EAL,  "DPDK Version %s\n", rte_version());
>>
>>There is already the -v option to the EAL to print the DPDK version. Just
>>add
>>that flag to any command, as it has no other effects. I don't think we
>>need to
>>increase the verbosity of startup by always printing it.
>
>OK will remove, but it is one of the things you always need to know when
>someone submits the startup messages. This way you do not have to put it
>in the email or ask them to tell you.
>>
>>>  	if (rte_eal_cpu_init() < 0)
>>>  		rte_panic("Cannot detect lcores\n");
>>>  
>>> @@ -454,9 +490,6 @@ rte_eal_init(int argc, char **argv)
>>>  	if (fctret < 0)
>>>  		exit(1);
>>>  
>>> -	/* set log level as early as possible */
>>> -	rte_set_log_level(internal_config.log_level);
>>> -
>>>  	if (internal_config.no_hugetlbfs == 0 &&
>>>  			internal_config.process_type != RTE_PROC_SECONDARY &&
>>>  			eal_hugepage_info_init() < 0)
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
>>>b/lib/librte_eal/linuxapp/eal/eal.c
>>> index bd770cf..090ec99 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>>> @@ -499,6 +499,38 @@ eal_get_hugepage_mem_size(void)
>>>  	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
>>>  }
>>>  
>>> +/* Parse the arguments for --log-level only */
>>> +static void
>>> +eal_log_level_parse(int argc, char **argv)
>>> +{
>>> +	int opt;
>>> +	char **argvopt;
>>> +	int option_index;
>>> +
>>> +	argvopt = argv;
>>> +
>>> +	eal_reset_internal_config(&internal_config);
>>> +
>>> +	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>>> +				  eal_long_options, &option_index)) != EOF) {
>>> +
>>> +		int ret;
>>> +
>>> +		/* getopt is not happy, stop right now */
>>> +		if (opt == '?')
>>> +			break;
>>> +
>>> +		ret = (opt == OPT_LOG_LEVEL_NUM)?
>>> +			eal_parse_common_option(opt, optarg, &internal_config) : 0;
>>> +
>>> +		/* common parser is not happy */
>>> +		if (ret < 0)
>>> +			break;
>>> +	}
>>> +
>>> +	optind = 0; /* reset getopt lib */
>>> +}
>>> +
>>
>>This function looks duplicated for linux and bsd. Can we move it to one
>>of the
>>common files instead?
>
>I looked at moving this function to the common location, but the problem
>is all of the routines of this type are all static functions. I did not
>want to add yet another global function to the mix. I will look at it and
>see what I can do.

Well it looks like a fair number of the functions in eal.c are common
between both versions. I could try to move them all or just leave the code
as is. What do you want here?
>>
>>Regards,
>>/Bruce
>

[dpdk-dev] [PATCH v2] eal:Fix log messages always being printed from rte_eal_cpu_init

[Keith Wiles]

The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
even when the log level on the command line was set to INFO or lower.

The problem is the rte_eal_cpu_init() routine was called before
the command line args are scanned. Setting --log-level=7 now
correctly does not print the messages from the rte_eal_cpu_init() routine.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 42 ++++++++++++++++++++++++++++++++++-----
 lib/librte_eal/linuxapp/eal/eal.c | 42 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 43e8a47..0112617 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -306,6 +306,38 @@ eal_get_hugepage_mem_size(void)
 	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
 }
 
+/* Parse the arguments for --log-level only */
+static void
+eal_log_level_parse(int argc, char **argv)
+{
+	int opt;
+	char **argvopt;
+	int option_index;
+
+	argvopt = argv;
+
+	eal_reset_internal_config(&internal_config);
+
+	while ((opt = getopt_long(argc, argvopt, eal_short_options,
+				  eal_long_options, &option_index)) != EOF) {
+
+		int ret;
+
+		/* getopt is not happy, stop right now */
+		if (opt == '?')
+			break;
+
+		ret = (opt == OPT_LOG_LEVEL_NUM)?
+			eal_parse_common_option(opt, optarg, &internal_config) : 0;
+
+		/* common parser is not happy */
+		if (ret < 0)
+			break;
+	}
+
+	optind = 0; /* reset getopt lib */
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -317,8 +349,6 @@ eal_parse_args(int argc, char **argv)
 
 	argvopt = argv;
 
-	eal_reset_internal_config(&internal_config);
-
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
@@ -447,6 +477,11 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_log_early_init() < 0)
 		rte_panic("Cannot init early logs\n");
 
+	eal_log_level_parse(argc, argv);
+
+	/* set log level as early as possible */
+	rte_set_log_level(internal_config.log_level);
+
 	if (rte_eal_cpu_init() < 0)
 		rte_panic("Cannot detect lcores\n");
 
@@ -454,9 +489,6 @@ rte_eal_init(int argc, char **argv)
 	if (fctret < 0)
 		exit(1);
 
-	/* set log level as early as possible */
-	rte_set_log_level(internal_config.log_level);
-
 	if (internal_config.no_hugetlbfs == 0 &&
 			internal_config.process_type != RTE_PROC_SECONDARY &&
 			eal_hugepage_info_init() < 0)
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index bd770cf..4f8d0d9 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -499,6 +499,38 @@ eal_get_hugepage_mem_size(void)
 	return (size < SIZE_MAX) ? (size_t)(size) : SIZE_MAX;
 }
 
+/* Parse the arguments for --log-level only */
+static void
+eal_log_level_parse(int argc, char **argv)
+{
+	int opt;
+	char **argvopt;
+	int option_index;
+
+	argvopt = argv;
+
+	eal_reset_internal_config(&internal_config);
+
+	while ((opt = getopt_long(argc, argvopt, eal_short_options,
+				  eal_long_options, &option_index)) != EOF) {
+
+		int ret;
+
+		/* getopt is not happy, stop right now */
+		if (opt == '?')
+			break;
+
+		ret = (opt == OPT_LOG_LEVEL_NUM)?
+			eal_parse_common_option(opt, optarg, &internal_config) : 0;
+
+		/* common parser is not happy */
+		if (ret < 0)
+			break;
+	}
+
+	optind = 0; /* reset getopt lib */
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -511,8 +543,6 @@ eal_parse_args(int argc, char **argv)
 
 	argvopt = argv;
 
-	eal_reset_internal_config(&internal_config);
-
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
@@ -717,6 +747,11 @@ rte_eal_init(int argc, char **argv)
 	if (rte_eal_log_early_init() < 0)
 		rte_panic("Cannot init early logs\n");
 
+	eal_log_level_parse(argc, argv);
+
+	/* set log level as early as possible */
+	rte_set_log_level(internal_config.log_level);
+
 	if (rte_eal_cpu_init() < 0)
 		rte_panic("Cannot detect lcores\n");
 
@@ -724,9 +759,6 @@ rte_eal_init(int argc, char **argv)
 	if (fctret < 0)
 		exit(1);
 
-	/* set log level as early as possible */
-	rte_set_log_level(internal_config.log_level);
-
 	if (internal_config.no_hugetlbfs == 0 &&
 			internal_config.process_type != RTE_PROC_SECONDARY &&
 			internal_config.xen_dom0_support == 0 &&
-- 
2.3.0

Re: [dpdk-dev] [PATCH v2] eal:Fix log messages always being printed from rte_eal_cpu_init

[David Marchand]

Hello Keith,

On Mon, Jun 8, 2015 at 11:55 PM, Keith Wiles <keith.wiles@intel.com> wrote:

> The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
> even when the log level on the command line was set to INFO or lower.
>
> The problem is the rte_eal_cpu_init() routine was called before
> the command line args are scanned. Setting --log-level=7 now
> correctly does not print the messages from the rte_eal_cpu_init() routine.
>
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c   | 42
> ++++++++++++++++++++++++++++++++++-----
>  lib/librte_eal/linuxapp/eal/eal.c | 42
> ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 74 insertions(+), 10 deletions(-)
>

We could have a different solution, but this patch is the quickest answer
to the described problem.
Please, fix the checkpatch error / warnings, then, ack.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] eal:Fix log messages always being printed from rte_eal_cpu_init

[Thomas Monjalon]

2015-06-19 11:54, David Marchand:
> On Mon, Jun 8, 2015 at 11:55 PM, Keith Wiles <keith.wiles@intel.com> wrote:
> > The RTE_LOG(DEBUG, ...) messages in rte_eal_cpu_init() are printed
> > even when the log level on the command line was set to INFO or lower.
> >
> > The problem is the rte_eal_cpu_init() routine was called before
> > the command line args are scanned. Setting --log-level=7 now
> > correctly does not print the messages from the rte_eal_cpu_init() routine.
> >
> > Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> 
> We could have a different solution, but this patch is the quickest answer
> to the described problem.
> Please, fix the checkpatch error / warnings, then, ack.

I fixed the checkpatch warnings.

Applied, thanks

I would prefer avoiding such copy/paste for bsdapp and linuxapp.
Ravi Kerur worked on factorizing EAL code but we never found the right time
to apply them and there are some concerns about the organizations of some
code areas. I suggest to try to apply these ideas just after 2.1.0-rc1, when
most of the pending EAL patches will be merged.
We'll have to do it piece by piece to avoid a "big review" effect.

Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Thomas Monjalon]

2015-06-06 19:04, Keith Wiles:
> --- a/config/common_bsdapp
> +++ b/config/common_bsdapp
> @@ -93,12 +93,18 @@ CONFIG_RTE_MAX_NUMA_NODES=8
>  CONFIG_RTE_MAX_MEMSEG=256
>  CONFIG_RTE_MAX_MEMZONE=2560
>  CONFIG_RTE_MAX_TAILQ=32
> -CONFIG_RTE_LOG_LEVEL=8
>  CONFIG_RTE_LOG_HISTORY=256
>  CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>  CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
>  
>  #
> +# Log level use: RTE_LOG_XXX
> +#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEBUG
> +#   Look in rte_log.h for others if any.
> +#

I think this comment is useless.

> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG

Yes, easier to read.
Please do not move line without good reason. It was more logic to see it along
with LOG_HISTORY.

> --- a/lib/librte_eal/common/eal_common_log.c
> +++ b/lib/librte_eal/common/eal_common_log.c
> @@ -82,7 +82,7 @@ static struct log_history_list log_history;
>  /* global log structure */
>  struct rte_logs rte_logs = {
>  	.type = ~0,
> -	.level = RTE_LOG_DEBUG,
> +	.level = RTE_LOG_LEVEL,
>  	.file = NULL,
>  };

OK, more consistent.
It was set to RTE_LOG_LEVEL later anyway.
(this comment would be useful in the commit message)

>  /* 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.             */
> +enum {
> +	RTE_LOG_NOOP = 0,   /**< Noop not used (zero entry)        */

NOOP is useless: EMERG may be = 1

> +	RTE_LOG_EMERG,      /**< System is unusable.               */
> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
> +	RTE_LOG_ERR,        /**< Error conditions.                 */
> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
> +	RTE_LOG_INFO,       /**< Informational.                    */
> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
> +};

What is the benefit of this change?

Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Wiles, Keith]

On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>2015-06-06 19:04, Keith Wiles:
>> --- a/config/common_bsdapp
>> +++ b/config/common_bsdapp
>> @@ -93,12 +93,18 @@ CONFIG_RTE_MAX_NUMA_NODES=8
>>  CONFIG_RTE_MAX_MEMSEG=256
>>  CONFIG_RTE_MAX_MEMZONE=2560
>>  CONFIG_RTE_MAX_TAILQ=32
>> -CONFIG_RTE_LOG_LEVEL=8
>>  CONFIG_RTE_LOG_HISTORY=256
>>  CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>>  CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
>>  
>>  #
>> +# Log level use: RTE_LOG_XXX
>> +#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEBUG
>> +#   Look in rte_log.h for others if any.
>> +#
>
>I think this comment is useless.

I do not think the comment is useless as some may not understand what
values the Log level can be set too in the future. Not commenting the
change would be a problem IMO. This is also why the line was moved.
>
>> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
>
>Yes, easier to read.
>Please do not move line without good reason. It was more logic to see it
>along
>with LOG_HISTORY.

Moving the line was for the comment and now it is a enum value instead of
a magic number. Magic numbers are bad right? Adding a comment to help the
user set this value is always reasonable IMO unless the comment is not
correct, is this the case?
>
>> --- a/lib/librte_eal/common/eal_common_log.c
>> +++ b/lib/librte_eal/common/eal_common_log.c
>> @@ -82,7 +82,7 @@ static struct log_history_list log_history;
>>  /* global log structure */
>>  struct rte_logs rte_logs = {
>>  	.type = ~0,
>> -	.level = RTE_LOG_DEBUG,
>> +	.level = RTE_LOG_LEVEL,
>>  	.file = NULL,
>>  };
>
>OK, more consistent.
>It was set to RTE_LOG_LEVEL later anyway.
>(this comment would be useful in the commit message)
>
>>  /* 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.             */
>> +enum {
>> +	RTE_LOG_NOOP = 0,   /**< Noop not used (zero entry)        */
>
>NOOP is useless: EMERG may be = 1

Does it really matter if I used RTE_LOG_NOOP, just to make sure someone
did not try to use zero here. Instead of setting the RTE_LOG_EMERG=1, I
can change it to be the way you suggest, but I think it does not hurt
anything does it?
>
>> +	RTE_LOG_EMERG,      /**< System is unusable.               */
>> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
>> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
>> +	RTE_LOG_ERR,        /**< Error conditions.                 */
>> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
>> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
>> +	RTE_LOG_INFO,       /**< Informational.                    */
>> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
>> +};
>
>What is the benefit of this change?

The change is to use a enum in place of using magic numbers, plus you get
the benefit of seeing the enum name in the debugger instead of a number.
It makes the code more readable IMHO.
>
>

To me the code is fine and the only change would be the RTE_LOG_NOOP being
remove and RTE_LOG_EMERG=1.

‹ 
Regards,
++Keith
Intel Corporation

Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Wiles, Keith]

On 8/2/15, 2:10 PM, "dev on behalf of Wiles, Keith" <dev-bounces@dpdk.org
on behalf of keith.wiles@intel.com> wrote:

>On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
>
>>2015-06-06 19:04, Keith Wiles:
>>> --- a/config/common_bsdapp
>>> +++ b/config/common_bsdapp
>>> @@ -93,12 +93,18 @@ CONFIG_RTE_MAX_NUMA_NODES=8
>>>  CONFIG_RTE_MAX_MEMSEG=256
>>>  CONFIG_RTE_MAX_MEMZONE=2560
>>>  CONFIG_RTE_MAX_TAILQ=32
>>> -CONFIG_RTE_LOG_LEVEL=8
>>>  CONFIG_RTE_LOG_HISTORY=256
>>>  CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>>>  CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
>>>  
>>>  #
>>> +# Log level use: RTE_LOG_XXX
>>> +#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or
>>>DEBUG
>>> +#   Look in rte_log.h for others if any.
>>> +#
>>
>>I think this comment is useless.
>
>I do not think the comment is useless as some may not understand what
>values the Log level can be set too in the future. Not commenting the
>change would be a problem IMO. This is also why the line was moved.
>>
>>> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
>>
>>Yes, easier to read.
>>Please do not move line without good reason. It was more logic to see it
>>along
>>with LOG_HISTORY.

Maybe moving LOG_HISTORY with LOG_LEVEL would have been a better option.
>
>Moving the line was for the comment and now it is a enum value instead of
>a magic number. Magic numbers are bad right? Adding a comment to help the
>user set this value is always reasonable IMO unless the comment is not
>correct, is this the case?
>>
>>> --- a/lib/librte_eal/common/eal_common_log.c
>>> +++ b/lib/librte_eal/common/eal_common_log.c
>>> @@ -82,7 +82,7 @@ static struct log_history_list log_history;
>>>  /* global log structure */
>>>  struct rte_logs rte_logs = {
>>>  	.type = ~0,
>>> -	.level = RTE_LOG_DEBUG,
>>> +	.level = RTE_LOG_LEVEL,
>>>  	.file = NULL,
>>>  };
>>
>>OK, more consistent.
>>It was set to RTE_LOG_LEVEL later anyway.
>>(this comment would be useful in the commit message)
>>
>>>  /* 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.             */
>>> +enum {
>>> +	RTE_LOG_NOOP = 0,   /**< Noop not used (zero entry)        */
>>
>>NOOP is useless: EMERG may be = 1
>
>Does it really matter if I used RTE_LOG_NOOP, just to make sure someone
>did not try to use zero here. Instead of setting the RTE_LOG_EMERG=1, I
>can change it to be the way you suggest, but I think it does not hurt
>anything does it?
>>
>>> +	RTE_LOG_EMERG,      /**< System is unusable.               */
>>> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
>>> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
>>> +	RTE_LOG_ERR,        /**< Error conditions.                 */
>>> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
>>> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
>>> +	RTE_LOG_INFO,       /**< Informational.                    */
>>> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
>>> +};
>>
>>What is the benefit of this change?
>
>The change is to use a enum in place of using magic numbers, plus you get
>the benefit of seeing the enum name in the debugger instead of a number.
>It makes the code more readable IMHO.
>>
>>
>
>To me the code is fine and the only change would be the RTE_LOG_NOOP being
>remove and RTE_LOG_EMERG=1.
>
>‹ 
>Regards,
>++Keith
>Intel Corporation
>
>
>
>


— 
Regards,
++Keith
Intel Corporation

Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Thomas Monjalon]

2015-08-02 19:10, Wiles, Keith:
> On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
> >2015-06-06 19:04, Keith Wiles:
> >> +# Log level use: RTE_LOG_XXX
> >> +#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or DEBUG
> >> +#   Look in rte_log.h for others if any.
> >> +#
> >
> >I think this comment is useless.
> 
> I do not think the comment is useless as some may not understand what
> values the Log level can be set too in the future. Not commenting the
> change would be a problem IMO. This is also why the line was moved.

It is already documented in the API doc.
I agree having some comments in the config files would be convenient but:
	- this one is 3 lines long
	- currently comments are only used to separate sections
Maybe you can do a oneline:
	# Minimum log level compiled: DEBUG, INFO, NOTICE, WARNING or ERR
I think it is important to tell it is a minimum log level, i.e. compiled logs.
And probably it is not needed to suggest a minimum level higher than ERR.

> >> +enum {
> >> +	RTE_LOG_NOOP = 0,   /**< Noop not used (zero entry)        */
> >
> >NOOP is useless: EMERG may be = 1
> 
> Does it really matter if I used RTE_LOG_NOOP, just to make sure someone
> did not try to use zero here. Instead of setting the RTE_LOG_EMERG=1, I
> can change it to be the way you suggest, but I think it does not hurt
> anything does it?

We avoid adding code without real use which could cause confusion.

> >> +	RTE_LOG_EMERG,      /**< System is unusable.               */
> >> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
> >> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
> >> +	RTE_LOG_ERR,        /**< Error conditions.                 */
> >> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
> >> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
> >> +	RTE_LOG_INFO,       /**< Informational.                    */
> >> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
> >> +};
> >
> >What is the benefit of this change?
> 
> The change is to use a enum in place of using magic numbers, plus you get
> the benefit of seeing the enum name in the debugger instead of a number.
> It makes the code more readable IMHO.

OK so a comment in the commit message could give the debugger justification.

Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Wiles, Keith]

On 8/2/15, 3:44 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>2015-08-02 19:10, Wiles, Keith:
>> On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com>
>>wrote:
>> >2015-06-06 19:04, Keith Wiles:
>> >> +# Log level use: RTE_LOG_XXX
>> >> +#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or
>>DEBUG
>> >> +#   Look in rte_log.h for others if any.
>> >> +#
>> >
>> >I think this comment is useless.
>> 
>> I do not think the comment is useless as some may not understand what
>> values the Log level can be set too in the future. Not commenting the
>> change would be a problem IMO. This is also why the line was moved.
>
>It is already documented in the API doc.
>I agree having some comments in the config files would be convenient but:
>	- this one is 3 lines long
>	- currently comments are only used to separate sections
>Maybe you can do a oneline:
>	# Minimum log level compiled: DEBUG, INFO, NOTICE, WARNING or ERR
>I think it is important to tell it is a minimum log level, i.e. compiled
>logs.
>And probably it is not needed to suggest a minimum level higher than ERR.

I will reduce the comment to one line, should I move the LOG_HISTORY down
to under LOG_LEVEL?
>
>> >> +enum {
>> >> +	RTE_LOG_NOOP = 0,   /**< Noop not used (zero entry)        */
>> >
>> >NOOP is useless: EMERG may be = 1
>> 
>> Does it really matter if I used RTE_LOG_NOOP, just to make sure someone
>> did not try to use zero here. Instead of setting the RTE_LOG_EMERG=1, I
>> can change it to be the way you suggest, but I think it does not hurt
>> anything does it?
>
>We avoid adding code without real use which could cause confusion.

I will remove NOOP and set EMERG=1.
>
>> >> +	RTE_LOG_EMERG,      /**< System is unusable.               */
>> >> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
>> >> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
>> >> +	RTE_LOG_ERR,        /**< Error conditions.                 */
>> >> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
>> >> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
>> >> +	RTE_LOG_INFO,       /**< Informational.                    */
>> >> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
>> >> +};
>> >
>> >What is the benefit of this change?
>> 
>> The change is to use a enum in place of using magic numbers, plus you
>>get
>> the benefit of seeing the enum name in the debugger instead of a number.
>> It makes the code more readable IMHO.
>
>OK so a comment in the commit message could give the debugger
>justification.

OK will add the debugger comment to the commit log.
>
>


‹ 
Regards,
++Keith
Intel Corporation

Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define

[Thomas Monjalon]

2015-08-02 20:58, Wiles, Keith:
> On 8/2/15, 3:44 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
> 
> >2015-08-02 19:10, Wiles, Keith:
> >> On 8/2/15, 12:15 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com>
> >>wrote:
> >> >2015-06-06 19:04, Keith Wiles:
> >> >> +# Log level use: RTE_LOG_XXX
> >> >> +#   XXX = NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or
> >>DEBUG
> >> >> +#   Look in rte_log.h for others if any.
> >> >> +#
> >> >
> >> >I think this comment is useless.
> >> 
> >> I do not think the comment is useless as some may not understand what
> >> values the Log level can be set too in the future. Not commenting the
> >> change would be a problem IMO. This is also why the line was moved.
> >
> >It is already documented in the API doc.
> >I agree having some comments in the config files would be convenient but:
> >	- this one is 3 lines long
> >	- currently comments are only used to separate sections
> >Maybe you can do a oneline:
> >	# Minimum log level compiled: DEBUG, INFO, NOTICE, WARNING or ERR
> >I think it is important to tell it is a minimum log level, i.e. compiled
> >logs.
> >And probably it is not needed to suggest a minimum level higher than ERR.
> 
> I will reduce the comment to one line, should I move the LOG_HISTORY down
> to under LOG_LEVEL?

Yes please.

> >> >> +	RTE_LOG_EMERG,      /**< System is unusable.               */
> >> >> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
> >> >> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
> >> >> +	RTE_LOG_ERR,        /**< Error conditions.                 */
> >> >> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
> >> >> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
> >> >> +	RTE_LOG_INFO,       /**< Informational.                    */
> >> >> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
> >> >> +};
> >> >
> >> >What is the benefit of this change?
> >> 
> >> The change is to use a enum in place of using magic numbers, plus you
> >>get
> >> the benefit of seeing the enum name in the debugger instead of a number.
> >> It makes the code more readable IMHO.
> >
> >OK so a comment in the commit message could give the debugger
> >justification.
> 
> OK will add the debugger comment to the commit log.

Thanks
Please also explain that rte_logs was set after options parsing,
defaulting to RTE_LOG_LEVEL, and it is now initialized at RTE_LOG_LEVEL
without behavioral change. 

[dpdk-dev] [PATCH v2] log:Change magic number on RTE_LOG_LEVEL to an enum name

[Keith Wiles]

Config files used RTE_LOG_LEVEL=8 to set log level to DEBUG. Using
a the RTE_LOG_XXXX is easier to maintain.

Converted the RTE_LOG_XXXX define into a enum of values with
the same names to reduce maintaining the define values.

The change is to use an enum in place of a magic number, plus
we get the benefit of seeing the enum name in the debugger
instead of a number.

The rte_logs was set after options parsing, defaulting to
RTE_LOG_LEVEL, and it is now initialized at RTE_LOG_LEVEL
without behavioral change.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
 config/common_bsdapp                    |  6 ++++--
 config/common_linuxapp                  |  6 ++++--
 lib/librte_eal/common/eal_common_log.c  |  4 ++--
 lib/librte_eal/common/include/rte_log.h | 18 ++++++++++--------
 4 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index 2c6eb40..b2e9462 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -102,12 +102,14 @@ CONFIG_RTE_MAX_NUMA_NODES=8
 CONFIG_RTE_MAX_MEMSEG=256
 CONFIG_RTE_MAX_MEMZONE=2560
 CONFIG_RTE_MAX_TAILQ=32
-CONFIG_RTE_LOG_LEVEL=8
-CONFIG_RTE_LOG_HISTORY=256
 CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
 CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# RTE_LOG_XXX = DEBUG, INFO, NOTICE, WARNING or ERR
+CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
+CONFIG_RTE_LOG_HISTORY=256
+
 #
 # FreeBSD contiguous memory driver settings
 #
diff --git a/config/common_linuxapp b/config/common_linuxapp
index bda9a63..eb0f659 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -102,8 +102,6 @@ CONFIG_RTE_MAX_NUMA_NODES=8
 CONFIG_RTE_MAX_MEMSEG=256
 CONFIG_RTE_MAX_MEMZONE=2560
 CONFIG_RTE_MAX_TAILQ=32
-CONFIG_RTE_LOG_LEVEL=8
-CONFIG_RTE_LOG_HISTORY=256
 CONFIG_RTE_LIBEAL_USE_HPET=n
 CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
 CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
@@ -111,6 +109,10 @@ CONFIG_RTE_EAL_IGB_UIO=y
 CONFIG_RTE_EAL_VFIO=y
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# RTE_LOG_XXX = DEBUG, INFO, NOTICE, WARNING or ERR
+CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
+CONFIG_RTE_LOG_HISTORY=256
+
 #
 # Special configurations in PCI Config Space for high performance
 #
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index 1ae8de7..6ed6743 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -82,7 +82,7 @@ static struct log_history_list log_history;
 /* global log structure */
 struct rte_logs rte_logs = {
 	.type = ~0,
-	.level = RTE_LOG_DEBUG,
+	.level = RTE_LOG_LEVEL,
 	.file = NULL,
 };
 
@@ -93,7 +93,7 @@ static int history_enabled = 1;
 
 /**
  * This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
  */
 struct log_cur_msg {
 	uint32_t loglevel; /**< log level - see rte_log.h */
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 24a55cc..be75a45 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -89,14 +89,16 @@ 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.             */
+enum {
+	RTE_LOG_EMERG=1,    /**< System is unusable.               */
+	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
+	RTE_LOG_CRIT,       /**< Critical conditions.              */
+	RTE_LOG_ERR,        /**< Error conditions.                 */
+	RTE_LOG_WARNING,    /**< Warning conditions.               */
+	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
+	RTE_LOG_INFO,       /**< Informational.                    */
+	RTE_LOG_DEBUG       /**< Debug-level messages.             */
+};
 
 /** The default log stream. */
 extern FILE *eal_default_log_stream;
-- 
2.3.0

Re: [dpdk-dev] [PATCH v2] log:Change magic number on RTE_LOG_LEVEL to an enum name

[Stephen Hemminger]

On Sun,  2 Aug 2015 16:40:24 -0500
Keith Wiles <keith.wiles@intel.com> wrote:

> Config files used RTE_LOG_LEVEL=8 to set log level to DEBUG. Using
> a the RTE_LOG_XXXX is easier to maintain.
> 
> Converted the RTE_LOG_XXXX define into a enum of values with
> the same names to reduce maintaining the define values.
> 
> The change is to use an enum in place of a magic number, plus
> we get the benefit of seeing the enum name in the debugger
> instead of a number.
> 
> The rte_logs was set after options parsing, defaulting to
> RTE_LOG_LEVEL, and it is now initialized at RTE_LOG_LEVEL
> without behavioral change.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
>  config/common_bsdapp                    |  6 ++++--
>  config/common_linuxapp                  |  6 ++++--
>  lib/librte_eal/common/eal_common_log.c  |  4 ++--
>  lib/librte_eal/common/include/rte_log.h | 18 ++++++++++--------
>  4 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/config/common_bsdapp b/config/common_bsdapp
> index 2c6eb40..b2e9462 100644
> --- a/config/common_bsdapp
> +++ b/config/common_bsdapp
> @@ -102,12 +102,14 @@ CONFIG_RTE_MAX_NUMA_NODES=8
>  CONFIG_RTE_MAX_MEMSEG=256
>  CONFIG_RTE_MAX_MEMZONE=2560
>  CONFIG_RTE_MAX_TAILQ=32
> -CONFIG_RTE_LOG_LEVEL=8
> -CONFIG_RTE_LOG_HISTORY=256
>  CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>  CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
>  CONFIG_RTE_MALLOC_DEBUG=n
>  
> +# RTE_LOG_XXX = DEBUG, INFO, NOTICE, WARNING or ERR
> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
> +CONFIG_RTE_LOG_HISTORY=256
> +
>  #
>  # FreeBSD contiguous memory driver settings
>  #
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index bda9a63..eb0f659 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -102,8 +102,6 @@ CONFIG_RTE_MAX_NUMA_NODES=8
>  CONFIG_RTE_MAX_MEMSEG=256
>  CONFIG_RTE_MAX_MEMZONE=2560
>  CONFIG_RTE_MAX_TAILQ=32
> -CONFIG_RTE_LOG_LEVEL=8
> -CONFIG_RTE_LOG_HISTORY=256
>  CONFIG_RTE_LIBEAL_USE_HPET=n
>  CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
>  CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
> @@ -111,6 +109,10 @@ CONFIG_RTE_EAL_IGB_UIO=y
>  CONFIG_RTE_EAL_VFIO=y
>  CONFIG_RTE_MALLOC_DEBUG=n
>  
> +# RTE_LOG_XXX = DEBUG, INFO, NOTICE, WARNING or ERR
> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
> +CONFIG_RTE_LOG_HISTORY=256
> +
>  #
>  # Special configurations in PCI Config Space for high performance
>  #
> diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
> index 1ae8de7..6ed6743 100644
> --- a/lib/librte_eal/common/eal_common_log.c
> +++ b/lib/librte_eal/common/eal_common_log.c
> @@ -82,7 +82,7 @@ static struct log_history_list log_history;
>  /* global log structure */
>  struct rte_logs rte_logs = {
>  	.type = ~0,
> -	.level = RTE_LOG_DEBUG,
> +	.level = RTE_LOG_LEVEL,
>  	.file = NULL,
>  };
>  
> @@ -93,7 +93,7 @@ static int history_enabled = 1;
>  
>  /**
>   * This global structure stores some informations about the message
> - * that is currently beeing processed by one lcore
> + * that is currently being processed by one lcore
>   */
>  struct log_cur_msg {
>  	uint32_t loglevel; /**< log level - see rte_log.h */
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index 24a55cc..be75a45 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -89,14 +89,16 @@ 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.             */
> +enum {
> +	RTE_LOG_EMERG=1,    /**< System is unusable.               */
> +	RTE_LOG_ALERT,      /**< Action must be taken immediately. */
> +	RTE_LOG_CRIT,       /**< Critical conditions.              */
> +	RTE_LOG_ERR,        /**< Error conditions.                 */
> +	RTE_LOG_WARNING,    /**< Warning conditions.               */
> +	RTE_LOG_NOTICE,     /**< Normal but significant condition. */
> +	RTE_LOG_INFO,       /**< Informational.                    */
> +	RTE_LOG_DEBUG       /**< Debug-level messages.             */
> +};
>  

This is technically a source API change. There are cases where
you could break build of existing code that was expecting #define's
(but I doubt anyone would notice0.