On Mon, Nov 17, 2008 at 12:36:16PM -0600, Adam Litke wrote:
> On Mon, 2008-11-17 at 17:17 +0000, Mel Gorman wrote:
> > On Fri, Nov 14, 2008 at 09:00:32PM +0000, Adam Litke wrote:
> > > The default verbosity level in libhugetlbfs is 1, meaning that only
> > > errors are
> > > printed. Many of the new library features can have outcomes that are very
> > > meaningful warnings which should be displayed by default. For example,
> > > if a
> > > user specifies an incorrect page size in HUGETLB_ELFMAP, a warning is
> > > issued
> > > and segment remapping is disabled. Under the current verbosity default,
> > > this
> > > important message is suppressed. We should switch the default verbosity
> > > to 2
> > > so that such warnings are displayed.
> > >
> > > Simply increasing the default verbosity is not a complete solution. Some
> > > messages that have been classified as warnings are more informational in
> > > nature
> > > and should not always be printed. Add a new INFO verbosity level for
> > > detailed
> > > messages about normal library operation.
> > >
> > > The library currently makes a strange differentiation between verbosity
> > > levels
> > > and whether debugging mode is enabled. Verbosity is controlled by one
> > > enviroment variable and debugging by another. Thus, a user could set
> > > verbosity
> > > to 0 (no messages) and enable debugging (resulting in output with out of
> > > place
> > > debugging info and non-obvious slow downs.
> > >
> > > One can argue that debugging mode is equivalent to the highest verbosity
> > > level.
> > > Making the switch simplifies the library and interface.
> > >
> >
> > Did we not separate out debug in case debug enabling took additional
> > debug-only steps? i.e. debug verbosity may mean printing out of more
> > messages like trace messages but debug-enabled made additional checks?
>
> Yes. The original reason for splitting this out was that
> HUGETLB_DEBUG=1 would enable tracing code that has a run-time
> performance cost. My argument is that there is little value in turning
> DEBUG on while restricting verbosity to 1 since the data collected by
> the trace functions wouldn't even be printed. Therefore, you can merge
> the two options and make a DEBUG verbosity level imply HUGETLB_DEBUG=1.
>
Ok, that's cool but I guess don't remove the __hugetlb_debug so we still
have an easy way of running additional debug code.
> >
> > > So after this patch is applied, we have the following four verbosity
> > > levels
> > > with their intended uses as defined herein.
> > >
> > > ERROR - Severe, unrecoverable errors
> > > WARNING - Recoverable condition, but may result in altered semantics
> > > INFO - Detailed information about normal library operations
> > > DEBUG - Diagnostic information for debugging, potential runtime
> > > overhead
> > >
> > > Signed-off-by: Adam Litke <[EMAIL PROTECTED]>
> > > ---
> > >
> > > debug.c | 3 +--
> > > libhugetlbfs_debug.h | 17 +++++++++++++----
> > > libhugetlbfs_internal.h | 21 +++++++++++++++------
> > > 3 files changed, 29 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/debug.c b/debug.c
> > > index e854dff..7842115 100644
> > > --- a/debug.c
> > > +++ b/debug.c
> > > @@ -28,7 +28,6 @@
> > > #include "libhugetlbfs_internal.h"
> > >
> > > int __hugetlbfs_verbose = VERBOSITY_DEFAULT;
> > > -int __hugetlbfs_debug = 0;
> > > int __hugetlbfs_prefault = 1;
> > > char __hugetlbfs_hostname[64];
> > >
> > > @@ -47,7 +46,7 @@ static void __hugetlbfs_init_debug(void)
> > >
> > > env = getenv("HUGETLB_DEBUG");
> > > if (env)
> > > - __hugetlbfs_debug = 1;
> > > + __hugetlbfs_verbose = VERBOSE_DEBUG;
> > >
> > > env = getenv("HUGETLB_NO_PREFAULT");
> > > if (env)
> > > diff --git a/libhugetlbfs_debug.h b/libhugetlbfs_debug.h
> > > index 7eb8f4e..cd490ad 100644
> > > --- a/libhugetlbfs_debug.h
> > > +++ b/libhugetlbfs_debug.h
> > > @@ -20,14 +20,23 @@
> > > #ifndef _LIBHUGETLBFS_DEBUG_H
> > > #define _LIBHUGETLBFS_DEBUG_H
> > >
> > > +/* Severe, unrecoverable errors */
> > > #define ERROR(...) REPORT(1, "ERROR", ##__VA_ARGS__)
> > > #define ERROR_CONT(...) REPORT_CONT(1, "ERROR", ##__VA_ARGS__)
> > > +
> > > +/* A condition that is recoverable, but may result in altered semantics
> > > */
> > > #define WARNING(...) REPORT(2, "WARNING", ##__VA_ARGS__)
> > > #define WARNING_CONT(...) REPORT_CONT(2, "WARNING", ##__VA_ARGS__)
> > > -#define DEBUG(...) REPORT(3, "DEBUG", ##__VA_ARGS__)
> > > -#define DEBUG_CONT(...) REPORT_CONT(3, "DEBUG", ##__VA_ARGS__)
> > >
> > > -#define VERBOSITY_MAX 3
> > > -#define VERBOSITY_DEFAULT 1
> > > +/* Detailed information about normal library operations */
> > > +#define INFO(...) REPORT(3, "INFO", ##__VA_ARGS__)
> > > +#define INFO_CONT(...) REPORT_CONT(3, "INFO", ##__VA_ARGS__)
> > > +
> > > +/* Diagnostic information used for debugging problems */
> > > +#define DEBUG(...) REPORT(4, "DEBUG", ##__VA_ARGS__)
> > > +#define DEBUG_CONT(...) REPORT_CONT(4, "DEBUG", ##__VA_ARGS__)
> > > +
> > > +#define VERBOSITY_MAX 4
> > > +#define VERBOSITY_DEFAULT 2
> > >
> > > #endif
> > > diff --git a/libhugetlbfs_internal.h b/libhugetlbfs_internal.h
> > > index 6ebe9c0..8bd979b 100644
> > > --- a/libhugetlbfs_internal.h
> > > +++ b/libhugetlbfs_internal.h
> > > @@ -66,8 +66,6 @@
> > > */
> > > #define __hugetlbfs_verbose __lh___hugetlbfs_verbose
> > > extern int __hugetlbfs_verbose;
> > > -#define __hugetlbfs_debug __lh___hugetlbfs_debug
> > > -extern int __hugetlbfs_debug;
> > > #define __hugetlbfs_prefault __lh___hugetlbfs_prefault
> > > extern int __hugetlbfs_prefault;
> > > #define hugetlbfs_setup_elflink __lh_hugetlbfs_setup_elflink
> > > @@ -90,12 +88,23 @@ extern long parse_page_size(const char *str);
> > > #ifndef REPORT_UTIL
> > > #define REPORT_UTIL "libhugetlbfs"
> > > #endif
> > > +
> > > +#define VERBOSE_ERROR 1
> > > +#define VERBOSE_WARNING 2
> > > +#define VERBOSE_INFO 3
> > > +#define VERBOSE_DEBUG 4
> > > +
> > > +#define __hugetlbfs_debug (__hugetlbfs_verbose >= VERBOSE_DEBUG)
> > > +
> > > #ifndef REPORT
> > > #define REPORT(level, prefix, format, ...) \
> > > do { \
> > > - if (__hugetlbfs_debug || __hugetlbfs_verbose >= level) { \
> > > - fprintf(stderr, REPORT_UTIL " [%s:%d]: " prefix ": " \
> > > - format, __hugetlbfs_hostname, getpid(), \
> > > + if (__hugetlbfs_verbose >= level) { \
> > > + fprintf(stderr, REPORT_UTIL); \
> > > + if (__hugetlbfs_verbose >= VERBOSE_DEBUG) \
> > > + fprintf(stderr, " [%s:%d]", \
> > > + __hugetlbfs_hostname, getpid()); \
> > > + fprintf(stderr, ": " prefix ": " format, \
> >
> > hmm, this is actually a semantic change. Previously, we always printed
> > the hostname and pid. Now, we only do it when verbosity is set to debug.
> > That might be confusing for parsers of logs for example and I'm failing
> > to see the benefit of the change. In addition, one fprintf is replaced
> > by three plus a branch. What is the thinking behind this change?
>
> The main reason for this change is that including the hostname and pid
> in every single message can make the resulting output pretty unreadable
> since the majority of each message is repetitive metadata. Since I
> recognize that the information can be useful when debugging problems, I
> chose to print it only in that verbosity level. Do you have any
> suggestions for making the check more efficient?
>
None that would ultimately a useful difference. It's not worth optimising
on the grounds we don't print out all that often. I'm happy with the
reasoning for the change.
> >
> > > ##__VA_ARGS__); \
> > > fflush(stderr); \
> > > } \
> > > @@ -103,7 +112,7 @@ extern long parse_page_size(const char *str);
> > >
> > > #define REPORT_CONT(level, prefix, ...) \
> > > do { \
> > > - if (__hugetlbfs_debug || __hugetlbfs_verbose >= level) { \
> > > + if (__hugetlbfs_verbose >= level) { \
> > > fprintf(stderr, ##__VA_ARGS__); \
> > > fflush(stderr); \
> > > } \
> > >
> > >
> >
> --
> Adam Litke - (agl at us.ibm.com)
> IBM Linux Technology Center
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Libhugetlbfs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
