[dpdk-dev] [PATCH] i40e: fix the issue of wrongly reporting descriptor done
2015-07-30 06:08, Helin Zhang: > Header buffer address for header split will be filled with the > physical address for DMA, which is actually not needed at all, > as header split hasn't been supported. Hardware requires the > least bit of header address which is 'Descriptor Done' bit when > write back should be set to 0 by driver. > The issue is that if the user wants to reserve an odd number of > bytes between the mbuf header and data buffer, the physical address > to be filled in the descriptor would happen to be odd. That means > the DD bit would be set to non-zero by driver. That will result in > reporting descriptor done wrongly. > > Signed-off-by: Helin Zhang Applied, thanks
[dpdk-dev] [PATCH] e1000: fix the issue of wrongly reporting descriptor done
2015-07-30 10:39, Wenzhuo Lu: > Header buffer address for header split will be filled with the physical > address for DMA, which is actually not needed at all, as header split > hasn't been supported. Hardware requires the least bit of header address > which is 'Descriptor Done' bit when write back should be set to 0 by driver. > The issue is that if the user wants to reserve an odd number of bytes between > the mbuf header and data buffer, the physical address to be filled in the > descriptor would happen to be odd. That means the DD bit would be set to > non-zero by driver. That will result in reporting descriptor done wrongly. > > Signed-off-by: Wenzhuo Lu Applied, thanks
[dpdk-dev] [PATCH] app test: fix mempool cache_size not match limited cache_size
2015-07-29 11:22, Yong Liu: > In previous setting, mempool size and cache_size are both 32. > This is not satisfied with cache_size checking rule by now. > Cache size should less than CONFIG_RTE_MEMPOOL_CACHE_MAX_SIZE and mempool > size / 1.5. Sorry I don't really understand this explanation. > #define NB_MBUF 32 > #define MBUF_DATA_SZ (2048 + RTE_PKTMBUF_HEADROOM) > -#define PKT_BURST_SZ 32 > +#define PKT_BURST_SZ 0 > #define MEMPOOL_CACHE_SZ PKT_BURST_SZ Shouldn't be MEMPOOL_CACHE_SZ to set to 0?
[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define
2015-08-02 20:58, Wiles, Keith: > On 8/2/15, 3:44 PM, "Thomas Monjalon" wrote: > > >2015-08-02 19:10, Wiles, Keith: > >> On 8/2/15, 12:15 PM, "Thomas Monjalon" > >>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] ethdev: fix ABI breakage in lro code
2015-07-17 01:22, Vlad Zolotarov: > On 07/13/15 13:26, John McNamara wrote: > > Fix for ABI breakage introduced in LRO addition. Moves > > lro bitfield to the end of the struct/member. > > > > Fixes: 8eecb3295aed (ixgbe: add LRO support) > > > > Signed-off-by: John McNamara > > Acked-by: Vlad Zolotarov Applied, thanks We still don't know if POWER Big Endian ABI is broken.
[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define
2015-08-02 19:10, Wiles, Keith:
> On 8/2/15, 12:15 PM, "Thomas Monjalon" 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.
[dpdk-dev] [PATCH] eal/linux: fix negative value for undetermined numa_node
On Mon, Aug 03, 2015 at 09:46:54AM +0800, Liang, Cunming wrote: > According to the API definition, if the socket could not be determined, a > default of zero will take. > The '-1' is returned when the port_id value is out of range. Yes, but when I asked the exact same question and was told the documentation was wrong not the -1 return value. > To your concern, "difference between no NUMA, something running on socket > zero, and something with multiple sockets.". > The latter two belongs to the same situation, that is the numa_node stores > the NUMA id. > So in fact the concern is about using '-1' or '0' when there's no NUMA > detect. > If we won't plan to redefine the API return value, the fix patch is > reasonable. > > Btw, if it returns '-1' when no NUMA is detected, what will you do, do > condition check '-1' and then use node 0 instead ? > In that way, you can't distinguish '-'1 is out of range port_id error or no > NUMA detection error. I asked that question also, and the answer I got was to use node 0 instead. > If it is, why not follow the API definition. Sure, if nobody objects like the last time I asked. But this will change the user behavior as I am looking for the -1 now. > /Steve Matthew.
[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define
On 8/2/15, 3:44 PM, "Thomas Monjalon" wrote:
>2015-08-02 19:10, Wiles, Keith:
>> On 8/2/15, 12:15 PM, "Thomas Monjalon"
>>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
[dpdk-dev] [PATCH v2] log:Change magic number on RTE_LOG_LEVEL to an enum name
On Sun, 2 Aug 2015 16:40:24 -0500
Keith Wiles wrote:
> Config files used RTE_LOG_LEVEL=8 to set log level to DEBUG. Using
> a the RTE_LOG_ is easier to maintain.
>
> Converted the RTE_LOG_ 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
> ---
> 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 0x8000 /**< User-defined log type 8. */
>
> /* Can't use 0, as it gives compiler warnings */
> -#define RTE_LOG_EMERG1U /**< System is unusable. */
> -#define RTE_LOG_ALERT2U /**< 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_DEBUG8U /**< 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.
[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define
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_EMERG1U /**< System is unusable. */
> -#define RTE_LOG_ALERT2U /**< 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_DEBUG8U /**< 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?
[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define
On 8/2/15, 2:10 PM, "dev on behalf of Wiles, Keith" wrote:
>On 8/2/15, 12:15 PM, "Thomas Monjalon" 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_EMERG1U /**< System is unusable. */
>>> -#define RTE_LOG_ALERT2U /**< 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_DEBUG8U /**< 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
[dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define
On 8/2/15, 12:15 PM, "Thomas Monjalon" 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_EMERG1U /**< System is unusable. */
>> -#define RTE_LOG_ALERT2U /**< 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_DEBUG8U /**< 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
[dpdk-dev] [PATCH v2] log:Change magic number on RTE_LOG_LEVEL to an enum name
Config files used RTE_LOG_LEVEL=8 to set log level to DEBUG. Using
a the RTE_LOG_ is easier to maintain.
Converted the RTE_LOG_ 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
---
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 0x8000 /**< User-defined log type 8. */
/* Can't use 0, as it gives compiler warnings */
-#define RTE_LOG_EMERG1U /**< System is unusable. */
-#define RTE_LOG_ALERT2U /**< 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_DEBUG8U /**< 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
[dpdk-dev] dpdk 2.0 - how to build the documentation
On 8/2/15, 12:41 AM, "dev on behalf of Thomas Monjalon" wrote: >2015-08-02 09:39, Kevin Wilson: >> I have downloaded dpdk-2.0.0.tar.gz from dpdk.org >> How do I build the documentation (pdf files)? > >They are available here: > http://dpdk.org/doc/archives/dpdk-2.0/pdf/guides/ >If you want to build them: > make doc-guides-pdf >You'll need these packages: > python-sphinx, inkscape, texlive-collection-latexextra, >texlive-collection-fontsextra Here is the command line I used for my Ubuntu install: sudo apt-get install texlive-latex-extra texlive-fonts-extra python-sphinx inkscape Distributor ID: Ubuntu Description:Ubuntu 14.04.2 LTS Release:14.04 Codename: trusty Hope that help someone else ? Regards, ++Keith Intel Corporation
[dpdk-dev] dpdk 2.0 - how to build the documentation
2015-08-02 09:39, Kevin Wilson: > I have downloaded dpdk-2.0.0.tar.gz from dpdk.org > How do I build the documentation (pdf files)? They are available here: http://dpdk.org/doc/archives/dpdk-2.0/pdf/guides/ If you want to build them: make doc-guides-pdf You'll need these packages: python-sphinx, inkscape, texlive-collection-latexextra, texlive-collection-fontsextra
[dpdk-dev] dpdk 2.0 - how to build the documentation
Hi, I have downloaded dpdk-2.0.0.tar.gz from dpdk.org How do I build the documentation (pdf files)? Regards, Kevin
