Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Dhaval Giani]

On Mon, Jun 01, 2009 at 11:02:58PM -0700, akpm@linux-foundation.org wrote:
> 
> The patch titled
>      cgroups: forbid noprefix if mounting more than just cpuset subsystem
> has been added to the -mm tree.  Its filename is
>      cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
> 
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> 
> ------------------------------------------------------
> Subject: cgroups: forbid noprefix if mounting more than just cpuset subsystem
> From: Li Zefan <lizf@cn.fujitsu.com>
> 
> The 'noprefix' option was introduced for backwards-compatibility of
> cpuset, but actually it can be used when mounting other subsystems.
> 
> This results in possibility of name collision, and now the collision can
> really happen, because we have 'stat' file in both memory and cpuacct
> subsystem:
> 
> 	# mount -t cgroup -o noprefix,memory,cpuacct xxx /mnt
> 
> Cgroup will happily mount the 2 subsystems, but only 'stat' file of memory
> subsys can be seen.
> 
> We don't want users to use nopreifx, and also want to avoid name
> collision, so we change to allow noprefix only if mounting just the cpuset
> subsystem.
> 

I am not sure if this is a good idea. For libcgroup, we would then be
adding a special case for just cpuset. I would rather that we allow it
either for all the subsystems or none of them.

thanks,
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> Cc: Paul Menage <menage@google.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Cc: Dhaval Giani <dhaval@linux.vnet.ibm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  kernel/cgroup.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff -puN kernel/cgroup.c~cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem kernel/cgroup.c
> --- a/kernel/cgroup.c~cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem
> +++ a/kernel/cgroup.c
> @@ -842,6 +842,11 @@ static int parse_cgroupfs_options(char *
>  				     struct cgroup_sb_opts *opts)
>  {
>  	char *token, *o = data ?: "all";
> +	unsigned long mask = (unsigned long)-1;
> +
> +#ifdef CONFIG_CPUSETS
> +	mask = ~(1 << cpuset_subsys_id);
> +#endif
> 
>  	opts->subsys_bits = 0;
>  	opts->flags = 0;
> @@ -886,6 +891,11 @@ static int parse_cgroupfs_options(char *
>  		}
>  	}
> 
> +	/* We allow noprefix only if mounting just the cpuset subsystem */
> +	if (test_bit(ROOT_NOPREFIX, &opts->flags) &&
> +	    (opts->subsys_bits & mask))
> +		return -EINVAL;
> +
>  	/* We can't have an empty hierarchy */
>  	if (!opts->subsys_bits)
>  		return -EINVAL;
> _
> 
> Patches currently in -mm which might be from lizf@cn.fujitsu.com are
> 
> origin.patch
> linux-next.patch
> mm-add-swap-cache-interface-for-swap-reference.patch
> mm-modify-swap_map-and-add-swap_has_cache-flag.patch
> mm-reuse-unused-swap-entry-if-necessary.patch
> hexdump-remove-the-trailing-space.patch
> cgroups-make-messages-more-readable.patch
> cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem.patch
> devcgroup-skip-superfluous-checks-when-found-the-dev_all-elem.patch
> memcg-remove-some-redundant-checks.patch
> memcg-remove-unneeded-forward-declaration-from-schedh.patch
> memcg-fix-swap-accounting.patch

-- 
regards,
Dhaval

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Paul Menage]

On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
>
> I am not sure if this is a good idea. For libcgroup, we would then be
> adding a special case for just cpuset. I would rather that we allow it
> either for all the subsystems or none of them.
>

libcgroup shouldn't be using the noprefix option. Its only intentded
use is to allow the legacy "cpuset" filesystem type to be mounted and
to see the same fileset as it had before the cgroups transition.

Paul

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Dhaval Giani]

On Tue, Jun 02, 2009 at 09:08:04AM -0700, Paul Menage wrote:
> On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> >
> > I am not sure if this is a good idea. For libcgroup, we would then be
> > adding a special case for just cpuset. I would rather that we allow it
> > either for all the subsystems or none of them.
> >
> 
> libcgroup shouldn't be using the noprefix option. Its only intentded
> use is to allow the legacy "cpuset" filesystem type to be mounted and
> to see the same fileset as it had before the cgroups transition.
> 

It does not. But if some user is using that option, we need to be in a
position to handle it.

I am quite happy not supporting the noprefix option in the library if it
is fine.

Thanks,
-- 
regards,
Dhaval

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Andrew Morton]

On Tue, 2 Jun 2009 23:10:42 +0530
Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:

> On Tue, Jun 02, 2009 at 09:08:04AM -0700, Paul Menage wrote:
> > On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> > >
> > > I am not sure if this is a good idea. For libcgroup, we would then be
> > > adding a special case for just cpuset. I would rather that we allow it
> > > either for all the subsystems or none of them.
> > >
> > 
> > libcgroup shouldn't be using the noprefix option. Its only intentded
> > use is to allow the legacy "cpuset" filesystem type to be mounted and
> > to see the same fileset as it had before the cgroups transition.
> > 
> 
> It does not. But if some user is using that option, we need to be in a
> position to handle it.
> 
> I am quite happy not supporting the noprefix option in the library if it
> is fine.
> 

fyi, the above discussion transitions akpm into the "confused" state. 
I'll keep the patch on hold until akpm transitions back out of that
state.

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Balbir Singh]

