Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

From: ebiederm@xmission.com (Eric W. Biederman)
To: Adrian Reber <adrian@lisas.de>
Cc: linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Pavel Emelyanov <xemul@virtuozzo.com>,
	Andrei Vagin <avagin@virtuozzo.com>,
	Hendrik Brueckner <brueckner@linux.vnet.ibm.com>,
	Cyrill Gorcunov <gorcunov@openvz.org>,
	Kees Cook <keescook@chromium.org>,
	Linux Containers <containers@lists.linux-foundation.org>
Subject: Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE
Date: Thu, 12 Jul 2018 11:33:33 -0500	[thread overview]
Message-ID: <87sh4o5s82.fsf@xmission.com> (raw)
In-Reply-To: <20180712130733.11510-1-adrian@lisas.de> (Adrian Reber's message of "Thu, 12 Jul 2018 15:07:33 +0200")

Adrian Reber <adrian@lisas.de> writes:

> The CHECKPOINT_RESTORE configuration option was introduced in 2012 and
> combined with EXPERT. CHECKPOINT_RESTORE is already enabled in many
> distribution kernels and also part of the defconfigs of various
> architectures.
>
> To make it easier for distributions to enable CHECKPOINT_RESTORE this
> removes EXPERT and moves the configuration option out of the EXPERT
> block.

I think we should change the help text at the same time, to match
our improve understanding of the situation.

Does anyone remember why this option was added at all?
Why this option was placed under expert?

What is the value of disabling this functionality ever?

Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
entirely?

Certainly we are at a point where distro's are enabling this so hiding
it behind CONFIG_EXPERT with a default of N seems inapparopriate.

The only thing I can imagine might be sensible is changing the default
to Y and leaving it behind CONFIG_EXPERT.

I want to know what is the point of maintaining all of the complexity of
the ifdefs.  If no one can come up with a reason I say let's just remove
CONFIG_CHECKPOINT_RESTORE entirely.

Eric

> Signed-off-by: Adrian Reber <adrian@lisas.de>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Pavel Emelyanov <xemul@virtuozzo.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Andrei Vagin <avagin@virtuozzo.com>
> Cc: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> ---
>  init/Kconfig | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 041f3a022122..9c529c763326 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -932,6 +932,18 @@ config NET_NS
>  
>  endif # NAMESPACES
>  
> +config CHECKPOINT_RESTORE
> +	bool "Checkpoint/restore support"
> +	select PROC_CHILDREN
> +	default n
> +	help
> +	  Enables additional kernel features in a sake of checkpoint/restore.
> +	  In particular it adds auxiliary prctl codes to setup process text,
> +	  data and heap segment sizes, and a few additional /proc filesystem
> +	  entries.
> +
> +	  If unsure, say N here.
> +
>  config SCHED_AUTOGROUP
>  	bool "Automatic process group scheduling"
>  	select CGROUPS
> @@ -1348,18 +1360,6 @@ config MEMBARRIER
>  
>  	  If unsure, say Y.
>  
> -config CHECKPOINT_RESTORE
> -	bool "Checkpoint/restore support" if EXPERT
> -	select PROC_CHILDREN
> -	default n
> -	help
> -	  Enables additional kernel features in a sake of checkpoint/restore.
> -	  In particular it adds auxiliary prctl codes to setup process text,
> -	  data and heap segment sizes, and a few additional /proc filesystem
> -	  entries.
> -
> -	  If unsure, say N here.
> -
>  config KALLSYMS
>  	 bool "Load all symbols for debugging/ksymoops" if EXPERT
>  	 default y