|
[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: _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |