[PATCH] C/R: simplify checkpoint debugging

[PATCH] C/R: simplify checkpoint debugging

[Nathan Lynch]

To enable printing of checkpoint/restart messages, one must enable
CHECKPOINT_DEBUG and boot with ckpt_debug= ... something I haven't
been able to figure out.  Further complicating matters is that if
DYNAMIC_DEBUG is enabled, you still won't get any debugging output
unless you manually enable debug output for all the functions you're
interested in via <debugfs>/dynamic_debug/control.  At this stage of
development most users will want all C/R messages either on or off and
don't need a fancy filtering mechanism.

Change ckpt_debug to print messages if CHECKPOINT_DEBUG=y.  Otherwise
it will emit nothing.  Remove ckpt_debug= setup code.  Reword
CHECKPOINT_DEBUG config help text appropriately.

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 checkpoint/sys.c           |   22 ----------------------
 include/linux/checkpoint.h |    4 +---
 lib/Kconfig.debug          |    9 +++------
 3 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index 38a5299..3c0056f 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -338,25 +338,3 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int, fd, unsigned long, flags)
 	ckpt_ctx_put(ctx);
 	return ret;
 }
-
-
-/* 'ckpt_debug_level' controls the verbosity level of c/r code */
-#ifdef CONFIG_CHECKPOINT_DEBUG
-
-/* FIX: allow to change during runtime */
-unsigned long __read_mostly ckpt_debug_level = CKPT_DDEFAULT;
-
-static __init int ckpt_debug_setup(char *s)
-{
-	long val, ret;
-
-	ret = strict_strtoul(s, 10, &val);
-	if (ret < 0)
-		return ret;
-	ckpt_debug_level = val;
-	return 0;
-}
-
-__setup("ckpt_debug=", ckpt_debug_setup);
-
-#endif /* CONFIG_CHECKPOINT_DEBUG */
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index b92bf95..f1aad34 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -264,13 +264,11 @@ extern int restore_memory_contents(struct ckpt_ctx *ctx, struct inode *inode);
 #endif
 
 #ifdef CONFIG_CHECKPOINT_DEBUG
