Re: [Minios-devel] [UNIKRAFT PATCH] lib/ukdebug: print DLVL_EXTRA messages only in debug build

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi Simon,

> although I dislike the fact that it requires to manually modify the
> Makefiles.uk.
This is used only at debug time - meaning this is a developer, in a
debug mode working on the problem. He would anyways touch files.

> But maybe we could add later a debug option into each libraries menu
> so that their Makefile.uk's set -D__UK_DEBUG__ by themselves. This way
> libraries could provide a menu option to enable/disable debugging.
Exactly, that what I was thinking.

> I think it is still a valuable option to select the verbosity-level
> even if you enable debug message printing only for a subset of
> libraries or objects.
This is going to be the next step - implementing dynamic debug. So you
could change the level of each module on the fly. And I would not do it
at the compile time. In fact, I kind of like the way it happens in linux
- all levels of printk (except the debug level) are compiled always, but
they are doing nothing at least the right level is enabled.

> I would make this option independent of the chosen
> debug-verbosity-level.
Unfortunately we need to revisit the current scheme for this. Because of
the check "if (lvl > DLVL_MAX)".

What we need is a separate macro for printing debug
messages. Unconditionally, independent of the current DLVL_*.

I was actually thinking about introducing the family of macro:
uk_pr_extra()
uk_pr_info()
uk_pr_warn()
uk_pr_err()
uk_pr_crit()
uk_pr_debug()

The first 5 will be a simple translation to uk_printd() with
corresponding debug level.

And the last one does not care about the level, but it is not even
compiled only if UK_DEBUG is defined.


> I would actually add a bool option in ukdebug if debug messages should
> be on globally (default) or selective only.
This is very easy to do, but I don't see this actually helps. Debug
messages are way too verbose, and one just can not spot what is
important. Remember when we was hunting the problem in lwip together?
Enabling all the "DLVL_EXTRA" output made the prints unuseful
immediately.

However, the situation is different when we will have a dynamic
print. All levels, including debug MUST be enabled at the build
time. And a user will be able to pick the desired level either for
chosen units, or for for the entire system.

In my opinion, all the outputs which are NOT debug, should be readable
even if the maximum level is enabled. If it is producing too much of
data it either should be rate limited print, or should go to the debug.

-- Yuri.

Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:

