Re: [PATCH] cgroup: deprecate remount option changes and "name=" mount option

Re: [PATCH] cgroup: deprecate remount option changes and "name=" mount option

[Tejun Heo]

Hello,

On Thu, Mar 15, 2012 at 12:29:25AM +0100, Lennart Poettering wrote:
> Hmm, this is not clear to me. Are you intending to remove the name=
> mount option entirely, or just intend to remove that you can change it
> in remounts?
> 
> systemd uses name=systemd to get its own private cgroup tree, that has
> no controller bound, hence we kinda need this option. Or are you
> suggesting there was an other way to get a private cgroup tree that
> systemd can use for grouping and labelling things?

Eh, you're right.  I want to remove support for duplicate mounting by
name or subsys as it just isn't necessary, so what I want is,

* "mount -t cgroupfs -o SUBSYS" will fail if SUBSYS is already mounted
  somewhere else.

* "mount -t cgroupfs -o none" will always create a new cgroup
  hierarchy w/o any controller attached.

I didn't realize that the mount code didn't allow none w/o name and
now I don't see a non-intrusive way to remove the name option. :( Eh
well, I'll drop that part.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: deprecate remount option changes and "name=" mount option

[Lennart Poettering]

On Thu, 15.03.12 09:54, Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:

> 
> Hello,
> 
> On Thu, Mar 15, 2012 at 12:29:25AM +0100, Lennart Poettering wrote:
> > Hmm, this is not clear to me. Are you intending to remove the name=
> > mount option entirely, or just intend to remove that you can change it
> > in remounts?
> > 
> > systemd uses name=systemd to get its own private cgroup tree, that has
> > no controller bound, hence we kinda need this option. Or are you
> > suggesting there was an other way to get a private cgroup tree that
> > systemd can use for grouping and labelling things?
> 
> Eh, you're right.  I want to remove support for duplicate mounting by
> name or subsys as it just isn't necessary, so what I want is,
> 
> * "mount -t cgroupfs -o SUBSYS" will fail if SUBSYS is already mounted
>   somewhere else.
> 
> * "mount -t cgroupfs -o none" will always create a new cgroup
>   hierarchy w/o any controller attached.
> 
> I didn't realize that the mount code didn't allow none w/o name and
> now I don't see a non-intrusive way to remove the name option. :( Eh
> well, I'll drop that part.

Insisting on a name or controller (i.e. not allowing completely
anonymous cgroupfs mounts) is probably a good idea since otherwise the
data in /proc/$PID/cgroup might be a bit useless since you couldn't
sensible match up the data in there with your mount point.

Multiple mounts of the same tree can go, as long as bind mounting is
still supported so that we can make the cgroup tree available in limited
form to containers and such.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH] cgroup: deprecate remount option changes and "name=" mount option

[Lennart Poettering]

On Wed, 14.03.12 14:52, Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:

Heya,

> * "name=" mount option: This has been broken for years before Li fixed
>   it recently.  It has very marginal usefulness (plain bind mount can
>   mostly replace it) and is quite unusual.  Take the chance and mark
>   for deprecation.

Hmm, this is not clear to me. Are you intending to remove the name=
mount option entirely, or just intend to remove that you can change it
in remounts?

systemd uses name=systemd to get its own private cgroup tree, that has
no controller bound, hence we kinda need this option. Or are you
suggesting there was an other way to get a private cgroup tree that
systemd can use for grouping and labelling things?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH] cgroup: deprecate remount option changes and "name=" mount option

[Serge Hallyn]

On 03/14/2012 04:52 PM, Tejun Heo wrote:
> This patch removes the following features.
>
> * Rebinding subsys by remount: Never reached useful state - only works
>    on empty hierarchies.
>
> * release_agent update by remount: release_agent itself will be
>    replaced with conventional fsnotify notification.

meaning any applications currently relying on release agent will need to 
add a daemon using fsnotify?  Or will it be done in the kernel?

thanks,
-serge

[PATCH] cgroup: deprecate remount option changes mount option

[Tejun Heo]

This patch marks the following features for deprecation.

* Rebinding subsys by remount: Never reached useful state - only works
  on empty hierarchies.

* release_agent update by remount: release_agent itself will be
  replaced with conventional fsnotify notification.

v2: Lennart pointed out that "name=" is necessary for mounts w/o any
    controller attached.  Drop "name=" deprecation.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Lennart Poettering <mzxreary-uLTowLwuiw4b1SvskN2V4Q@public.gmane.org>
---
 Documentation/feature-removal-schedule.txt |   10 ++++++++++
 kernel/cgroup.c                            |    5 +++++
 2 files changed, 15 insertions(+)

Index: work/Documentation/feature-removal-schedule.txt
===================================================================
--- work.orig/Documentation/feature-removal-schedule.txt
+++ work/Documentation/feature-removal-schedule.txt
@@ -510,3 +510,13 @@ Why:	The pci_scan_bus_parented() interfa
 	convert to using pci_scan_root_bus() so they can supply a list of
 	bus resources when the bus is created.
 Who:	Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