-extern unsigned long ckpt_debug_level;
 
 /* use this to select a specific debug level */
 #define _ckpt_debug(level, fmt, args...)			\
 	do {						\
-		if (ckpt_debug_level & (level))		\
-			pr_debug("[%d:c/r:%s] " fmt,	\
+		printk(KERN_DEBUG "[%d:c/r:%s] " fmt,	\
 				task_pid_vnr(current),	\
 				 __func__, ## args);	\
 	} while (0)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ab795a6..2f6556b 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -966,13 +966,10 @@ config CHECKPOINT_DEBUG
 	depends on CHECKPOINT
 	default y
 	help
-	  This options turns on the debugging output of checkpoint/restart.
-	  The level of verbosity is controlled by 'ckpt_debug_level' and can
-	  be set at boot time with "ckpt_debug=" option.
+	  If enabled, debugging messages related to checkpoint and
+	  restart operations will be printed to the kernel log.
 
-	  Turning this option off will reduce the size of the c/r code. If
-	  turned on, it is unlikely to incur visible overhead if the debug
-	  level is set to zero.
+	  Turn this on if you plan to test checkpoint/restart.
 
 source "samples/Kconfig"
 
-- 
1.6.0.6

Re: [PATCH] C/R: simplify checkpoint debugging

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> To enable printing of checkpoint/restart messages, one must enable
> CHECKPOINT_DEBUG and boot with ckpt_debug= ... something I haven't
> been able to figure out.  Further complicating matters is that if
> DYNAMIC_DEBUG is enabled, you still won't get any debugging output
> unless you manually enable debug output for all the functions you're
> interested in via <debugfs>/dynamic_debug/control.  At this stage of
> development most users will want all C/R messages either on or off and
> don't need a fancy filtering mechanism.
> 
> Change ckpt_debug to print messages if CHECKPOINT_DEBUG=y.  Otherwise
> it will emit nothing.  Remove ckpt_debug= setup code.  Reword
> CHECKPOINT_DEBUG config help text appropriately.
> 
> Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>

Yes please!

-serge

> ---
>  checkpoint/sys.c           |   22 ----------------------
>  include/linux/checkpoint.h |    4 +---
>  lib/Kconfig.debug          |    9 +++------
>  3 files changed, 4 insertions(+), 31 deletions(-)
> 
> diff --git a/checkpoint/sys.c b/checkpoint/sys.c
> index 38a5299..3c0056f 100644
> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -338,25 +338,3 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int, fd, unsigned long, flags)
>  	ckpt_ctx_put(ctx);
>  	return ret;
>  }
> -
> -
> -/* 'ckpt_debug_level' controls the verbosity level of c/r code */
> -#ifdef CONFIG_CHECKPOINT_DEBUG
> -
> -/* FIX: allow to change during runtime */
> -unsigned long __read_mostly ckpt_debug_level = CKPT_DDEFAULT;
> -
> -static __init int ckpt_debug_setup(char *s)
> -{
> -	long val, ret;
> -
> -	ret = strict_strtoul(s, 10, &val);
> -	if (ret < 0)
> -		return ret;
> -	ckpt_debug_level = val;
> -	return 0;
> -}
> -
> -__setup("ckpt_debug=", ckpt_debug_setup);
> -
> -#endif /* CONFIG_CHECKPOINT_DEBUG */
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index b92bf95..f1aad34 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -264,13 +264,11 @@ extern int restore_memory_contents(struct ckpt_ctx *ctx, struct inode *inode);
>  #endif
> 
>  #ifdef CONFIG_CHECKPOINT_DEBUG
> -extern unsigned long ckpt_debug_level;
> 
>  /* use this to select a specific debug level */
>  #define _ckpt_debug(level, fmt, args...)			\
>  	do {						\
> -		if (ckpt_debug_level & (level))		\
> -			pr_debug("[%d:c/r:%s] " fmt,	\
> +		printk(KERN_DEBUG "[%d:c/r:%s] " fmt,	\
>  				task_pid_vnr(current),	\
>  				 __func__, ## args);	\
>  	} while (0)
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ab795a6..2f6556b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -966,13 +966,10 @@ config CHECKPOINT_DEBUG
>  	depends on CHECKPOINT
>  	default y
>  	help
> -	  This options turns on the debugging output of checkpoint/restart.
> -	  The level of verbosity is controlled by 'ckpt_debug_level' and can
> -	  be set at boot time with "ckpt_debug=" option.
> +	  If enabled, debugging messages related to checkpoint and
> +	  restart operations will be printed to the kernel log.
> 
> -	  Turning this option off will reduce the size of the c/r code. If
> -	  turned on, it is unlikely to incur visible overhead if the debug
> -	  level is set to zero.
> +	  Turn this on if you plan to test checkpoint/restart.
> 
>  source "samples/Kconfig"
> 
> -- 
> 1.6.0.6
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] C/R: simplify checkpoint debugging

[Dan Smith]

SH> Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
>> To enable printing of checkpoint/restart messages, one must enable
>> CHECKPOINT_DEBUG and boot with ckpt_debug= ... something I haven't
>> been able to figure out.  Further complicating matters is that if
>> DYNAMIC_DEBUG is enabled, you still won't get any debugging output
>> unless you manually enable debug output for all the functions you're
>> interested in via <debugfs>/dynamic_debug/control.  At this stage of
>> development most users will want all C/R messages either on or off and
>> don't need a fancy filtering mechanism.
>> 
>> Change ckpt_debug to print messages if CHECKPOINT_DEBUG=y.  Otherwise
>> it will emit nothing.  Remove ckpt_debug= setup code.  Reword
>> CHECKPOINT_DEBUG config help text appropriately.
>> 
>> Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>

SH> Yes please!

Enthusiastically seconded.  Or thirded, or whatever it would be.

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH] C/R: simplify checkpoint debugging

[Oren Laadan]

Nathan Lynch wrote:
> To enable printing of checkpoint/restart messages, one must enable
> CHECKPOINT_DEBUG and boot with ckpt_debug= ... something I haven't
> been able to figure out.  Further complicating matters is that if
> DYNAMIC_DEBUG is enabled, you still won't get any debugging output
> unless you manually enable debug output for all the functions you're
> interested in via <debugfs>/dynamic_debug/control.  At this stage of
> development most users will want all C/R messages either on or off and
> don't need a fancy filtering mechanism.

Thanks for bringing this up. In part it has been annoying to me too.

I'm all for replacing pr_debug(...) with printk(KERN_DEBUG ...).
Done.

I prefer to keep the selection of debug verbosity in - it becomes
useful when debugging a checkpoint or restart that produce plenty
of output. It keeps the noise (and time) to an acceptable level.

So I did also change the default (CKPT_DDEFAULT) to select all
debug flags. So the normal behavior is as intended in this patch.

Given the enthusiastic responses to the proposed changes, do you
still wish to remove the option to select verbosity level ?

Oren.


> Change ckpt_debug to print messages if CHECKPOINT_DEBUG=y.  Otherwise
> it will emit nothing.  Remove ckpt_debug= setup code.  Reword
> CHECKPOINT_DEBUG config help text appropriately.
> 
> Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> ---
>  checkpoint/sys.c           |   22 ----------------------
>  include/linux/checkpoint.h |    4 +---
>  lib/Kconfig.debug          |    9 +++------
>  3 files changed, 4 insertions(+), 31 deletions(-)
> 
> diff --git a/checkpoint/sys.c b/checkpoint/sys.c
> index 38a5299..3c0056f 100644
> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -338,25 +338,3 @@ SYSCALL_DEFINE3(restart, pid_t, pid, int, fd, unsigned long, flags)
>  	ckpt_ctx_put(ctx);
>  	return ret;
>  }
> -
> -
> -/* 'ckpt_debug_level' controls the verbosity level of c/r code */
> -#ifdef CONFIG_CHECKPOINT_DEBUG
> -
> -/* FIX: allow to change during runtime */
> -unsigned long __read_mostly ckpt_debug_level = CKPT_DDEFAULT;
> -
> -static __init int ckpt_debug_setup(char *s)
> -{
> -	long val, ret;
> -
> -	ret = strict_strtoul(s, 10, &val);
> -	if (ret < 0)
> -		return ret;
> -	ckpt_debug_level = val;
> -	return 0;
> -}
> -
> -__setup("ckpt_debug=", ckpt_debug_setup);
> -
> -#endif /* CONFIG_CHECKPOINT_DEBUG */
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index b92bf95..f1aad34 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -264,13 +264,11 @@ extern int restore_memory_contents(struct ckpt_ctx *ctx, struct inode *inode);
>  #endif
>  
>  #ifdef CONFIG_CHECKPOINT_DEBUG
> -extern unsigned long ckpt_debug_level;
>  
>  /* use this to select a specific debug level */
>  #define _ckpt_debug(level, fmt, args...)			\
>  	do {						\
> -		if (ckpt_debug_level & (level))		\
> -			pr_debug("[%d:c/r:%s] " fmt,	\
> +		printk(KERN_DEBUG "[%d:c/r:%s] " fmt,	\
>  				task_pid_vnr(current),	\
>  				 __func__, ## args);	\
>  	} while (0)
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ab795a6..2f6556b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -966,13 +966,10 @@ config CHECKPOINT_DEBUG
>  	depends on CHECKPOINT
>  	default y
>  	help
> -	  This options turns on the debugging output of checkpoint/restart.
> -	  The level of verbosity is controlled by 'ckpt_debug_level' and can
> -	  be set at boot time with "ckpt_debug=" option.
> +	  If enabled, debugging messages related to checkpoint and
> +	  restart operations will be printed to the kernel log.
>  
> -	  Turning this option off will reduce the size of the c/r code. If
> -	  turned on, it is unlikely to incur visible overhead if the debug
> -	  level is set to zero.
> +	  Turn this on if you plan to test checkpoint/restart.
>  
>  source "samples/Kconfig"
>  

Re: [PATCH] C/R: simplify checkpoint debugging

[Nathan Lynch]

Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:

> Nathan Lynch wrote:
>> To enable printing of checkpoint/restart messages, one must enable
>> CHECKPOINT_DEBUG and boot with ckpt_debug= ... something I haven't
>> been able to figure out.  Further complicating matters is that if
>> DYNAMIC_DEBUG is enabled, you still won't get any debugging output
>> unless you manually enable debug output for all the functions you're
>> interested in via <debugfs>/dynamic_debug/control.  At this stage of
>> development most users will want all C/R messages either on or off and
>> don't need a fancy filtering mechanism.
>
> Thanks for bringing this up. In part it has been annoying to me too.
>
> I'm all for replacing pr_debug(...) with printk(KERN_DEBUG ...).
> Done.
>
> I prefer to keep the selection of debug verbosity in - it becomes
> useful when debugging a checkpoint or restart that produce plenty
> of output. It keeps the noise (and time) to an acceptable level.
>
> So I did also change the default (CKPT_DDEFAULT) to select all
> debug flags. So the normal behavior is as intended in this patch.
>
> Given the enthusiastic responses to the proposed changes, do you
> still wish to remove the option to select verbosity level ?

If you add some documentation (in comments or kconfig help) on how to
use ckpt_debug= I wouldn't object.  AFAIK one has to read the code to
figure out which values to use.  Apologies if I've missed something...

In the long term, however, detailed reporting on C/R operations may be
better implemented using static tracepoints -- less runtime overhead
than printk, and (I think) they can be selectively enabled, which would
satisfy the speed/noise concerns you raise above.