> Hey Yuri,
>
> in general this is a feasible solution to enable selective debug 
> messages, although I dislike the fact that it requires to manually 
> modify the Makefiles.uk. But maybe we could add later a debug option 
> into each libraries menu so that their Makefile.uk's set -D__UK_DEBUG__ 
> by themselves. This way libraries could provide a menu option to 
> enable/disable debugging.
>
> Having this in mind I would change the behavior of this patch. I would 
> actually add a bool option in ukdebug if debug messages should be on 
> globally (default) or selective only. I would make this option 
> independent of the chosen debug-verbosity-level. I think it is still a 
> valuable option to select the verbosity-level even if you enable debug 
> message printing only for a subset of libraries or objects.
> This would even enable that libraries would not compile in the call to 
> uk_printd() if they were unselected and global debugging is off.
>
> What do you think?
>
> On 18.07.2018 23:23, Yuri Volchkov wrote:
>> At this point enabling LIBUKDEBUG_PRINTD_EXTRA does not help. You will
>> be drowned with the output.
>> 
>> Basically this became a real debug-level of message explicitness. So
>> let's use it for debug purposes.
>> 
>> With this patch, messages of DLVL_EXTRA will be printed ONLY if
>> UK_DEBUG is defined. Now a developer can chose for which parts of
>> Unikraft he wants an extra verbosity of the output, by adding a single
>> line into the Makefile.uk.
>> 
>> For example:
>>      /* Enable for one lib */
>>      LIBNAME_CFLAGS-y             += -DUK_DEBUG
>> 
>>      /* Enable globally in Unikraft (brace yourself) */
>>      CFLAGS-y                     += -DUK_DEBUG
>> 
>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>>   lib/ukdebug/Config.uk            |  2 +-
>>   lib/ukdebug/include/uk/hexdump.h |  2 +-
>>   lib/ukdebug/include/uk/print.h   | 16 ++++++++++++++--
>>   3 files changed, 16 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/ukdebug/Config.uk b/lib/ukdebug/Config.uk
>> index dcaeb3a..ff6279c 100644
>> --- a/lib/ukdebug/Config.uk
>> +++ b/lib/ukdebug/Config.uk
>> @@ -24,7 +24,7 @@ choice
>>        Set the level of detail of debug messages
>>   
>>   config LIBUKDEBUG_PRINTD_EXTRA
>> -    bool "Show all types of debug messages"
>> +    bool "Same as info + debug level messages (UK_DEBUG needs to be 
>> defined)"
>>   
>>   config LIBUKDEBUG_PRINTD_INFO
>>      bool "Show critical, error, warning, and information messages"
>> diff --git a/lib/ukdebug/include/uk/hexdump.h 
>> b/lib/ukdebug/include/uk/hexdump.h
>> index 4d32647..927769d 100644
>> --- a/lib/ukdebug/include/uk/hexdump.h
>> +++ b/lib/ukdebug/include/uk/hexdump.h
>> @@ -92,7 +92,7 @@ void _uk_hexdumpd(int lvl, const char *libname, const char 
>> *srcname,
>>    */
>>   #define uk_hexdumpd(lvl, data, len, flags, grps_per_line)                  
>>     \
>>      do {                                                                   \
>> -            if ((lvl) <= DLVL_MAX)                                         \
>> +            if (__ukdebug_is_printable_lvl(lvl))                           \
>>                      _uk_hexdumpd((lvl), __STR_LIBNAME__, __STR_BASENAME__, \
>>                                   __LINE__, (data), (len),                  \
>>                                   ((size_t)(data)), (flags),                \
>> diff --git a/lib/ukdebug/include/uk/print.h b/lib/ukdebug/include/uk/print.h
>> index c5c5557..61e6bf6 100644
>> --- a/lib/ukdebug/include/uk/print.h
>> +++ b/lib/ukdebug/include/uk/print.h
>> @@ -120,16 +120,28 @@ void _uk_printd(int lvl, const char *libname, const 
>> char *srcname,
>>   #define __STR_BASENAME__ (NULL)
>>   #endif
>>   
>> +#ifdef UK_DEBUG
>> +#define __uk_is_debug_lvl(lvl) (lvl <= DLVL_EXTRA)
>> +#else
>> +#define __uk_is_debug_lvl(lvl) (0)
>> +#endif
>> +
>> +#if defined(UK_DEBUG) && DLVL_MAX == DLVL_EXTRA
>> +#define __ukdebug_is_printable_lvl(lvl) (lvl <= DLVL_MAX)
>> +#else
>> +#define __ukdebug_is_printable_lvl(lvl) (lvl <= MIN(DLVL_MAX, DLVL_INFO))
>> +#endif
>> +
>>   #define uk_vprintd(lvl, fmt, ap)                                           
>>     \
>>      do {                                                                   \
>> -            if ((lvl) <= DLVL_MAX)                                         \
>> +            if (__ukdebug_is_printable_lvl(lvl))                           \
>>                      _uk_vprintd((lvl), __STR_LIBNAME__, __STR_BASENAME__,  \
>>                                  __LINE__, (fmt), ap);                      \
>>      } while (0)
>>   
>>   #define uk_printd(lvl, fmt, ...)                                           
>>     \
>>      do {                                                                   \
>> -            if ((lvl) <= DLVL_MAX)                                         \
>> +            if (__ukdebug_is_printable_lvl(lvl))                           \
>>                      _uk_printd((lvl), __STR_LIBNAME__, __STR_BASENAME__,   \
>>                                 __LINE__, (fmt), ##__VA_ARGS__);            \
>>      } while (0)
>> 

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel