Re: [PATCH] Enhance checkpoint/restart Kconfig options

Re: [PATCH] Enhance checkpoint/restart Kconfig options

[Nathan Lynch]

Dave Hansen <dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:

> I spent way too much time yesterday trying to figure out why
> my c/r tests weren't running.  It turned out that I didn't
> have any of the namespaces enabled, the sys_unshare() was
> failing, and the debugging didn't explain any of this.
>
> So, to protect the world from repeating my mistakes, let's
> improve the Kconfig options a bit.
>
> 1. Don't say "Enable" in the c/r menu, that's just silly
> 2. move the c/r menu next to the namespaces
> 3. make CHECKPOINT select all of the namespaces
> 4. put a Kconfig 'comment' in explaining how everything
>    got selected.
> 5. Put the namespaces in their own menu
>
> Perhaps I should break this up a bit and we can push some
> of this to the beginning of the series.  Thoughts?
>
> ---
>
>  linux-2.6.git-dave/checkpoint/Kconfig |    7 ++++++-
>  linux-2.6.git-dave/init/Kconfig       |    9 ++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff -puN checkpoint/Kconfig~cr-kconfig-fixup checkpoint/Kconfig
> --- linux-2.6.git/checkpoint/Kconfig~cr-kconfig-fixup	2009-06-04 11:38:57.000000000 -0700
> +++ linux-2.6.git-dave/checkpoint/Kconfig	2009-06-04 11:38:57.000000000 -0700
> @@ -7,9 +7,14 @@ config DEFERQUEUE
>  	default n
>  
>  config CHECKPOINT
> -	bool "Enable checkpoint/restart (EXPERIMENTAL)"
> +	bool "Checkpoint/restart (EXPERIMENTAL)"
>  	depends on CHECKPOINT_SUPPORT && EXPERIMENTAL
>  	select DEFERQUEUE
> +	select UTS_NS
> +	select IPC_NS
> +	select NET_NS
> +	select PID_NS
> +	select USER_NS

*_NS all depend on NAMESPACES at the very least; some have additional
dependencies.  Selecting a symbol without ensuring its dependencies are
enabled is one way to break a build.

Also - checkpoint is useful without *_NS, no?  Forcing these things on
seems a little user-hostile..

Re: [PATCH] Enhance checkpoint/restart Kconfig options

[Dave Hansen]

On Thu, 2009-06-04 at 15:01 -0500, Nathan Lynch wrote:
> Dave Hansen <dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:
> > diff -puN checkpoint/Kconfig~cr-kconfig-fixup checkpoint/Kconfig
> > --- linux-2.6.git/checkpoint/Kconfig~cr-kconfig-fixup	2009-06-04 11:38:57.000000000 -0700
> > +++ linux-2.6.git-dave/checkpoint/Kconfig	2009-06-04 11:38:57.000000000 -0700
> > @@ -7,9 +7,14 @@ config DEFERQUEUE
> >  	default n
> >  
> >  config CHECKPOINT
> > -	bool "Enable checkpoint/restart (EXPERIMENTAL)"
> > +	bool "Checkpoint/restart (EXPERIMENTAL)"
> >  	depends on CHECKPOINT_SUPPORT && EXPERIMENTAL
> >  	select DEFERQUEUE
> > +	select UTS_NS
> > +	select IPC_NS
> > +	select NET_NS
> > +	select PID_NS
> > +	select USER_NS
> 
> *_NS all depend on NAMESPACES at the very least; some have additional
> dependencies.  Selecting a symbol without ensuring its dependencies are
> enabled is one way to break a build.

Ahh, but there *isn't* a strict build dependency on them.  We just want
to make sure that if they're available that we enable them.  If we have
CONFIG_NET=n, then we certainly don't want to make CHECKPOINT depends on
it.  

> Also - checkpoint is useful without *_NS, no?  Forcing these things on
> seems a little user-hostile..

I think the hostile thing is trying to completely unshare all the
namespaces without checking to see which ones are enabled, first. ;)

-- Dave

Re: [PATCH] Enhance checkpoint/restart Kconfig options

[Oren Laadan]

Dave Hansen wrote:
> On Thu, 2009-06-04 at 15:01 -0500, Nathan Lynch wrote:
>> Dave Hansen <dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:
>>> diff -puN checkpoint/Kconfig~cr-kconfig-fixup checkpoint/Kconfig
>>> --- linux-2.6.git/checkpoint/Kconfig~cr-kconfig-fixup	2009-06-04 11:38:57.000000000 -0700
>>> +++ linux-2.6.git-dave/checkpoint/Kconfig	2009-06-04 11:38:57.000000000 -0700
>>> @@ -7,9 +7,14 @@ config DEFERQUEUE
>>>  	default n
>>>  
>>>  config CHECKPOINT
>>> -	bool "Enable checkpoint/restart (EXPERIMENTAL)"
>>> +	bool "Checkpoint/restart (EXPERIMENTAL)"
>>>  	depends on CHECKPOINT_SUPPORT && EXPERIMENTAL
>>>  	select DEFERQUEUE
>>> +	select UTS_NS
>>> +	select IPC_NS
>>> +	select NET_NS
>>> +	select PID_NS
>>> +	select USER_NS
>> *_NS all depend on NAMESPACES at the very least; some have additional
>> dependencies.  Selecting a symbol without ensuring its dependencies are
>> enabled is one way to break a build.
> 
> Ahh, but there *isn't* a strict build dependency on them.  We just want
> to make sure that if they're available that we enable them.  If we have
> CONFIG_NET=n, then we certainly don't want to make CHECKPOINT depends on
> it.  
> 
>> Also - checkpoint is useful without *_NS, no?  Forcing these things on
>> seems a little user-hostile..

