[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Costin,

On 01.08.2018 07:57, Costin Lupu wrote:
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

Personally, I do not like this since the DEBUG/EXTRA level behaves differently than the other levels. For clarity, I prefer a different interface.

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(), ...).

I am fine with this. I would introduce them as shortcut macros to the corresponding uk_printk() and uk_printd() calls.


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.

Great! ;-)


Cheers,
Costin

Cheers,

Simon


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


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.