[PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Adrian Reber]

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.

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
-- 
2.17.1

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Oleg Nesterov]

On 07/12, Adrian Reber wrote:
>
> 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.

Agreed.

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Hendrik Brueckner]

On Thu, Jul 12, 2018 at 03:07:33PM +0200, Adrian Reber wrote:
> 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.
> 
> 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

Thanks!

Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Alice Frosi]

On 12.07.2018 15:47, Hendrik Brueckner wrote:
> On Thu, Jul 12, 2018 at 03:07:33PM +0200, Adrian Reber wrote:
>> 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.
>>
>> 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
> Thanks!
>
> Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>

Reviewed-by: Alice Frosi <alice@linux.vnet.ibm.com>

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Eric W. Biederman]

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

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Pavel Emelyanov]

On 07/12/2018 04:07 PM, Adrian Reber wrote:
> 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.
> 
> 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>

Acked-by: Pavel Emelyanov <xemul@virtuozzo.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
> 

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Pavel Emelyanov]

On 07/12/2018 07:33 PM, Eric W. Biederman wrote:
> 
> 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?

Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail:

   However I'm less confident than the developers that it will all
   eventually work! So what I'm asking them to do is to wrap each piece
   of new code inside CONFIG_CHECKPOINT_RESTORE.  So if it all
   eventually comes to tears and the project as a whole fails, it should
   be a simple matter to go through and delete all trace of it.

the best link with full e-mail I googled for is
https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734

-- Pavel

> 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
> .
> 

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Eric W. Biederman]

Pavel Emelyanov <xemul@virtuozzo.com> writes:

> On 07/12/2018 07:33 PM, Eric W. Biederman wrote:
>> 
>> 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?
>
> Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail:
>
>    However I'm less confident than the developers that it will all
>    eventually work! So what I'm asking them to do is to wrap each piece
>    of new code inside CONFIG_CHECKPOINT_RESTORE.  So if it all
>    eventually comes to tears and the project as a whole fails, it should
>    be a simple matter to go through and delete all trace of it.
>
> the best link with full e-mail I googled for is
> https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734

Good explanation.  Thank you.

At this point we even have not CRIU users of some of the pieces.
The project as a whole has not failed.

The code is old enough an common enough (enabled in some distros) that
we need to do the whole watch out for regressions if we remove any part
of it.

Which is a long way of saying the original justifiction for
CONFIG_CHECKPOINT_RESTORE is gone.  So please let's remove the entire
config option and simplify everyone's lives who has to test this stuff.

Unless someone can come up with a justification for keeping some of this
behind a config option.

Eric

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Adrian Reber]

On Fri, Jul 13, 2018 at 08:46:25AM -0500, Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@virtuozzo.com> writes:
> 
> > On 07/12/2018 07:33 PM, Eric W. Biederman wrote:
> >> 
> >> 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?
> >
> > Sure! Quoting Andrew's ~7 years ago akpm branch merge e-mail:
> >
> >    However I'm less confident than the developers that it will all
> >    eventually work! So what I'm asking them to do is to wrap each piece
> >    of new code inside CONFIG_CHECKPOINT_RESTORE.  So if it all
> >    eventually comes to tears and the project as a whole fails, it should
> >    be a simple matter to go through and delete all trace of it.
> >
> > the best link with full e-mail I googled for is
> > https://gitlab.imag.fr/kaunetem/linux-kaunetem/commit/099469502f62fbe0d7e4f0b83a2f22538367f734
> 
> Good explanation.  Thank you.
> 
> At this point we even have not CRIU users of some of the pieces.
> The project as a whole has not failed.
> 
> The code is old enough an common enough (enabled in some distros) that
> we need to do the whole watch out for regressions if we remove any part
> of it.
> 
> Which is a long way of saying the original justifiction for
> CONFIG_CHECKPOINT_RESTORE is gone.  So please let's remove the entire
> config option and simplify everyone's lives who has to test this stuff.

Sounds good.

> Unless someone can come up with a justification for keeping some of this
> behind a config option.

I can provide a patch removing CONFIG_CHECKPOINT_RESTORE if there are no
further objections against it.

		Adrian

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Andrew Morton]

On Thu, 12 Jul 2018 11:33:33 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:

> 
> 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?

Because at the time it was quite unclear that the overall project would
produce a viable result.  But the code is splattered over so many
places that getting it all going out-of-tree was impractical.  So the
#ifdef CONFIG_CHECKPOINT_RESTORE markers were initially there to a)
identify places where we should go if we decide to rip the whole thing
out and to b) ensure that the kernel would still compile and work OK
after that removal.

This caution eventually proved to be unnecessary.

> Why this option was placed under expert?

Dunno.

> What is the value of disabling this functionality ever?
> 
> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
> entirely?

For the vast number of Linux machines which aren't servers?  Check out
some defconfigs - only one of arm's 119 defconfigs selects it.

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Kees Cook]

On Fri, Jul 13, 2018 at 1:55 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 12 Jul 2018 11:33:33 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:
>> What is the value of disabling this functionality ever?
>>
>> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
>> entirely?
>
> For the vast number of Linux machines which aren't servers?  Check out
> some defconfigs - only one of arm's 119 defconfigs selects it.

