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

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Simon,

While I agree this change clarifies more the purpose of uk_printk() and
uk_printd(), I believe it is still not the optimum solution and here is
why. Our current experience shows that using two types of "functions"
(uk_printd() with flags vs uk_printk()) is confusing and actually only
one of the two gets to be used in the code. So why not dropping one of
the two? For simplicity and consistency, I would propose to either:
1. use only uk_printk(lvl, fmt, ...) and keep the EXTRA flag (and
possibly rename it to DEBUG), or
2. use the schema proposed by Yuri in the previous mail (e.g. one name
for each kind of output: uk_pr_extra(), uk_pr_info(), ...).

With one of the amendments above, I would vote for +1. Also, in both
cases, we can still use Yuri's patch as a transition step.

Cheers,
Costin

On 07/31/2018 04:05 PM, Simon Kuenzer wrote:
> Hi Florian, Sharan, Costin, Wei,
> 
> after I had an offline discussion with Yuri, I understood what he
> intended to do. He wants to introduce another debug print option that is
> independent of any level while not touching the existing level scheme.
> This extra debug print should only be enabled when the file is compiled
> with a new introduced flag: "-DUK_DEBUG"
> 
> As we discussed, currently there exists confusion and unclarity when to
> use uk_printk() and uk_printd(). uk_printk() is almost not used at all
> and was intended for applications only.
> Now, we progressed with a project a bit further and know that libc's are
> providing printf() for applications which makes uk_printk() obsolete.
> So, I want to have a brief discussion what you think about the following
> change of the API:
> 
> Make uk_printk() as the new default output for Unikraft library messages:
> 
> uk_printk(lvl, fmt, ...)
> - where lvl is the level parameter as known from uk_printd() today
>   (CRIT, ERR, WARN, INFO, but no EXTRA).
> 
> uk_printd(fmt, ...)
>  becomes a stand-alone debug print that is only enabled if a file is
>  compiled with "-DUK_DEBUG". It is "more-or-less" a replacement for
>  the EXTRA level that we had before.
> 
> Printing of the individual levels would be still configurable with
> libukdebug. uk_printd() could be enabled by each individual library by
> adding a boolean menu option in their menu that adds "-DUK_DEBUG" to the
> compilation units through the Makefile.uk's.
> 
> I think this change makes the purpose of uk_printk() and uk_printd()
> much clearer but I would like to know what you think. Give me a +1/0/-1.
> In case most of you guys agree, I would take this patch of Yuri as an
> intermediate transition step to this new proposed scheme. It just
> redefines DLVL_EXTRA as the debug print. This patch won't break current
> code. So, we are still able to adopt each code location later when we
> have the ARM patches integrated.
> 
> Thanks,
> 
> Simon
> 
> My vote: +1 ;-)
> 
> On 20.07.2018 11:28, Yuri Volchkov wrote:
>> 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)
>>>>
>>
> 
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel

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