* Dhaval Giani <dhaval@linux.vnet.ibm.com> [2009-06-02 23:10:42]:

> On Tue, Jun 02, 2009 at 09:08:04AM -0700, Paul Menage wrote:
> > On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> > >
> > > I am not sure if this is a good idea. For libcgroup, we would then be
> > > adding a special case for just cpuset. I would rather that we allow it
> > > either for all the subsystems or none of them.
> > >
> > 
> > libcgroup shouldn't be using the noprefix option. Its only intentded
> > use is to allow the legacy "cpuset" filesystem type to be mounted and
> > to see the same fileset as it had before the cgroups transition.
> > 
> 
> It does not. But if some user is using that option, we need to be in a
> position to handle it.
> 
> I am quite happy not supporting the noprefix option in the library if it
> is fine.
>

I am not for supporting noprefix in the library, like Paul said it is
for legacy users who already have their applications working with
libcpuset or an equivalent library. libcgroup should not bother about
noprefix. 

-- 
	Balbir

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[KAMEZAWA Hiroyuki]

On Tue, 2 Jun 2009 13:35:34 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 2 Jun 2009 23:10:42 +0530
> Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Jun 02, 2009 at 09:08:04AM -0700, Paul Menage wrote:
> > > On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> > > >
> > > > I am not sure if this is a good idea. For libcgroup, we would then be
> > > > adding a special case for just cpuset. I would rather that we allow it
> > > > either for all the subsystems or none of them.
> > > >
> > > 
> > > libcgroup shouldn't be using the noprefix option. Its only intentded
> > > use is to allow the legacy "cpuset" filesystem type to be mounted and
> > > to see the same fileset as it had before the cgroups transition.
> > > 
> > 
> > It does not. But if some user is using that option, we need to be in a
> > position to handle it.
> > 
> > I am quite happy not supporting the noprefix option in the library if it
> > is fine.
> > 
> 
> fyi, the above discussion transitions akpm into the "confused" state. 
> I'll keep the patch on hold until akpm transitions back out of that
> state.
> 
Traffic control...

[What "noprefix" is]
 - When "noprefix" is used, the name of file under cgroup is..
  [Without noprefix]  ... (dir)/subsysname.filename
  [With noprefix]     ... (dir)/filename

Then, cpuset's files will be
  [Without noprefix]  ... (dir)/cpuset.xxx
  [With noprefix]     ... (dir)/xxx

This is for _backward compatibility_.

[Problem]
 cpustat subsys has "stat" file.
 memory  subsys has "stat" file.

 So, these cannot be mounted at the same mount point with "noprefix".

Considering arbitrary subsys can be mounted at the same point, allowing "noprefix"
other than cpuset just makes user-land complex, and "noprefix" itself is
troublesome, it breaks naming rule for cgroup files.

Then, I vote for that this patch should go.

Thanks,
-Kame


 













> 

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Li Zefan]

Dhaval Giani wrote:
> On Tue, Jun 02, 2009 at 09:08:04AM -0700, Paul Menage wrote:
>> On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
>>> I am not sure if this is a good idea. For libcgroup, we would then be
>>> adding a special case for just cpuset. I would rather that we allow it
>>> either for all the subsystems or none of them.
>>>
>> libcgroup shouldn't be using the noprefix option. Its only intentded
>> use is to allow the legacy "cpuset" filesystem type to be mounted and
>> to see the same fileset as it had before the cgroups transition.
>>
> 
> It does not. But if some user is using that option, we need to be in a
> position to handle it.
> 
> I am quite happy not supporting the noprefix option in the library if it
> is fine.
> 

I don't think we need to support noprefix in libcgroup.

Even if we decide to support noprefix in the lib, it shouldn't be harder
to handle noprefix+cpuset only than handle noprefix+any combination of
subsystems.

Re: + cgroups-forbid-noprefix-if-mounting-more-than-just-cpuset-subsystem. patch added to -mm tree

[Dhaval Giani]

On Tue, Jun 02, 2009 at 01:35:34PM -0700, Andrew Morton wrote:
> On Tue, 2 Jun 2009 23:10:42 +0530
> Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Jun 02, 2009 at 09:08:04AM -0700, Paul Menage wrote:
> > > On Tue, Jun 2, 2009 at 2:09 AM, Dhaval Giani <dhaval@linux.vnet.ibm.com> wrote:
> > > >
> > > > I am not sure if this is a good idea. For libcgroup, we would then be
> > > > adding a special case for just cpuset. I would rather that we allow it
> > > > either for all the subsystems or none of them.
> > > >
> > > 
> > > libcgroup shouldn't be using the noprefix option. Its only intentded
> > > use is to allow the legacy "cpuset" filesystem type to be mounted and
> > > to see the same fileset as it had before the cgroups transition.
> > > 
> > 
> > It does not. But if some user is using that option, we need to be in a
> > position to handle it.
> > 
> > I am quite happy not supporting the noprefix option in the library if it
> > is fine.
> > 
> 
> fyi, the above discussion transitions akpm into the "confused" state. 
> I'll keep the patch on hold until akpm transitions back out of that
> state.

I can help with that :).

We will not support noprefix in libcgroup and this patch should go in
because it fixes an obvious bug.

Thanks,
-- 
regards,
Dhaval