+
+----------------------------
+
+What:	cgroup option updates via remount
+When:	March 2013
+Why:	Remount currently allows changing bound subsystems and
+	release_agent.  Rebinding is hardly useful as it only works
+	when the hierarchy is empty and release_agent itself should be
+	replaced with conventional fsnotify.
+Who:	Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Index: work/kernel/cgroup.c
===================================================================
--- work.orig/kernel/cgroup.c
+++ work/kernel/cgroup.c
@@ -1294,6 +1294,11 @@ static int cgroup_remount(struct super_b
 	if (ret)
 		goto out_unlock;
 
+	/* See feature-removal-schedule.txt */
+	if (opts.subsys_bits != root->actual_subsys_bits || opts.release_agent)
+		pr_warning("cgroup: option changes via remount are deprecated (pid=%d comm=%s)\n",
+			   task_tgid_nr(current), current->comm);
+
 	/* Don't allow flags or name to change at remount */
 	if (opts.flags != root->flags ||
 	    (opts.name && strcmp(opts.name, root->name))) {

Re: [PATCH] cgroup: deprecate remount option changes mount option

[Lennart Poettering]

On Thu, 15.03.12 09:56, Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) wrote:

> This patch marks the following features for deprecation.
> 
> * Rebinding subsys by remount: Never reached useful state - only works
>   on empty hierarchies.
> 
> * release_agent update by remount: release_agent itself will be
>   replaced with conventional fsnotify notification.

Just wondering, what kind of event do you plan to emit if a cgroup runs
empty? To userspace, with fanotify, will this be FAN_MODIFY? I'd very
much prefer if we only get events really when a group runs empty, and
not each time a PID is added/removed from a cgroup, which FAN_NOTIFY
might suggest?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.

Re: [PATCH] cgroup: deprecate remount option changes mount option

[Tejun Heo]

Hello,

On Thu, Mar 15, 2012 at 08:36:35PM +0100, Lennart Poettering wrote:
> > * release_agent update by remount: release_agent itself will be
> >   replaced with conventional fsnotify notification.
> 
> Just wondering, what kind of event do you plan to emit if a cgroup runs
> empty? To userspace, with fanotify, will this be FAN_MODIFY? I'd very
> much prefer if we only get events really when a group runs empty, and
> not each time a PID is added/removed from a cgroup, which FAN_NOTIFY
> might suggest?

MODIFY probably is the only thing which makes sense and I don't think
we would be emitting events for every task addition/removals.  That
better fits MODIFY events on the tasks file anyway.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: deprecate remount option changes mount option

[Tejun Heo]

Li, re-sending to your new address just in case you missed this one.

On Thu, Mar 15, 2012 at 09:56:55AM -0700, Tejun Heo wrote:
> This patch marks the following features for deprecation.
> 
> * Rebinding subsys by remount: Never reached useful state - only works
>   on empty hierarchies.
> 
> * release_agent update by remount: release_agent itself will be
>   replaced with conventional fsnotify notification.
> 
> v2: Lennart pointed out that "name=" is necessary for mounts w/o any
>     controller attached.  Drop "name=" deprecation.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: Lennart Poettering <mzxreary-uLTowLwuiw4b1SvskN2V4Q@public.gmane.org>
> ---
>  Documentation/feature-removal-schedule.txt |   10 ++++++++++
>  kernel/cgroup.c                            |    5 +++++
>  2 files changed, 15 insertions(+)
> 
> Index: work/Documentation/feature-removal-schedule.txt
> ===================================================================
> --- work.orig/Documentation/feature-removal-schedule.txt
> +++ work/Documentation/feature-removal-schedule.txt
> @@ -510,3 +510,13 @@ Why:	The pci_scan_bus_parented() interfa
>  	convert to using pci_scan_root_bus() so they can supply a list of
>  	bus resources when the bus is created.
>  Who:	Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> +
> +----------------------------
> +
> +What:	cgroup option updates via remount
> +When:	March 2013
> +Why:	Remount currently allows changing bound subsystems and
> +	release_agent.  Rebinding is hardly useful as it only works
> +	when the hierarchy is empty and release_agent itself should be
> +	replaced with conventional fsnotify.
> +Who:	Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Index: work/kernel/cgroup.c
> ===================================================================
> --- work.orig/kernel/cgroup.c
> +++ work/kernel/cgroup.c
> @@ -1294,6 +1294,11 @@ static int cgroup_remount(struct super_b
>  	if (ret)
>  		goto out_unlock;
>  
> +	/* See feature-removal-schedule.txt */
> +	if (opts.subsys_bits != root->actual_subsys_bits || opts.release_agent)
> +		pr_warning("cgroup: option changes via remount are deprecated (pid=%d comm=%s)\n",
> +			   task_tgid_nr(current), current->comm);
> +
>  	/* Don't allow flags or name to change at remount */
>  	if (opts.flags != root->flags ||
>  	    (opts.name && strcmp(opts.name, root->name))) {

-- 
tejun