I concur.

> 
> I think the hostile thing is trying to completely unshare all the
> namespaces without checking to see which ones are enabled, first. ;)

I concur again :o

Will fix that.

Oren.

Re: [PATCH] Enhance checkpoint/restart Kconfig options

[Nathan Lynch]

Dave Hansen <dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:
> On Thu, 2009-06-04 at 15:01 -0500, Nathan Lynch wrote:
>> Dave Hansen <dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> writes:
>> > diff -puN checkpoint/Kconfig~cr-kconfig-fixup checkpoint/Kconfig
>> > --- linux-2.6.git/checkpoint/Kconfig~cr-kconfig-fixup	2009-06-04 11:38:57.000000000 -0700
>> > +++ linux-2.6.git-dave/checkpoint/Kconfig	2009-06-04 11:38:57.000000000 -0700
>> > @@ -7,9 +7,14 @@ config DEFERQUEUE
>> >  	default n
>> >  
>> >  config CHECKPOINT
>> > -	bool "Enable checkpoint/restart (EXPERIMENTAL)"
>> > +	bool "Checkpoint/restart (EXPERIMENTAL)"
>> >  	depends on CHECKPOINT_SUPPORT && EXPERIMENTAL
>> >  	select DEFERQUEUE
>> > +	select UTS_NS
>> > +	select IPC_NS
>> > +	select NET_NS
>> > +	select PID_NS
>> > +	select USER_NS
>> 
>> *_NS all depend on NAMESPACES at the very least; some have additional
>> dependencies.  Selecting a symbol without ensuring its dependencies are
>> enabled is one way to break a build.
>
> Ahh, but there *isn't* a strict build dependency on them.  We just want
> to make sure that if they're available that we enable them.  If we have
> CONFIG_NET=n, then we certainly don't want to make CHECKPOINT depends on
> it.

Sorry, I'm afraid I don't understand what you're saying here.

Here's a concrete example of what I'm talking about.

$ git checkout oren/ckpt-v16
$ make ARCH=i386 i386_defconfig
[ edit .config, enable CONFIG_CHECKPOINT and disable CONFIG_NET ]
$ make ARCH=i386 -s vmlinux   # which should succeed
[ apply your patch ]
$ make ARCH=i386 -s vmlinux
kernel/built-in.o: In function `free_nsproxy':
(.text+0x1daaf): undefined reference to `__put_net'
make: *** [.tmp_vmlinux1] Error 1

Re: [PATCH] Enhance checkpoint/restart Kconfig options

[Dave Hansen]

On Thu, 2009-06-04 at 23:25 -0500, Nathan Lynch wrote:
> > Ahh, but there *isn't* a strict build dependency on them.  We just
> want
> > to make sure that if they're available that we enable them.  If we
> have
> > CONFIG_NET=n, then we certainly don't want to make CHECKPOINT
> depends on
> > it.
> 
> Sorry, I'm afraid I don't understand what you're saying here.
> 
> Here's a concrete example of what I'm talking about.
> 
> $ git checkout oren/ckpt-v16
> $ make ARCH=i386 i386_defconfig
> [ edit .config, enable CONFIG_CHECKPOINT and disable CONFIG_NET ]
> $ make ARCH=i386 -s vmlinux   # which should succeed
> [ apply your patch ]
> $ make ARCH=i386 -s vmlinux
> kernel/built-in.o: In function `free_nsproxy':
> (.text+0x1daaf): undefined reference to `__put_net'
> make: *** [.tmp_vmlinux1] Error 1

Kconfig is almost as retarded as me. :)  Thanks for the cluebat.

-- Dave

Re: [PATCH] Enhance checkpoint/restart Kconfig options

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> 
> I spent way too much time yesterday trying to figure out why
> my c/r tests weren't running.  It turned out that I didn't
> have any of the namespaces enabled, the sys_unshare() was
> failing, and the debugging didn't explain any of this.
> 
> So, to protect the world from repeating my mistakes, let's
> improve the Kconfig options a bit.
> 
> 1. Don't say "Enable" in the c/r menu, that's just silly
> 2. move the c/r menu next to the namespaces
> 3. make CHECKPOINT select all of the namespaces
> 4. put a Kconfig 'comment' in explaining how everything
>    got selected.
> 5. Put the namespaces in their own menu
> 
> Perhaps I should break this up a bit and we can push some
> of this to the beginning of the series.  Thoughts?

Yes the selects are needed imo.

-serge