[dpdk-dev,v4,11/17] log: fix the gap to support non-EAL thread

Message ID 1422842559-13617-12-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Cunming Liang Feb. 2, 2015, 2:02 a.m. UTC
For those non-EAL thread, *_lcore_id* is invalid and probably larger than RTE_MAX_LCORE.
The patch adds the check and allows only EAL thread using EAL per thread log level and log type.
Others shares the global log level.

Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 lib/librte_eal/common/eal_common_log.c  | 17 +++++++++++++++--
 lib/librte_eal/common/include/rte_log.h |  5 +++++
 2 files changed, 20 insertions(+), 2 deletions(-)
  

Comments

Olivier Matz Feb. 8, 2015, 8:01 p.m. UTC | #1
Hi,

On 02/02/2015 03:02 AM, Cunming Liang wrote:
> For those non-EAL thread, *_lcore_id* is invalid and probably larger than RTE_MAX_LCORE.
> The patch adds the check and allows only EAL thread using EAL per thread log level and log type.
> Others shares the global log level.
> 
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> ---
>  lib/librte_eal/common/eal_common_log.c  | 17 +++++++++++++++--
>  lib/librte_eal/common/include/rte_log.h |  5 +++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
> index cf57619..e8dc94a 100644
> --- a/lib/librte_eal/common/eal_common_log.c
> +++ b/lib/librte_eal/common/eal_common_log.c
> @@ -193,11 +193,20 @@ rte_set_log_type(uint32_t type, int enable)
>  		rte_logs.type &= (~type);
>  }
>  
> +/* Get global log type */
> +uint32_t
> +rte_get_log_type(void)
> +{
> +	return rte_logs.type;
> +}
> +
>  /* get the current loglevel for the message beeing processed */
>  int rte_log_cur_msg_loglevel(void)
>  {
>  	unsigned lcore_id;
>  	lcore_id = rte_lcore_id();
> +	if (lcore_id >= RTE_MAX_LCORE)
> +		return rte_get_log_level();
>  	return log_cur_msg[lcore_id].loglevel;
>  }
>  
> @@ -206,6 +215,8 @@ int rte_log_cur_msg_logtype(void)
>  {
>  	unsigned lcore_id;
>  	lcore_id = rte_lcore_id();
> +	if (lcore_id >= RTE_MAX_LCORE)
> +		return rte_get_log_type();
>  	return log_cur_msg[lcore_id].logtype;
>  }
>  
> @@ -265,8 +276,10 @@ rte_vlog(__attribute__((unused)) uint32_t level,
>  
>  	/* save loglevel and logtype in a global per-lcore variable */
>  	lcore_id = rte_lcore_id();
> -	log_cur_msg[lcore_id].loglevel = level;
> -	log_cur_msg[lcore_id].logtype = logtype;
> +	if (lcore_id < RTE_MAX_LCORE) {
> +		log_cur_msg[lcore_id].loglevel = level;
> +		log_cur_msg[lcore_id].logtype = logtype;
> +	}
>  
>  	ret = vfprintf(f, format, ap);
>  	fflush(f);
> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index db1ea08..f83a0d9 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -144,6 +144,11 @@ uint32_t rte_get_log_level(void);
>  void rte_set_log_type(uint32_t type, int enable);
>  
>  /**
> + * Get the global log type.
> + */
> +uint32_t rte_get_log_type(void);
> +
> +/**
>   * Get the current loglevel for the message being processed.
>   *
>   * Before calling the user-defined stream for logging, the log
> 

Wouldn't it be better to change the variable:
static struct log_cur_msg log_cur_msg[RTE_MAX_LCORE];
into a pthread (tls) variable?

With your patch, the log level and log type are not saved for
non-EAL threads. If TLS were used, I think it would work in any case.

Regards,
Olivier
  
Cunming Liang Feb. 9, 2015, 2:19 p.m. UTC | #2
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 09, 2015 4:01 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 11/17] log: fix the gap to support non-EAL
> thread
> 
> Hi,
> 
> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> > For those non-EAL thread, *_lcore_id* is invalid and probably larger than
> RTE_MAX_LCORE.
> > The patch adds the check and allows only EAL thread using EAL per thread log
> level and log type.
> > Others shares the global log level.
> >
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > ---
> >  lib/librte_eal/common/eal_common_log.c  | 17 +++++++++++++++--
> >  lib/librte_eal/common/include/rte_log.h |  5 +++++
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_log.c
> b/lib/librte_eal/common/eal_common_log.c
> > index cf57619..e8dc94a 100644
> > --- a/lib/librte_eal/common/eal_common_log.c
> > +++ b/lib/librte_eal/common/eal_common_log.c
> > @@ -193,11 +193,20 @@ rte_set_log_type(uint32_t type, int enable)
> >  		rte_logs.type &= (~type);
> >  }
> >
> > +/* Get global log type */
> > +uint32_t
> > +rte_get_log_type(void)
> > +{
> > +	return rte_logs.type;
> > +}
> > +
> >  /* get the current loglevel for the message beeing processed */
> >  int rte_log_cur_msg_loglevel(void)
> >  {
> >  	unsigned lcore_id;
> >  	lcore_id = rte_lcore_id();
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		return rte_get_log_level();
> >  	return log_cur_msg[lcore_id].loglevel;
> >  }
> >
> > @@ -206,6 +215,8 @@ int rte_log_cur_msg_logtype(void)
> >  {
> >  	unsigned lcore_id;
> >  	lcore_id = rte_lcore_id();
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		return rte_get_log_type();
> >  	return log_cur_msg[lcore_id].logtype;
> >  }
> >
> > @@ -265,8 +276,10 @@ rte_vlog(__attribute__((unused)) uint32_t level,
> >
> >  	/* save loglevel and logtype in a global per-lcore variable */
> >  	lcore_id = rte_lcore_id();
> > -	log_cur_msg[lcore_id].loglevel = level;
> > -	log_cur_msg[lcore_id].logtype = logtype;
> > +	if (lcore_id < RTE_MAX_LCORE) {
> > +		log_cur_msg[lcore_id].loglevel = level;
> > +		log_cur_msg[lcore_id].logtype = logtype;
> > +	}
> >
> >  	ret = vfprintf(f, format, ap);
> >  	fflush(f);
> > diff --git a/lib/librte_eal/common/include/rte_log.h
> b/lib/librte_eal/common/include/rte_log.h
> > index db1ea08..f83a0d9 100644
> > --- a/lib/librte_eal/common/include/rte_log.h
> > +++ b/lib/librte_eal/common/include/rte_log.h
> > @@ -144,6 +144,11 @@ uint32_t rte_get_log_level(void);
> >  void rte_set_log_type(uint32_t type, int enable);
> >
> >  /**
> > + * Get the global log type.
> > + */
> > +uint32_t rte_get_log_type(void);
> > +
> > +/**
> >   * Get the current loglevel for the message being processed.
> >   *
> >   * Before calling the user-defined stream for logging, the log
> >
> 
> Wouldn't it be better to change the variable:
> static struct log_cur_msg log_cur_msg[RTE_MAX_LCORE];
> into a pthread (tls) variable?
> 
> With your patch, the log level and log type are not saved for
> non-EAL threads. If TLS were used, I think it would work in any case.
[LCM] Good point. But for this patch set, still suppose not involve big impact to EAL thread.
For improve non-EAL thread, we'll have a separate patch set for it.
> 
> Regards,
> Olivier
  
Olivier Matz Feb. 9, 2015, 5:44 p.m. UTC | #3
Hi,

On 02/09/2015 03:19 PM, Liang, Cunming wrote:
>>> --- a/lib/librte_eal/common/include/rte_log.h
>>> +++ b/lib/librte_eal/common/include/rte_log.h
>>> @@ -144,6 +144,11 @@ uint32_t rte_get_log_level(void);
>>>  void rte_set_log_type(uint32_t type, int enable);
>>>
>>>  /**
>>> + * Get the global log type.
>>> + */
>>> +uint32_t rte_get_log_type(void);
>>> +
>>> +/**
>>>   * Get the current loglevel for the message being processed.
>>>   *
>>>   * Before calling the user-defined stream for logging, the log
>>>
>>
>> Wouldn't it be better to change the variable:
>> static struct log_cur_msg log_cur_msg[RTE_MAX_LCORE];
>> into a pthread (tls) variable?
>>
>> With your patch, the log level and log type are not saved for
>> non-EAL threads. If TLS were used, I think it would work in any case.
> [LCM] Good point. But for this patch set, still suppose not involve big impact to EAL thread.
> For improve non-EAL thread, we'll have a separate patch set for it.

OK, that's fine

Will it be for 2.0 or later?
  
Cunming Liang Feb. 10, 2015, 2:56 a.m. UTC | #4
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, February 10, 2015 1:45 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 11/17] log: fix the gap to support non-EAL
> thread
> 
> Hi,
> 
> On 02/09/2015 03:19 PM, Liang, Cunming wrote:
> >>> --- a/lib/librte_eal/common/include/rte_log.h
> >>> +++ b/lib/librte_eal/common/include/rte_log.h
> >>> @@ -144,6 +144,11 @@ uint32_t rte_get_log_level(void);
> >>>  void rte_set_log_type(uint32_t type, int enable);
> >>>
> >>>  /**
> >>> + * Get the global log type.
> >>> + */
> >>> +uint32_t rte_get_log_type(void);
> >>> +
> >>> +/**
> >>>   * Get the current loglevel for the message being processed.
> >>>   *
> >>>   * Before calling the user-defined stream for logging, the log
> >>>
> >>
> >> Wouldn't it be better to change the variable:
> >> static struct log_cur_msg log_cur_msg[RTE_MAX_LCORE];
> >> into a pthread (tls) variable?
> >>
> >> With your patch, the log level and log type are not saved for
> >> non-EAL threads. If TLS were used, I think it would work in any case.
> > [LCM] Good point. But for this patch set, still suppose not involve big impact to
> EAL thread.
> > For improve non-EAL thread, we'll have a separate patch set for it.
> 
> OK, that's fine
> 
> Will it be for 2.0 or later?
[LCM] Will be in 2.1 I suppose, together with the patch for mempool cache to support non-EAL thread.
  

Patch

diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index cf57619..e8dc94a 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -193,11 +193,20 @@  rte_set_log_type(uint32_t type, int enable)
 		rte_logs.type &= (~type);
 }
 
+/* Get global log type */
+uint32_t
+rte_get_log_type(void)
+{
+	return rte_logs.type;
+}
+
 /* get the current loglevel for the message beeing processed */
 int rte_log_cur_msg_loglevel(void)
 {
 	unsigned lcore_id;
 	lcore_id = rte_lcore_id();
+	if (lcore_id >= RTE_MAX_LCORE)
+		return rte_get_log_level();
 	return log_cur_msg[lcore_id].loglevel;
 }
 
@@ -206,6 +215,8 @@  int rte_log_cur_msg_logtype(void)
 {
 	unsigned lcore_id;
 	lcore_id = rte_lcore_id();
+	if (lcore_id >= RTE_MAX_LCORE)
+		return rte_get_log_type();
 	return log_cur_msg[lcore_id].logtype;
 }
 
@@ -265,8 +276,10 @@  rte_vlog(__attribute__((unused)) uint32_t level,
 
 	/* save loglevel and logtype in a global per-lcore variable */
 	lcore_id = rte_lcore_id();
-	log_cur_msg[lcore_id].loglevel = level;
-	log_cur_msg[lcore_id].logtype = logtype;
+	if (lcore_id < RTE_MAX_LCORE) {
+		log_cur_msg[lcore_id].loglevel = level;
+		log_cur_msg[lcore_id].logtype = logtype;
+	}
 
 	ret = vfprintf(f, format, ap);
 	fflush(f);
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index db1ea08..f83a0d9 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -144,6 +144,11 @@  uint32_t rte_get_log_level(void);
 void rte_set_log_type(uint32_t type, int enable);
 
 /**
+ * Get the global log type.
+ */
+uint32_t rte_get_log_type(void);
+
+/**
  * Get the current loglevel for the message being processed.
  *
  * Before calling the user-defined stream for logging, the log