Right, and I would bet the minification folks would like to keep it
out of their builds too. I think we should keep the config.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Josh Triplett]

On Fri, Jul 13, 2018 at 09:44:15PM -0700, Kees Cook wrote:
> On Fri, Jul 13, 2018 at 1:55 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Thu, 12 Jul 2018 11:33:33 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:
> >> What is the value of disabling this functionality ever?
> >>
> >> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
> >> entirely?
> >
> > For the vast number of Linux machines which aren't servers?  Check out
> > some defconfigs - only one of arm's 119 defconfigs selects it.
> 
> Right, and I would bet the minification folks would like to keep it
> out of their builds too. I think we should keep the config.

Thank you, Kees.  Yes, please.

- Josh Triplett

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jul 13, 2018 at 1:55 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Thu, 12 Jul 2018 11:33:33 -0500 ebiederm@xmission.com (Eric W. Biederman) wrote:
>>> What is the value of disabling this functionality ever?
>>>
>>> Is there any reason why we don't just delete CONFIG_CHECKPOINT_RESTORE
>>> entirely?
>>
>> For the vast number of Linux machines which aren't servers?  Check out
>> some defconfigs - only one of arm's 119 defconfigs selects it.
>
> Right, and I would bet the minification folks would like to keep it
> out of their builds too. I think we should keep the config.

I take it then you are volunteering to test with and without the config
option?

Even if the config option is kept I intend to rip it out every time I
wind up touching code with it in.  Config options have a real cost in
testing and development.

For a config option that no one has come forward with an actual real
world use case for disabling, that cost seems much too high.

Eric

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Josh Triplett]

On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
> For a config option that no one has come forward with an actual real
> world use case for disabling, that cost seems much too high.

The real-world use case is precisely as stated: code size, both storage
and RAM.

I regularly encounter systems I'd *like* to put Linux in that have
around 1MB of storage and 1MB of RAM, or even less.

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Eric W. Biederman]

Josh Triplett <josh@joshtriplett.org> writes:

> On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
>> For a config option that no one has come forward with an actual real
>> world use case for disabling, that cost seems much too high.
>
> The real-world use case is precisely as stated: code size, both storage
> and RAM.

That is theoretical.  Which platform will break or feel distressed if we
make it unconditional.  That is real world.

> I regularly encounter systems I'd *like* to put Linux in that have
> around 1MB of storage and 1MB of RAM, or even less.

Yes.  There is so little code behind CONFIG_CHECKPOINT_RESTART that it
won't help with that.

But if minification is the actual requirement for disabling
CONFIG_CHECKPOINT_RESTART than CONFIG_CHECKPIONT_RESTART is properly
behind expert and it needs to be default y instead of default n.

Eric

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Josh Triplett]

On Sat, Jul 14, 2018 at 02:39:24PM -0500, Eric W. Biederman wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
> >> For a config option that no one has come forward with an actual real
> >> world use case for disabling, that cost seems much too high.
> >
> > The real-world use case is precisely as stated: code size, both storage
> > and RAM.
> 
> That is theoretical.

No, it isn't. I've *watched* the kernel's size trend steadily upward
over time. And it largely happens in individual features that don't
think *their* contribution to size is all that large.

> > I regularly encounter systems I'd *like* to put Linux in that have
> > around 1MB of storage and 1MB of RAM, or even less.
> 
> Yes.  There is so little code behind CONFIG_CHECKPOINT_RESTART that it
> won't help with that.

It adds up; there are hundreds more small features like it.

> But if minification is the actual requirement for disabling
> CONFIG_CHECKPOINT_RESTART than CONFIG_CHECKPIONT_RESTART is properly
> behind expert and it needs to be default y instead of default n.

I don't have any objection to *that*, as long as the option remains.

Re: [PATCH] kconfig: remove EXPERT from CHECKPOINT_RESTORE

[Cyrill Gorcunov]

On Sat, Jul 14, 2018 at 02:39:24PM -0500, Eric W. Biederman wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > On Sat, Jul 14, 2018 at 02:04:46PM -0500, Eric W. Biederman wrote:
> >> For a config option that no one has come forward with an actual real
> >> world use case for disabling, that cost seems much too high.
> >
> > The real-world use case is precisely as stated: code size, both storage
> > and RAM.
> 
> That is theoretical.  Which platform will break or feel distressed if we
> make it unconditional.  That is real world.
> 
> > I regularly encounter systems I'd *like* to put Linux in that have
> > around 1MB of storage and 1MB of RAM, or even less.
> 
> Yes.  There is so little code behind CONFIG_CHECKPOINT_RESTART that it
> won't help with that.
> 
> But if minification is the actual requirement for disabling
> CONFIG_CHECKPOINT_RESTART than CONFIG_CHECKPIONT_RESTART is properly
> behind expert and it needs to be default y instead of default n.

I happened to miss this thread, sorry. So as far as I remember it
was me who introduced this option in first place, and initially
I placed it under expert so it won't be enabled by default. Lately
we found that some of functionality introduced for criu sake actually
pretty convenient for other tools (for example vmflags reported in
procfs) so we dropped CONFIG_ option out for such blocks and merged
them into kernel directly. I won't mind if left is merged into the
kernel, there should not be that many places.