> James,
> 
> If I understand correctly, you're saying you want to be able to build without 
> debug support...?  I'm not convinced that building a client without debug 
> support is interesting or useful.  In fact, I think it would be harmful, and 
> we shouldn't open up the possibility - this is switchable debug with very low 
> overhead when not actually "on".  It would be really awful to get a problem 
> on a running system and discover there's no debug support - that you can't 
> even enable debug without a reinstall.
> 
> If I've understood you correctly, then I would want to see proof of a 
> significant performance cost when debug is built but *off* before agreeing to 
> even exposing this option.  (I know it's a choice they'd have to make, but if 
> it's not really useful with a side order of potentially harmful, we shouldn't 
> even give people the choice.)

I'm not saying add the option today but this is more for the long game.
While the Intel lustre developers deeply love lustre's debugging 
infrastructure I see a future where something better will come along to
replace it. When that day comes we will have a period where both
debugging infrastructurs will exist and some deployers of lustre will
want to turn off the old debugging infrastructure and just use the new.
That is what I have in mind. A switch to flip between options.

> - Patrick
> 
> ´╗┐On 4/15/18, 10:49 PM, "lustre-devel on behalf of James Simmons" 
> <lustre-devel-boun...@lists.lustre.org on behalf of jsimm...@infradead.org> 
> wrote:
> 
>     
>     > CDEBUG_STACK() and CHECK_STACK() are macros to help with
>     > debugging, so move them from
>     >    drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
>     > to
>     >    drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
>     > 
>     > This seems a more fitting location, and is a step towards
>     > removing linux/libcfs.h and simplifying the include file structure.
>     
>     Nak. Currently the lustre client always enables debugging but that
>     shouldn't be the case. What we do need is the able to turn off the 
>     crazy debugging stuff. In the development branch of lustre it is
>     done with CDEBUG_ENABLED. We need something like that in Kconfig
>     much like we have CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK. Since we like
>     to be able to turn that off this should be moved to just after
>     LIBCFS_DEBUG_MSG_DATA_DECL. Then from CHECK_STACK down to CWARN()
>     it can be build out. When CDEBUG_ENABLED is disabled CDEBUG_LIMIT
>     would be empty.
>      
>     > Signed-off-by: NeilBrown <ne...@suse.com>
>     > ---
>     >  .../lustre/include/linux/libcfs/libcfs_debug.h     |   32 
> ++++++++++++++++++++
>     >  .../lustre/include/linux/libcfs/linux/libcfs.h     |   31 
> -------------------
>     >  2 files changed, 32 insertions(+), 31 deletions(-)
>     > 
>     > diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h 
> b/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
>     > index 9290a19429e7..0dc7b91efe7c 100644
>     > --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
>     > +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h
>     > @@ -62,6 +62,38 @@ int libcfs_debug_str2mask(int *mask, const char 
> *str, int is_subsys);
>     >  extern unsigned int libcfs_catastrophe;
>     >  extern unsigned int libcfs_panic_on_lbug;
>     >  
>     > +/* Enable debug-checks on stack size - except on x86_64 */
>     > +#if !defined(__x86_64__)
>     > +# ifdef __ia64__
>     > +#  define CDEBUG_STACK() (THREAD_SIZE -                                
>  \
>     > +                         ((unsigned long)__builtin_dwarf_cfa() &       
> \
>     > +                          (THREAD_SIZE - 1)))
>     > +# else
>     > +#  define CDEBUG_STACK() (THREAD_SIZE -                                
>  \
>     > +                         ((unsigned long)__builtin_frame_address(0) &  
> \
>     > +                          (THREAD_SIZE - 1)))
>     > +# endif /* __ia64__ */
>     > +
>     > +#define __CHECK_STACK(msgdata, mask, cdls)                           \
>     > +do {                                                               \
>     > +       if (unlikely(CDEBUG_STACK() > libcfs_stack)) {            \
>     > +               LIBCFS_DEBUG_MSG_DATA_INIT(msgdata, D_WARNING, NULL);   
> \
>     > +               libcfs_stack = CDEBUG_STACK();                    \
>     > +               libcfs_debug_msg(msgdata,                              \
>     > +                                "maximum lustre stack %lu\n",    \
>     > +                                CDEBUG_STACK());                      \
>     > +               (msgdata)->msg_mask = mask;                          \
>     > +               (msgdata)->msg_cdls = cdls;                          \
>     > +               dump_stack();                                      \
>     > +             /*panic("LBUG");*/                                        
> \
>     > +       }                                                              \
>     > +} while (0)
>     > +#define CFS_CHECK_STACK(msgdata, mask, cdls)  __CHECK_STACK(msgdata, 
> mask, cdls)
>     > +#else /* __x86_64__ */
>     > +#define CFS_CHECK_STACK(msgdata, mask, cdls) do {} while (0)
>     > +#define CDEBUG_STACK() (0L)
>     > +#endif /* __x86_64__ */
>     > +
>     >  #ifndef DEBUG_SUBSYSTEM
>     >  # define DEBUG_SUBSYSTEM S_UNDEFINED
>     >  #endif
>     > diff --git a/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h 
> b/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
>     > index 07d3cb2217d1..83aec9c7698f 100644
>     > --- a/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
>     > +++ b/drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h
>     > @@ -80,35 +80,4 @@
>     >  #include <stdarg.h>
>     >  #include "linux-cpu.h"
>     >  
>     > -#if !defined(__x86_64__)
>     > -# ifdef __ia64__
>     > -#  define CDEBUG_STACK() (THREAD_SIZE -                                
>  \
>     > -                         ((unsigned long)__builtin_dwarf_cfa() &       
> \
>     > -                          (THREAD_SIZE - 1)))
>     > -# else
>     > -#  define CDEBUG_STACK() (THREAD_SIZE -                                
>  \
>     > -                         ((unsigned long)__builtin_frame_address(0) &  
> \
>     > -                          (THREAD_SIZE - 1)))
>     > -# endif /* __ia64__ */
>     > -
>     > -#define __CHECK_STACK(msgdata, mask, cdls)                           \
>     > -do {                                                               \
>     > -       if (unlikely(CDEBUG_STACK() > libcfs_stack)) {            \
>     > -               LIBCFS_DEBUG_MSG_DATA_INIT(msgdata, D_WARNING, NULL);   
> \
>     > -               libcfs_stack = CDEBUG_STACK();                    \
>     > -               libcfs_debug_msg(msgdata,                              \
>     > -                                "maximum lustre stack %lu\n",    \
>     > -                                CDEBUG_STACK());                      \
>     > -               (msgdata)->msg_mask = mask;                          \
>     > -               (msgdata)->msg_cdls = cdls;                          \
>     > -               dump_stack();                                      \
>     > -             /*panic("LBUG");*/                                        
> \
>     > -       }                                                              \
>     > -} while (0)
>     > -#define CFS_CHECK_STACK(msgdata, mask, cdls)  __CHECK_STACK(msgdata, 
> mask, cdls)
>     > -#else /* __x86_64__ */
>     > -#define CFS_CHECK_STACK(msgdata, mask, cdls) do {} while (0)
>     > -#define CDEBUG_STACK() (0L)
>     > -#endif /* __x86_64__ */
>     > -
>     >  #endif /* _LINUX_LIBCFS_H */
>     > 
>     > 
>     > 
>     _______________________________________________
>     lustre-devel mailing list
>     lustre-de...@lists.lustre.org
>     http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
>     
> 
> 

Reply via email to