DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier MATZ <olivier.matz@6wind.com>
To: Pablo de Lara <pablo.de.lara.guarch@intel.com>
Cc: thomas.monjalon@6wind.com, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] eal: redefine logtype values
Date: Wed, 12 Apr 2017 21:23:47 +0200	[thread overview]
Message-ID: <20170412212347.0cd1966d@neon> (raw)
In-Reply-To: <1492011332-5846-1-git-send-email-pablo.de.lara.guarch@intel.com>

Hi Pablo,


On Wed, 12 Apr 2017 16:35:32 +0100
Pablo de Lara <pablo.de.lara.guarch@intel.com> wrote:

> After the changes in commit c1b5fa94a46f
> ("eal: support dynamic log types"), logtype is not treated as a
> bitmask, but a decimal value. Therefore, values have to be
> converted.
> 
> Fixes: c1b5fa94a46f ("eal: support dynamic log types")
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> 
> Changes in v2:
> - Used new RTE_LOGTYPE values in rte_log_init()
> 
>  lib/librte_eal/common/eal_common_log.c  | 56 ++++++++++++++++---------------
>  lib/librte_eal/common/include/rte_log.h | 58 ++++++++++++++++-----------------
>  2 files changed, 58 insertions(+), 56 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
> index dd4d30c..b745811 100644
> --- a/lib/librte_eal/common/eal_common_log.c
> +++ b/lib/librte_eal/common/eal_common_log.c
> @@ -249,33 +249,35 @@ rte_log_init(void)
>  	if (rte_logs.dynamic_types == NULL)
>  		return;
>  
> -	/* register legacy log types, keep sync'd with RTE_LOGTYPE_* */
> -	__rte_log_register("eal", 0);
> -	__rte_log_register("malloc", 1);
> -	__rte_log_register("ring", 2);
> -	__rte_log_register("mempool", 3);
> -	__rte_log_register("timer", 4);
> -	__rte_log_register("pmd", 5);
> -	__rte_log_register("hash", 6);
> -	__rte_log_register("lpm", 7);
> -	__rte_log_register("kni", 8);
> -	__rte_log_register("acl", 9);
> -	__rte_log_register("power", 10);
> -	__rte_log_register("meter", 11);
> -	__rte_log_register("sched", 12);
> -	__rte_log_register("port", 13);
> -	__rte_log_register("table", 14);
> -	__rte_log_register("pipeline", 15);
> -	__rte_log_register("mbuf", 16);
> -	__rte_log_register("cryptodev", 17);
> -	__rte_log_register("user1", 24);
> -	__rte_log_register("user2", 25);
> -	__rte_log_register("user3", 26);
> -	__rte_log_register("user4", 27);
> -	__rte_log_register("user5", 28);
> -	__rte_log_register("user6", 29);
> -	__rte_log_register("user7", 30);
> -	__rte_log_register("user8", 31);
> +	/* register legacy log types */
> +	__rte_log_register("eal",       RTE_LOGTYPE_EAL);
> +	__rte_log_register("malloc",    RTE_LOGTYPE_MALLOC);
> +	__rte_log_register("ring",      RTE_LOGTYPE_RING);
> +	__rte_log_register("mempool",   RTE_LOGTYPE_MEMPOOL);
> +	__rte_log_register("timer",     RTE_LOGTYPE_TIMER);
> +	__rte_log_register("pmd",       RTE_LOGTYPE_PMD);
> +	__rte_log_register("hash",      RTE_LOGTYPE_HASH);
> +	__rte_log_register("lpm",       RTE_LOGTYPE_LPM);
> +	__rte_log_register("kni",       RTE_LOGTYPE_KNI);
> +	__rte_log_register("acl",       RTE_LOGTYPE_ACL);
> +	__rte_log_register("power",     RTE_LOGTYPE_POWER);
> +	__rte_log_register("meter",     RTE_LOGTYPE_METER);
> +	__rte_log_register("sched",     RTE_LOGTYPE_SCHED);
> +	__rte_log_register("port",      RTE_LOGTYPE_PORT);
> +	__rte_log_register("table",     RTE_LOGTYPE_TABLE);
> +	__rte_log_register("pipeline",  RTE_LOGTYPE_PIPELINE);
> +	__rte_log_register("mbuf",      RTE_LOGTYPE_MBUF);
> +	__rte_log_register("cryptodev", RTE_LOGTYPE_CRYPTODEV);
> +	__rte_log_register("efd",       RTE_LOGTYPE_EFD);
> +	__rte_log_register("eventdev",  RTE_LOGTYPE_EVENTDEV);
> +	__rte_log_register("user1",     RTE_LOGTYPE_USER1);
> +	__rte_log_register("user2",     RTE_LOGTYPE_USER2);
> +	__rte_log_register("user3",     RTE_LOGTYPE_USER3);
> +	__rte_log_register("user4",     RTE_LOGTYPE_USER4);
> +	__rte_log_register("user5",     RTE_LOGTYPE_USER5);
> +	__rte_log_register("user6",     RTE_LOGTYPE_USER6);
> +	__rte_log_register("user7",     RTE_LOGTYPE_USER7);
> +	__rte_log_register("user8",     RTE_LOGTYPE_USER8);
>  
>  	rte_logs.dynamic_types_len = RTE_LOGTYPE_FIRST_EXT_ID;
>  }
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index c2ff975..3419138 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -66,37 +66,37 @@ struct rte_logs {
>  /** Global log informations */
>  extern struct rte_logs rte_logs;
>  
> -/* SDK log type, keep sync'd with rte_log_init() */
> -#define RTE_LOGTYPE_EAL     0x00000001 /**< Log related to eal. */
> -#define RTE_LOGTYPE_MALLOC  0x00000002 /**< Log related to malloc. */
> -#define RTE_LOGTYPE_RING    0x00000004 /**< Log related to ring. */
> -#define RTE_LOGTYPE_MEMPOOL 0x00000008 /**< Log related to mempool. */
> -#define RTE_LOGTYPE_TIMER   0x00000010 /**< Log related to timers. */
> -#define RTE_LOGTYPE_PMD     0x00000020 /**< Log related to poll mode driver. */
> -#define RTE_LOGTYPE_HASH    0x00000040 /**< Log related to hash table. */
> -#define RTE_LOGTYPE_LPM     0x00000080 /**< Log related to LPM. */
> -#define RTE_LOGTYPE_KNI     0x00000100 /**< Log related to KNI. */
> -#define RTE_LOGTYPE_ACL     0x00000200 /**< Log related to ACL. */
> -#define RTE_LOGTYPE_POWER   0x00000400 /**< Log related to power. */
> -#define RTE_LOGTYPE_METER   0x00000800 /**< Log related to QoS meter. */
> -#define RTE_LOGTYPE_SCHED   0x00001000 /**< Log related to QoS port scheduler. */
> -#define RTE_LOGTYPE_PORT    0x00002000 /**< Log related to port. */
> -#define RTE_LOGTYPE_TABLE   0x00004000 /**< Log related to table. */
> -#define RTE_LOGTYPE_PIPELINE 0x00008000 /**< Log related to pipeline. */
> -#define RTE_LOGTYPE_MBUF    0x00010000 /**< Log related to mbuf. */
> -#define RTE_LOGTYPE_CRYPTODEV 0x00020000 /**< Log related to cryptodev. */
> -#define RTE_LOGTYPE_EFD     0x00040000 /**< Log related to EFD. */
> -#define RTE_LOGTYPE_EVENTDEV 0x00080000 /**< Log related to eventdev. */
> +/* SDK log type */
> +#define RTE_LOGTYPE_EAL        0 /**< Log related to eal. */
> +#define RTE_LOGTYPE_MALLOC     1 /**< Log related to malloc. */
> +#define RTE_LOGTYPE_RING       2 /**< Log related to ring. */
> +#define RTE_LOGTYPE_MEMPOOL    3 /**< Log related to mempool. */
> +#define RTE_LOGTYPE_TIMER      4 /**< Log related to timers. */
> +#define RTE_LOGTYPE_PMD        5 /**< Log related to poll mode driver. */
> +#define RTE_LOGTYPE_HASH       6 /**< Log related to hash table. */
> +#define RTE_LOGTYPE_LPM        7 /**< Log related to LPM. */
> +#define RTE_LOGTYPE_KNI        8 /**< Log related to KNI. */
> +#define RTE_LOGTYPE_ACL        9 /**< Log related to ACL. */
> +#define RTE_LOGTYPE_POWER     10 /**< Log related to power. */
> +#define RTE_LOGTYPE_METER     11 /**< Log related to QoS meter. */
> +#define RTE_LOGTYPE_SCHED     12 /**< Log related to QoS port scheduler. */
> +#define RTE_LOGTYPE_PORT      13 /**< Log related to port. */
> +#define RTE_LOGTYPE_TABLE     14 /**< Log related to table. */
> +#define RTE_LOGTYPE_PIPELINE  15 /**< Log related to pipeline. */
> +#define RTE_LOGTYPE_MBUF      16 /**< Log related to mbuf. */
> +#define RTE_LOGTYPE_CRYPTODEV 17 /**< Log related to cryptodev. */
> +#define RTE_LOGTYPE_EFD       18 /**< Log related to EFD. */
> +#define RTE_LOGTYPE_EVENTDEV  19 /**< Log related to eventdev. */
>  
>  /* these log types can be used in an application */
> -#define RTE_LOGTYPE_USER1   0x01000000 /**< User-defined log type 1. */
> -#define RTE_LOGTYPE_USER2   0x02000000 /**< User-defined log type 2. */
> -#define RTE_LOGTYPE_USER3   0x04000000 /**< User-defined log type 3. */
> -#define RTE_LOGTYPE_USER4   0x08000000 /**< User-defined log type 4. */
> -#define RTE_LOGTYPE_USER5   0x10000000 /**< User-defined log type 5. */
> -#define RTE_LOGTYPE_USER6   0x20000000 /**< User-defined log type 6. */
> -#define RTE_LOGTYPE_USER7   0x40000000 /**< User-defined log type 7. */
> -#define RTE_LOGTYPE_USER8   0x80000000 /**< User-defined log type 8. */
> +#define RTE_LOGTYPE_USER1     24 /**< User-defined log type 1. */
> +#define RTE_LOGTYPE_USER2     25 /**< User-defined log type 2. */
> +#define RTE_LOGTYPE_USER3     26 /**< User-defined log type 3. */
> +#define RTE_LOGTYPE_USER4     27 /**< User-defined log type 4. */
> +#define RTE_LOGTYPE_USER5     28 /**< User-defined log type 5. */
> +#define RTE_LOGTYPE_USER6     29 /**< User-defined log type 6. */
> +#define RTE_LOGTYPE_USER7     30 /**< User-defined log type 7. */
> +#define RTE_LOGTYPE_USER8     31 /**< User-defined log type 8. */
>  
>  /** First identifier for extended logs */
>  #define RTE_LOGTYPE_FIRST_EXT_ID 32



Thanks for spotting this issue. I think there is still a problem with
the deprecated functions for which we want to keep compat:

/* Set global log type */
__rte_deprecated void
rte_set_log_type(uint32_t type, int enable)
{
	if (type < RTE_LOGTYPE_FIRST_EXT_ID) {
		if (enable)
			rte_logs.type |= type;
		else
			rte_logs.type &= ~type;
	}

	if (enable)
		rte_log_set_level(type, 0);
	else
		rte_log_set_level(type, RTE_LOG_DEBUG);
}

/* Get global log type */
__rte_deprecated uint32_t
rte_get_log_type(void)
{
	return rte_logs.type;
}


There is a problem in these functions because they expect bitmasks
for the first part.

I does not look so easy to both fix the issue and keep a full compat
with previous functions.

The first solution is your patch + add a shift in rte_set_log_type().
It would work except if an application uses rte_get_log_type() and masks
the result with a RTE_LOGTYPE_* (which will not be a bitmask). I think
it's a rare case.

The second approach would be to define new constants for the new
functions and keep the old ones for the old functions. This approach
is probably more confusing. It also requires to check the doc and
examples again.

Any opinion?
I would prefer the first solution. If you want I can send a patch
updated based on yours.

Sorry for having missed that.

Olivier

  reply	other threads:[~2017-04-12 19:23 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-12 14:14 [dpdk-dev] [PATCH] " Pablo de Lara
2017-04-12 15:15 ` Thomas Monjalon
2017-04-12 15:22   ` De Lara Guarch, Pablo
2017-04-12 15:35 ` [dpdk-dev] [PATCH v2] " Pablo de Lara
2017-04-12 19:23   ` Olivier MATZ [this message]
2017-04-13  8:32     ` De Lara Guarch, Pablo
2017-04-13  9:09       ` De Lara Guarch, Pablo
2017-04-12 21:41   ` Stephen Hemminger
2017-04-13 13:43     ` De Lara Guarch, Pablo
2017-04-13 13:42   ` [dpdk-dev] [PATCH v3] " Pablo de Lara
2017-04-18  9:57     ` Olivier MATZ
2017-04-19 11:12       ` De Lara Guarch, Pablo
2017-04-19 11:22     ` [dpdk-dev] [PATCH] " Pablo de Lara
2017-04-19 11:23       ` De Lara Guarch, Pablo
2017-04-19 11:24     ` [dpdk-dev] [PATCH v4] " Pablo de Lara
2017-04-19 12:15       ` Olivier MATZ
2017-04-19 13:46         ` De Lara Guarch, Pablo
2017-04-19 14:06       ` [dpdk-dev] [PATCH v5] " Pablo de Lara
2017-04-19 14:16         ` Olivier MATZ
2017-04-19 22:49           ` Thomas Monjalon

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=20170412212347.0cd1966d@neon \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=thomas.monjalon@6wind.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).