[RFD/RFC v2] event about group change

[RFD/RFC v2] event about group change

[Alexander Nikiforov]

Hi,

Main idea of this patch described in
http://comments.gmane.org/gmane.linux.kernel.cgroups/1448

and according to Tejun advice
http://permalink.gmane.org/gmane.linux.kernel.cgroups/1753

need your opinion. I think it'll very useful functionality in every user 
space tool which works with cgroup.

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [RFD/RFC v2] event about group change

[Alexander Nikiforov]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: cgroup_fsnotify.patch --]
[-- Type: text/x-patch, Size: 4355 bytes --]

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 5a85b34..3594c40 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -179,6 +179,8 @@ struct cgroup {
 	struct cgroup *parent;		/* my parent */
 	struct dentry __rcu *dentry;	/* cgroup fs entry, RCU protected */
 
+	struct dentry *tasks_dentry;	/* "tasks" dentry */
+
 	/* Private pointers for each registered subsystem */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
 
@@ -387,7 +389,8 @@ struct cgroup_scanner {
  * called by subsystems from within a populate() method
  */
 int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
-		       const struct cftype *cft);
+		       const struct cftype *cft,
+		       int tasks);
 
 /*
  * Add a set of new files to the given cgroup directory. Should
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ed64cca..9ba3c02 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/fsnotify.h>
 
 #include <linux/atomic.h>
 
@@ -699,6 +700,21 @@ static struct cgroup *task_cgroup_from_root(struct task_struct *task,
 	return res;
 }
 
+static inline void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
+{
+	struct cgroupfs_root *root;
+	struct inode	*d_inode;
+	struct cgroup	*cgrp;
+
+	for_each_active_root(root) {
+		cgrp = task_cgroup_from_root(tsk, root);
+		d_inode = cgrp->tasks_dentry->d_inode;
+
+		fsnotify_parent(NULL, cgrp->tasks_dentry, mask);
+		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	}
+}
+
 /*
  * There is one global cgroup mutex. We also require taking
  * task_lock() when dereferencing a task's cgroup subsys pointers.
@@ -1924,6 +1940,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		goto out;
 	}
 
+	/* send event to the userspace */
+	fsnotify_cgroup(tsk, FS_MODIFY);
+
 	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
 
 	for_each_subsys(root, ss) {
@@ -2605,7 +2624,8 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
 
 int cgroup_add_file(struct cgroup *cgrp,
 		       struct cgroup_subsys *subsys,
-		       const struct cftype *cft)
+		       const struct cftype *cft,
+		       int tasks)
 {
 	struct dentry *dir = cgrp->dentry;
 	struct dentry *dentry;
@@ -2629,6 +2649,12 @@ int cgroup_add_file(struct cgroup *cgrp,
 		dput(dentry);
 	} else
 		error = PTR_ERR(dentry);
+
+	if(tasks) {
+		pr_warn("%s(): cft name: %s\n", __func__, name);
+		cgrp->tasks_dentry = dentry;
+	}
+
 	return error;
 }
 EXPORT_SYMBOL_GPL(cgroup_add_file);
@@ -2640,7 +2666,7 @@ int cgroup_add_files(struct cgroup *cgrp,
 {
 	int i, err;
 	for (i = 0; i < count; i++) {
-		err = cgroup_add_file(cgrp, subsys, &cft[i]);
+		err = cgroup_add_file(cgrp, subsys, &cft[i], 0);
 		if (err)
 			return err;
 	}
@@ -3642,12 +3668,16 @@ static int cgroup_populate_dir(struct cgroup *cgrp)
 	/* First clear out any existing files */
 	cgroup_clear_directory(cgrp->dentry);
 
-	err = cgroup_add_files(cgrp, NULL, files, ARRAY_SIZE(files));
+	err = cgroup_add_file(cgrp, NULL, files, 1);
+	if (err)
+		return err;
+
+	err = cgroup_add_files(cgrp, NULL, files + 1, ARRAY_SIZE(files) - 1);
 	if (err < 0)
 		return err;
 
 	if (cgrp == cgrp->top_cgroup) {
-		if ((err = cgroup_add_file(cgrp, NULL, &cft_release_agent)) < 0)
+		if ((err = cgroup_add_file(cgrp, NULL, &cft_release_agent, 0)) < 0)
 			return err;
 	}
 
@@ -4480,6 +4510,7 @@ static const struct file_operations proc_cgroupstats_operations = {
  */
 void cgroup_fork(struct task_struct *child)
 {
+
 	/*
 	 * We don't need to task_lock() current because current->cgroups
 	 * can't be changed concurrently here. The parent obviously hasn't
@@ -4489,6 +4520,10 @@ void cgroup_fork(struct task_struct *child)
 	child->cgroups = current->cgroups;
 	get_css_set(child->cgroups);
 	INIT_LIST_HEAD(&child->cg_list);
+
+	cgroup_lock();
+	fsnotify_cgroup(child, FS_MODIFY);
+	cgroup_unlock();
 }
 
 /**
@@ -4611,6 +4646,11 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 	/* Reassign the task to the init_css_set. */
 	task_lock(tsk);
 	cg = tsk->cgroups;
+
+	cgroup_lock();
+	fsnotify_cgroup(tsk, FS_MODIFY);
+	cgroup_unlock();
+
 	tsk->cgroups = &init_css_set;
 
 	if (run_callbacks && need_forkexit_callback) {

Re: [RFD/RFC v2] event about group change

[Alexander Nikiforov]

Fix in cgroup_add_file() with tasks flag a little bit ugly, but 
cgroup_exit() with run_callbacks flag works on this way. Don't know now 
how to do it better, split funtction to register "tasks" and other files 
separately looks ugly too.

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [RFD/RFC v2] event about group change

[Tejun Heo]

Hello,

I like it generally (well I suggested it so...) but can you please
post a proper patch with SOB against cgroup/for-3.5 branch?

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.5

Also, can you please cc fsnotify people so that they can go over the
new usage?

> @@ -179,6 +179,8 @@ struct cgroup {
>  	struct cgroup *parent;		/* my parent */
>  	struct dentry __rcu *dentry;	/* cgroup fs entry, RCU protected */
>  
> +	struct dentry *tasks_dentry;	/* "tasks" dentry */

Urgh... not the prettiest but I suppose it's necessary.  It will
probably be better to point to cfent instead.

> +static inline void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)

No need to make it inline.

> +{
> +	struct cgroupfs_root *root;
> +	struct inode	*d_inode;
> +	struct cgroup	*cgrp;

What are the locking rules?

> +	for_each_active_root(root) {
> +		cgrp = task_cgroup_from_root(tsk, root);
> +		d_inode = cgrp->tasks_dentry->d_inode;
> +
> +		fsnotify_parent(NULL, cgrp->tasks_dentry, mask);
> +		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);

The interface is rather weird.  It's called fsnotify_cgroup() and it
always generates the requested event on its tasks file?

>  int cgroup_add_file(struct cgroup *cgrp,
>  		       struct cgroup_subsys *subsys,
> -		       const struct cftype *cft)
> +		       const struct cftype *cft,
> +		       int tasks)

Ugh... this is ugly.

>  {
>  	struct dentry *dir = cgrp->dentry;
>  	struct dentry *dentry;
> @@ -2629,6 +2649,12 @@ int cgroup_add_file(struct cgroup *cgrp,
>  		dput(dentry);
>  	} else
>  		error = PTR_ERR(dentry);
> +
> +	if(tasks) {

          ^ missing space

> +		pr_warn("%s(): cft name: %s\n", __func__, name);

Why pr_warn?

> +		cgrp->tasks_dentry = dentry;
> +	}
> +
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(cgroup_add_file);
> @@ -2640,7 +2666,7 @@ int cgroup_add_files(struct cgroup *cgrp,
>  {
>  	int i, err;
>  	for (i = 0; i < count; i++) {
> -		err = cgroup_add_file(cgrp, subsys, &cft[i]);
> +		err = cgroup_add_file(cgrp, subsys, &cft[i], 0);
>  		if (err)
>  			return err;
>  	}
> @@ -3642,12 +3668,16 @@ static int cgroup_populate_dir(struct cgroup *cgrp)
>  	/* First clear out any existing files */
>  	cgroup_clear_directory(cgrp->dentry);
>  
> -	err = cgroup_add_files(cgrp, NULL, files, ARRAY_SIZE(files));
> +	err = cgroup_add_file(cgrp, NULL, files, 1);
> +	if (err)
> +		return err;
> +
> +	err = cgroup_add_files(cgrp, NULL, files + 1, ARRAY_SIZE(files) - 1);
>  	if (err < 0)
>  		return err;
>  
>  	if (cgrp == cgrp->top_cgroup) {
> -		if ((err = cgroup_add_file(cgrp, NULL, &cft_release_agent)) < 0)
> +		if ((err = cgroup_add_file(cgrp, NULL, &cft_release_agent, 0)) < 0)
>  			return err;
>  	}

Wouldn't it be better to make cgroup_add_file() return the created cft
and let the caller handle the tasks special case?  Also, why use 1/0
for boolean values instead of true/false?

> @@ -4480,6 +4510,7 @@ static const struct file_operations proc_cgroupstats_operations = {
>   */
>  void cgroup_fork(struct task_struct *child)
>  {
> +

Why the new line?

Thanks.

-- 
tejun

Re: [RFD/RFC v2] event about group change

[Alexander Nikiforov]

Hi,

Tejun thx for your reply. I need to have agreement with 
cgroup_add_file() and I'll post v3. Comments on your comments below

On 04/28/2012 02:34 AM, Tejun Heo wrote:
> Hello,
>
> I like it generally (well I suggested it so...) but can you please
> post a proper patch with SOB against cgroup/for-3.5 branch?
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.5
About for-3.5 - sure. I'll do it. Sorry, but what is SOB? :)
> Also, can you please cc fsnotify people so that they can go over the
> new usage?
Done. But unfortunately mail from my box not reach Li Zefan. Routing loop.

>> @@ -179,6 +179,8 @@ struct cgroup {
>>   	struct cgroup *parent;		/* my parent */
>>   	struct dentry __rcu *dentry;	/* cgroup fs entry, RCU protected */
>>
>> +	struct dentry *tasks_dentry;	/* "tasks" dentry */
> Urgh... not the prettiest but I suppose it's necessary.  It will
> probably be better to point to cfent instead.
Are you talking about struct cftype. If yes, I think for now put 
tasks_dentry into cgroup better. But if we can take dentry directly from 
cftype (look on this, for now I have no idea how can I do it)
it will be of course better. If we can't take, we will have pointer to 
every file inside cgroup. Since for memcg we have different event 
approach, I don't think this proper way.
>> +static inline void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
> No need to make it inline.
Ok
>> +{
>> +	struct cgroupfs_root *root;
>> +	struct inode	*d_inode;
>> +	struct cgroup	*cgrp;
> What are the locking rules?
fsnotify_cgroup() called inside cgroup_lock(), is it not sufficient??
>> +	for_each_active_root(root) {
>> +		cgrp = task_cgroup_from_root(tsk, root);
>> +		d_inode = cgrp->tasks_dentry->d_inode;
>> +
>> +		fsnotify_parent(NULL, cgrp->tasks_dentry, mask);
>> +		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> The interface is rather weird.  It's called fsnotify_cgroup() and it
> always generates the requested event on its tasks file?
>
yes
>>   int cgroup_add_file(struct cgroup *cgrp,
>>   		       struct cgroup_subsys *subsys,
>> -		       const struct cftype *cft)
>> +		       const struct cftype *cft,
>> +		       int tasks)
> Ugh... this is ugly.
yes. I wrote about this. But run_callback in do_exit()->cgroup_exit() 
works on this way.
>>   {
>>   	struct dentry *dir = cgrp->dentry;
>>   	struct dentry *dentry;
>> @@ -2629,6 +2649,12 @@ int cgroup_add_file(struct cgroup *cgrp,
>>   		dput(dentry);
>>   	} else
>>   		error = PTR_ERR(dentry);
>> +
>> +	if(tasks) {
>            ^ missing space
>
>> +		pr_warn("%s(): cft name: %s\n", __func__, name);
> Why pr_warn?
sorry, I sent this patch RFC, forgot to clean up.
>
>> +		cgrp->tasks_dentry = dentry;
>> +	}
>> +
>>   	return error;
>>   }
>>   EXPORT_SYMBOL_GPL(cgroup_add_file);
>> @@ -2640,7 +2666,7 @@ int cgroup_add_files(struct cgroup *cgrp,
>>   {
>>   	int i, err;
>>   	for (i = 0; i<  count; i++) {
>> -		err = cgroup_add_file(cgrp, subsys,&cft[i]);
>> +		err = cgroup_add_file(cgrp, subsys,&cft[i], 0);
>>   		if (err)
>>   			return err;
>>   	}
>> @@ -3642,12 +3668,16 @@ static int cgroup_populate_dir(struct cgroup *cgrp)
>>   	/* First clear out any existing files */
>>   	cgroup_clear_directory(cgrp->dentry);
>>
>> -	err = cgroup_add_files(cgrp, NULL, files, ARRAY_SIZE(files));
>> +	err = cgroup_add_file(cgrp, NULL, files, 1);
>> +	if (err)
>> +		return err;
>> +
>> +	err = cgroup_add_files(cgrp, NULL, files + 1, ARRAY_SIZE(files) - 1);
>>   	if (err<  0)
>>   		return err;
>>
>>   	if (cgrp == cgrp->top_cgroup) {
>> -		if ((err = cgroup_add_file(cgrp, NULL,&cft_release_agent))<  0)
>> +		if ((err = cgroup_add_file(cgrp, NULL,&cft_release_agent, 0))<  0)
>>   			return err;
>>   	}
> Wouldn't it be better to make cgroup_add_file() return the created cft
> and let the caller handle the tasks special case?  Also, why use 1/0
> for boolean values instead of true/false?
How can I understand that this cft from tasks, only with strcmp with 
name. Don't think this is the best way, but my way ugly too.
About 1/0 try to write on current way, just made it like run_callbacks
>> @@ -4480,6 +4510,7 @@ static const struct file_operations proc_cgroupstats_operations = {
>>    */
>>   void cgroup_fork(struct task_struct *child)
>>   {
>> +
> Why the new line?
Forgot to cleanup
>
> Thanks.
>

Thanks.

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [RFD/RFC v2] event about group change

[Tejun Heo]

Hello,

On Sat, Apr 28, 2012 at 09:40:49AM +0400, Alexander Nikiforov wrote:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.5
>
> About for-3.5 - sure. I'll do it. Sorry, but what is SOB? :)

Signed-off-by tag.  Sorry about the abbreviation.

> >Also, can you please cc fsnotify people so that they can go over the
> >new usage?
>
> Done. But unfortunately mail from my box not reach Li Zefan. Routing loop.

His new mail address is lizefan-hv44wF8Li92Xj1p+fO2waQ@public.gmane.org

> >>@@ -179,6 +179,8 @@ struct cgroup {
> >>  	struct cgroup *parent;		/* my parent */
> >>  	struct dentry __rcu *dentry;	/* cgroup fs entry, RCU protected */
> >>
> >>+	struct dentry *tasks_dentry;	/* "tasks" dentry */
> >Urgh... not the prettiest but I suppose it's necessary.  It will
> >probably be better to point to cfent instead.
>
> Are you talking about struct cftype. If yes, I think for now put
> tasks_dentry into cgroup better. But if we can take dentry directly
> from cftype (look on this, for now I have no idea how can I do it)
> it will be of course better. If we can't take, we will have pointer
> to every file inside cgroup. Since for memcg we have different event
> approach, I don't think this proper way.

If you look at cgroup/for-3.5 branch, there's new entry named struct
cfent representing each instance of the file which has a pointer to
the matching dentry.

> >>+{
> >>+	struct cgroupfs_root *root;
> >>+	struct inode	*d_inode;
> >>+	struct cgroup	*cgrp;
> >What are the locking rules?
>
> fsnotify_cgroup() called inside cgroup_lock(), is it not sufficient??

I was hoping for lockdep_assert_held() annotation. :)

> >Wouldn't it be better to make cgroup_add_file() return the created cft
> >and let the caller handle the tasks special case?  Also, why use 1/0
> >for boolean values instead of true/false?
>
> How can I understand that this cft from tasks, only with strcmp with
> name. Don't think this is the best way, but my way ugly too.
> About 1/0 try to write on current way, just made it like run_callbacks

I meant separating out "task" cftype entry and creating it separately
using a function returning the created cfent and storing it.  Instead
of putting the special logic inside file creation function.

Thanks.

-- 
tejun

Re: [RFD/RFC v2] event about group change

[Alexander Nikiforov]

On 04/29/2012 01:41 AM, Tejun Heo wrote:
> Hello,
>
> On Sat, Apr 28, 2012 at 09:40:49AM +0400, Alexander Nikiforov wrote:
>>>    git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.5
>> About for-3.5 - sure. I'll do it. Sorry, but what is SOB? :)
> Signed-off-by tag.  Sorry about the abbreviation.

done

>
>>> Also, can you please cc fsnotify people so that they can go over the
>>> new usage?
>> Done. But unfortunately mail from my box not reach Li Zefan. Routing loop.
> His new mail address is lizefan-hv44wF8Li92Xj1p+fO2waQ@public.gmane.org

done

>
>>>> @@ -179,6 +179,8 @@ struct cgroup {
>>>>   	struct cgroup *parent;		/* my parent */
>>>>   	struct dentry __rcu *dentry;	/* cgroup fs entry, RCU protected */
>>>>
>>>> +	struct dentry *tasks_dentry;	/* "tasks" dentry */
>>> Urgh... not the prettiest but I suppose it's necessary.  It will
>>> probably be better to point to cfent instead.
>> Are you talking about struct cftype. If yes, I think for now put
>> tasks_dentry into cgroup better. But if we can take dentry directly
>> from cftype (look on this, for now I have no idea how can I do it)
>> it will be of course better. If we can't take, we will have pointer
>> to every file inside cgroup. Since for memcg we have different event
>> approach, I don't think this proper way.
> If you look at cgroup/for-3.5 branch, there's new entry named struct
> cfent representing each instance of the file which has a pointer to
> the matching dentry.

done

>
>>>> +{
>>>> +	struct cgroupfs_root *root;
>>>> +	struct inode	*d_inode;
>>>> +	struct cgroup	*cgrp;
>>> What are the locking rules?
>> fsnotify_cgroup() called inside cgroup_lock(), is it not sufficient??
> I was hoping for lockdep_assert_held() annotation. :)

done

>
>>> Wouldn't it be better to make cgroup_add_file() return the created cft
>>> and let the caller handle the tasks special case?  Also, why use 1/0
>>> for boolean values instead of true/false?
>> How can I understand that this cft from tasks, only with strcmp with
>> name. Don't think this is the best way, but my way ugly too.
>> About 1/0 try to write on current way, just made it like run_callbacks
> I meant separating out "task" cftype entry and creating it separately
> using a function returning the created cfent and storing it.  Instead
> of putting the special logic inside file creation function.

done

>
> Thanks.
>

Please review new version

Thanks

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

[PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: cgroup_fsnotify.patch --]
[-- Type: text/x-patch, Size: 4441 bytes --]

Signed-off-by: Alex Nikiforov <a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index d3f5fba..9d0d622 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -181,6 +181,8 @@ struct cgroup {
 	struct cgroup *parent;		/* my parent */
 	struct dentry __rcu *dentry;	/* cgroup fs entry, RCU protected */
 
+	struct cfent *tasks_cfe;	/* "tasks" cfent */
+
 	/* Private pointers for each registered subsystem */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ad8eae5..8fabfd4 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/fsnotify.h> /* used in fsnotify_cgroup() */
 #include <linux/kthread.h>
 
 #include <linux/atomic.h>
@@ -738,6 +739,23 @@ static struct cgroup *task_cgroup_from_root(struct task_struct *task,
 	return res;
 }
 
+static void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
+{
+	struct cgroupfs_root *root;
+	struct inode	*d_inode;
+	struct cgroup	*cgrp;
+
+	lockdep_assert_held(&cgroup_mutex);
+
+	for_each_active_root(root) {
+		cgrp = task_cgroup_from_root(tsk, root);
+		d_inode = cgrp->tasks_cfe->dentry->d_inode;
+
+		fsnotify_parent(NULL, cgrp->tasks_cfe->dentry, mask);
+		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	}
+}
+
 /*
  * There is one global cgroup mutex. We also require taking
  * task_lock() when dereferencing a task's cgroup subsys pointers.
@@ -1978,6 +1996,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		goto out;
 	}
 
+	/* cgroup_attach_task() already guarded by cgroup_lock by caller */
+	fsnotify_cgroup(tsk, FS_MODIFY);
+
 	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
 
 	for_each_subsys(root, ss) {
@@ -2669,7 +2690,7 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
 	return mode;
 }
 
-static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
+static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 			   const struct cftype *cft)
 {
 	struct dentry *dir = cgrp->dentry;
@@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 
 	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
 	if (!cfe)
-		return -ENOMEM;
+		return NULL;
 
 	dentry = lookup_one_len(name, dir, strlen(name));
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
-		goto out;
+		kfree(cfe);
+		return NULL;
 	}
 
 	mode = cgroup_file_mode(cft);
@@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 		cfe->dentry = dentry;
 		dentry->d_fsdata = cfe;
 		list_add_tail(&cfe->node, &parent->files);
-		cfe = NULL;
 	}
 	dput(dentry);
-out:
-	kfree(cfe);
-	return error;
+
+	return cfe;
 }
 
 static int cgroup_addrm_files(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 			      const struct cftype cfts[], bool is_add)
 {
 	const struct cftype *cft;
-	int err, ret = 0;
+	struct cfent *cfe;
+	int ret = 0;
 
 	for (cft = cfts; cft->name[0] != '\0'; cft++) {
-		if (is_add)
-			err = cgroup_add_file(cgrp, subsys, cft);
-		else
-			err = cgroup_rm_file(cgrp, cft);
-		if (err) {
-			pr_warning("cgroup_addrm_files: failed to %s %s, err=%d\n",
-				   is_add ? "add" : "remove", cft->name, err);
-			ret = err;
+		if (is_add) {
+			cfe = cgroup_add_file(cgrp, subsys, cft);
+			if(!cfe) {
+				pr_warning("%s: failed to add %s\n",
+					   __func__, cft->name);
+				ret = -1;
+			} else if(strcmp(cft->name, "tasks") == 0)
+				cgrp->tasks_cfe = cfe;
+		} else {
+			if (cgroup_rm_file(cgrp, cft)) {
+				pr_warning("%s: failed to remove %s\n",
+					   __func__, cft->name);
+				ret = -1;
+			}
 		}
 	}
 	return ret;
@@ -4757,6 +4784,10 @@ void cgroup_fork(struct task_struct *child)
 	child->cgroups = current->cgroups;
 	get_css_set(child->cgroups);
 	INIT_LIST_HEAD(&child->cg_list);
+
+	cgroup_lock();
+	fsnotify_cgroup(child, FS_MODIFY);
+	cgroup_unlock();
 }
 
 /**
@@ -4879,6 +4910,11 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 	/* Reassign the task to the init_css_set. */
 	task_lock(tsk);
 	cg = tsk->cgroups;
+
+	cgroup_lock();
+	fsnotify_cgroup(tsk, FS_MODIFY);
+	cgroup_unlock();
+
 	tsk->cgroups = &init_css_set;
 
 	if (run_callbacks && need_forkexit_callback) {

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo]

> Signed-off-by: Alex Nikiforov <a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Please read how other patches are being posted.  You need to explain
what the patch does and why.

> +static void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
> +{
> +	struct cgroupfs_root *root;
> +	struct inode	*d_inode;
> +	struct cgroup	*cgrp;

Please read how other local variable definitions are indented.  Read
the coding style and follow your surroundings.

> +	lockdep_assert_held(&cgroup_mutex);
> +
> +	for_each_active_root(root) {
> +		cgrp = task_cgroup_from_root(tsk, root);
> +		d_inode = cgrp->tasks_cfe->dentry->d_inode;

Why not define these variables inside loop?

> +		fsnotify_parent(NULL, cgrp->tasks_cfe->dentry, mask);
> +		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	}
> +}
> +
>  /*
>   * There is one global cgroup mutex. We also require taking
>   * task_lock() when dereferencing a task's cgroup subsys pointers.
> @@ -1978,6 +1996,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  		goto out;
>  	}
>  
> +	/* cgroup_attach_task() already guarded by cgroup_lock by caller */

If you have lockdep annotation, I don't think this is necessary.

> +	fsnotify_cgroup(tsk, FS_MODIFY);
> +
>  	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
>  
>  	for_each_subsys(root, ss) {
> @@ -2669,7 +2690,7 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
>  	return mode;
>  }
>  
> -static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> +static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>  			   const struct cftype *cft)
>  {
>  	struct dentry *dir = cgrp->dentry;
> @@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>  
>  	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
>  	if (!cfe)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	dentry = lookup_one_len(name, dir, strlen(name));
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
> -		goto out;
> +		kfree(cfe);
> +		return NULL;
>  	}
>  
>  	mode = cgroup_file_mode(cft);
> @@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>  		cfe->dentry = dentry;
>  		dentry->d_fsdata = cfe;
>  		list_add_tail(&cfe->node, &parent->files);
> -		cfe = NULL;
>  	}
>  	dput(dentry);
> -out:
> -	kfree(cfe);
> -	return error;
> +
> +	return cfe;
>  }

Are you sure you're not creating a memory leak here?

>  static int cgroup_addrm_files(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>  			      const struct cftype cfts[], bool is_add)
>  {
>  	const struct cftype *cft;
> -	int err, ret = 0;
> +	struct cfent *cfe;
> +	int ret = 0;
>  
>  	for (cft = cfts; cft->name[0] != '\0'; cft++) {
> -		if (is_add)
> -			err = cgroup_add_file(cgrp, subsys, cft);
> -		else
> -			err = cgroup_rm_file(cgrp, cft);
> -		if (err) {
> -			pr_warning("cgroup_addrm_files: failed to %s %s, err=%d\n",
> -				   is_add ? "add" : "remove", cft->name, err);
> -			ret = err;
> +		if (is_add) {
> +			cfe = cgroup_add_file(cgrp, subsys, cft);
> +			if(!cfe) {
> +				pr_warning("%s: failed to add %s\n",
> +					   __func__, cft->name);
> +				ret = -1;
> +			} else if(strcmp(cft->name, "tasks") == 0)
                                 ^
				space here.

Please read Documents/CodingStyle.

> +				cgrp->tasks_cfe = cfe;
> +		} else {
> +			if (cgroup_rm_file(cgrp, cft)) {
> +				pr_warning("%s: failed to remove %s\n",
> +					   __func__, cft->name);
> +				ret = -1;
> +			}

Please don't bury this inside cgroup_addrm_files() with strcmp()
trying to find out again which one is the task file - in general, if
you're doing strcmp() inside kernel which isn't parsing userland
input, something is wrong.  Just separate out the cftype of task file
into a standalone entry and let cgroup_populate_dir() call
cgroup_add_file() on it and then record the returned cfe.

Thanks.

-- 
tejun

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov]

On 05/03/2012 07:50 PM, Tejun Heo wrote:
>> Signed-off-by: Alex Nikiforov<a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Please read how other patches are being posted.  You need to explain
> what the patch does and why.
Sorry, I will.
>
>> +static void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
>> +{
>> +	struct cgroupfs_root *root;
>> +	struct inode	*d_inode;
>> +	struct cgroup	*cgrp;
> Please read how other local variable definitions are indented.  Read
> the coding style and follow your surroundings.
>
>> +	lockdep_assert_held(&cgroup_mutex);
>> +
>> +	for_each_active_root(root) {
>> +		cgrp = task_cgroup_from_root(tsk, root);
>> +		d_inode = cgrp->tasks_cfe->dentry->d_inode;
> Why not define these variables inside loop?
sure
>
>> +		fsnotify_parent(NULL, cgrp->tasks_cfe->dentry, mask);
>> +		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
>> +	}
>> +}
>> +
>>   /*
>>    * There is one global cgroup mutex. We also require taking
>>    * task_lock() when dereferencing a task's cgroup subsys pointers.
>> @@ -1978,6 +1996,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>>   		goto out;
>>   	}
>>
>> +	/* cgroup_attach_task() already guarded by cgroup_lock by caller */
> If you have lockdep annotation, I don't think this is necessary.
Ok
>
>> +	fsnotify_cgroup(tsk, FS_MODIFY);
>> +
>>   	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
>>
>>   	for_each_subsys(root, ss) {
>> @@ -2669,7 +2690,7 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
>>   	return mode;
>>   }
>>
>> -static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>> +static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>   			   const struct cftype *cft)
>>   {
>>   	struct dentry *dir = cgrp->dentry;
>> @@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>
>>   	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
>>   	if (!cfe)
>> -		return -ENOMEM;
>> +		return NULL;
>>
>>   	dentry = lookup_one_len(name, dir, strlen(name));
>>   	if (IS_ERR(dentry)) {
>>   		error = PTR_ERR(dentry);
>> -		goto out;
>> +		kfree(cfe);
>> +		return NULL;
>>   	}
>>
>>   	mode = cgroup_file_mode(cft);
>> @@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>   		cfe->dentry = dentry;
>>   		dentry->d_fsdata = cfe;
>>   		list_add_tail(&cfe->node,&parent->files);
>> -		cfe = NULL;
>>   	}
>>   	dput(dentry);
>> -out:
>> -	kfree(cfe);
>> -	return error;
>> +
>> +	return cfe;
>>   }
> Are you sure you're not creating a memory leak here?

code make the same things, just without goto. move kfree inside if 
(IS_ERR(dentry), but before dput(dentry)  cfe set to NULL, so kfree(cfe) 
will do nothing. I think it's without any mem leaks.

>
>>   static int cgroup_addrm_files(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>   			      const struct cftype cfts[], bool is_add)
>>   {
>>   	const struct cftype *cft;
>> -	int err, ret = 0;
>> +	struct cfent *cfe;
>> +	int ret = 0;
>>
>>   	for (cft = cfts; cft->name[0] != '\0'; cft++) {
>> -		if (is_add)
>> -			err = cgroup_add_file(cgrp, subsys, cft);
>> -		else
>> -			err = cgroup_rm_file(cgrp, cft);
>> -		if (err) {
>> -			pr_warning("cgroup_addrm_files: failed to %s %s, err=%d\n",
>> -				   is_add ? "add" : "remove", cft->name, err);
>> -			ret = err;
>> +		if (is_add) {
>> +			cfe = cgroup_add_file(cgrp, subsys, cft);
>> +			if(!cfe) {
>> +				pr_warning("%s: failed to add %s\n",
>> +					   __func__, cft->name);
>> +				ret = -1;
>> +			} else if(strcmp(cft->name, "tasks") == 0)
>                                   ^
> 				space here.
>
> Please read Documents/CodingStyle.
Sorry, fixed.
>
>> +				cgrp->tasks_cfe = cfe;
>> +		} else {
>> +			if (cgroup_rm_file(cgrp, cft)) {
>> +				pr_warning("%s: failed to remove %s\n",
>> +					   __func__, cft->name);
>> +				ret = -1;
>> +			}
> Please don't bury this inside cgroup_addrm_files() with strcmp()
> trying to find out again which one is the task file - in general, if
> you're doing strcmp() inside kernel which isn't parsing userland
> input, something is wrong.  Just separate out the cftype of task file
> into a standalone entry and let cgroup_populate_dir() call
> cgroup_add_file() on it and then record the returned cfe.
done
>
> Thanks.
>

Thx, for your time.

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo]

Hello,

On Fri, May 04, 2012 at 04:55:44PM +0400, Alexander Nikiforov wrote:
> >>-static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>+static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>  			   const struct cftype *cft)
> >>  {
> >>  	struct dentry *dir = cgrp->dentry;
> >>@@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>
> >>  	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
> >>  	if (!cfe)
> >>-		return -ENOMEM;
> >>+		return NULL;
> >>
> >>  	dentry = lookup_one_len(name, dir, strlen(name));
> >>  	if (IS_ERR(dentry)) {
> >>  		error = PTR_ERR(dentry);
> >>-		goto out;
> >>+		kfree(cfe);
> >>+		return NULL;
> >>  	}
> >>
> >>  	mode = cgroup_file_mode(cft);
> >>@@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
> >>  		cfe->dentry = dentry;
> >>  		dentry->d_fsdata = cfe;
> >>  		list_add_tail(&cfe->node,&parent->files);
> >>-		cfe = NULL;
> >>  	}
> >>  	dput(dentry);
> >>-out:
> >>-	kfree(cfe);
> >>-	return error;
> >>+
> >>+	return cfe;
> >>  }
> >Are you sure you're not creating a memory leak here?
> 
> code make the same things, just without goto. move kfree inside if
> (IS_ERR(dentry), but before dput(dentry)  cfe set to NULL, so
> kfree(cfe) will do nothing. I think it's without any mem leaks.

I'm confused.  Who's freeing cfe after cgroup_create_file() failure?
Also, why are you changing this at all?

Thanks.

-- 
tejun

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov]

Hi, Tejun

On 05/04/2012 08:54 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, May 04, 2012 at 04:55:44PM +0400, Alexander Nikiforov wrote:
>>>> -static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>> +static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>   			   const struct cftype *cft)
>>>>   {
>>>>   	struct dentry *dir = cgrp->dentry;
>>>> @@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>
>>>>   	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
>>>>   	if (!cfe)
>>>> -		return -ENOMEM;
>>>> +		return NULL;
>>>>
>>>>   	dentry = lookup_one_len(name, dir, strlen(name));
>>>>   	if (IS_ERR(dentry)) {
>>>>   		error = PTR_ERR(dentry);
>>>> -		goto out;
>>>> +		kfree(cfe);
>>>> +		return NULL;
>>>>   	}
>>>>
>>>>   	mode = cgroup_file_mode(cft);
>>>> @@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>   		cfe->dentry = dentry;
>>>>   		dentry->d_fsdata = cfe;
>>>>   		list_add_tail(&cfe->node,&parent->files);
>>>> -		cfe = NULL;
>>>>   	}
>>>>   	dput(dentry);
>>>> -out:
>>>> -	kfree(cfe);
>>>> -	return error;
>>>> +
>>>> +	return cfe;
>>>>   }
>>> Are you sure you're not creating a memory leak here?
>> code make the same things, just without goto. move kfree inside if
>> (IS_ERR(dentry), but before dput(dentry)  cfe set to NULL, so
>> kfree(cfe) will do nothing. I think it's without any mem leaks.
> I'm confused.  Who's freeing cfe after cgroup_create_file() failure?
> Also, why are you changing this at all?
Of course you are right, miss that.
>
> Thanks.
>

What about other in v4, any suggestion?

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

[PATCH V5] event about group change

[Alex Nikiforov]

Fix memory leak, also fix missing dput if cgroup_create_file() fails.

Alex Nikiforov (1):
  Currently, user can get inotify FS_MODIFY event only if     "tasks"
    file changed from the user space side (for example     echo $$ >
    /patch/to/cgroup/tasks), but if another process forked     user
    don't get FS_MODIFY event. This patch add this feature.     With
    this user can get FS_MODIFY on do_fork()/do_exit()/move PID    
    from one group to another.

 kernel/cgroup.c |   88 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 69 insertions(+), 19 deletions(-)

-- 
1.7.5.4

[PATCH V5] Currently, user can get inotify FS_MODIFY event only if "tasks" file changed from the user space side (for example echo $$ > /patch/to/cgroup/tasks), but if another process forked user don't get FS_MODIFY event. This patch add this feature. With this user can get FS_MODIFY on do_fork()/do_exit()/move PID from one group to another.

[Alex Nikiforov]

Signed-off-by: Alex Nikiforov <a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 kernel/cgroup.c |   88 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 69 insertions(+), 19 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ad8eae5..c95c23d 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/fsnotify.h> /* used in fsnotify_cgroup() */
 #include <linux/kthread.h>
 
 #include <linux/atomic.h>
@@ -738,6 +739,24 @@ static struct cgroup *task_cgroup_from_root(struct task_struct *task,
 	return res;
 }
 
+static void fsnotify_cgroup(struct task_struct *tsk, __u32 mask)
+{
+	struct cgroupfs_root *root;
+
+	lockdep_assert_held(&cgroup_mutex);
+
+	for_each_active_root(root) {
+		struct inode *d_inode;
+		struct cgroup *cgrp;
+
+		cgrp = task_cgroup_from_root(tsk, root);
+		d_inode = cgrp->tasks_cfe->dentry->d_inode;
+
+		fsnotify_parent(NULL, cgrp->tasks_cfe->dentry, mask);
+		fsnotify(d_inode, mask, d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	}
+}
+
 /*
  * There is one global cgroup mutex. We also require taking
  * task_lock() when dereferencing a task's cgroup subsys pointers.
@@ -1978,6 +1997,8 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 		goto out;
 	}
 
+	fsnotify_cgroup(tsk, FS_MODIFY);
+
 	cgroup_task_migrate(cgrp, oldcgrp, tsk, newcg);
 
 	for_each_subsys(root, ss) {
@@ -2669,7 +2690,7 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
 	return mode;
 }
 
-static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
+static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 			   const struct cftype *cft)
 {
 	struct dentry *dir = cgrp->dentry;
@@ -2696,44 +2717,50 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 
 	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
 	if (!cfe)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dentry = lookup_one_len(name, dir, strlen(name));
 	if (IS_ERR(dentry)) {
-		error = PTR_ERR(dentry);
-		goto out;
+		kfree(cfe);
+		return (struct cfent *)dentry;
 	}
 
 	mode = cgroup_file_mode(cft);
 	error = cgroup_create_file(dentry, mode | S_IFREG, cgrp->root->sb);
-	if (!error) {
+	if (error) {
+		kfree(cfe);
+		cfe = ERR_PTR(error);
+	} else {
 		cfe->type = (void *)cft;
 		cfe->dentry = dentry;
 		dentry->d_fsdata = cfe;
 		list_add_tail(&cfe->node, &parent->files);
-		cfe = NULL;
 	}
+
 	dput(dentry);
-out:
-	kfree(cfe);
-	return error;
+
+	return cfe;
 }
 
 static int cgroup_addrm_files(struct cgroup *cgrp, struct cgroup_subsys *subsys,
 			      const struct cftype cfts[], bool is_add)
 {
 	const struct cftype *cft;
-	int err, ret = 0;
+	struct cfent *cfe;
+	int ret = 0;
 
 	for (cft = cfts; cft->name[0] != '\0'; cft++) {
-		if (is_add)
-			err = cgroup_add_file(cgrp, subsys, cft);
-		else
-			err = cgroup_rm_file(cgrp, cft);
-		if (err) {
-			pr_warning("cgroup_addrm_files: failed to %s %s, err=%d\n",
-				   is_add ? "add" : "remove", cft->name, err);
-			ret = err;
+		if (is_add) {
+			cfe = cgroup_add_file(cgrp, subsys, cft);
+			if (IS_ERR(cfe)) {
+				pr_warn("%s: failed to add %s\n", __func__, cft->name);
+				ret = -1;
+			}
+		} else {
+			if (cgroup_rm_file(cgrp, cft)) {
+				pr_warn("%s: failed to remove %s\n", __func__, cft->name);
+				ret = -1;
+			}
 		}
 	}
 	return ret;
@@ -3804,6 +3831,10 @@ static int cgroup_clone_children_write(struct cgroup *cgrp,
 #define CGROUP_FILE_GENERIC_PREFIX "cgroup."
 static struct cftype files[] = {
 	{
+		/*
+		 * don't move tasks from first place, cgroup_populate_dir()
+		 * thinks that "tasks" is first
+		 */
 		.name = "tasks",
 		.open = cgroup_tasks_open,
 		.write_u64 = cgroup_tasks_write,
@@ -3846,8 +3877,18 @@ static int cgroup_populate_dir(struct cgroup *cgrp)
 {
 	int err;
 	struct cgroup_subsys *ss;
+	struct cfent *cfe;
+
+	/* create "tasks" file, save cfent */
+	cfe = cgroup_add_file(cgrp, NULL, files);
+	if (IS_ERR(cfe)) {
+		pr_warn("%s: failed to add %s\n", __func__, files[0].name);
+		return -1;
+	}
+	cgrp->tasks_cfe = cfe;
 
-	err = cgroup_addrm_files(cgrp, NULL, files, true);
+	/* add files after "tasks" */
+	err = cgroup_addrm_files(cgrp, NULL, files + 1, true);
 	if (err < 0)
 		return err;
 
@@ -4757,6 +4798,10 @@ void cgroup_fork(struct task_struct *child)
 	child->cgroups = current->cgroups;
 	get_css_set(child->cgroups);
 	INIT_LIST_HEAD(&child->cg_list);
+
+	cgroup_lock();
+	fsnotify_cgroup(child, FS_MODIFY);
+	cgroup_unlock();
 }
 
 /**
@@ -4879,6 +4924,11 @@ void cgroup_exit(struct task_struct *tsk, int run_callbacks)
 	/* Reassign the task to the init_css_set. */
 	task_lock(tsk);
 	cg = tsk->cgroups;
+
+	cgroup_lock();
+	fsnotify_cgroup(tsk, FS_MODIFY);
+	cgroup_unlock();
+
 	tsk->cgroups = &init_css_set;
 
 	if (run_callbacks && need_forkexit_callback) {
-- 
1.7.5.4

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov]

Hi

sorry for noise, but I have a question


     mode = cgroup_file_mode(cft);
     error = cgroup_create_file(dentry, mode | S_IFREG, cgrp->root->sb);
     if (!error) {
         cfe->type = (void *)cft;
         cfe->dentry = dentry;
         dentry->d_fsdata = cfe;
         list_add_tail(&cfe->node, &parent->files);
         cfe = NULL;
     }
     dput(dentry);
out:
     kfree(cfe);
     return error;

this is from for-3.5 version. dput executes if cgroup_create_file fails 
and done correct. but in cgroup_create_file dget calls just before 
return, so if cgroup_create_file we call dput without dget. is this 
correct behavioural??


On 05/04/2012 08:54 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, May 04, 2012 at 04:55:44PM +0400, Alexander Nikiforov wrote:
>>>> -static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>> +static struct cfent* cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>   			   const struct cftype *cft)
>>>>   {
>>>>   	struct dentry *dir = cgrp->dentry;
>>>> @@ -2696,12 +2717,13 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>
>>>>   	cfe = kzalloc(sizeof(*cfe), GFP_KERNEL);
>>>>   	if (!cfe)
>>>> -		return -ENOMEM;
>>>> +		return NULL;
>>>>
>>>>   	dentry = lookup_one_len(name, dir, strlen(name));
>>>>   	if (IS_ERR(dentry)) {
>>>>   		error = PTR_ERR(dentry);
>>>> -		goto out;
>>>> +		kfree(cfe);
>>>> +		return NULL;
>>>>   	}
>>>>
>>>>   	mode = cgroup_file_mode(cft);
>>>> @@ -2711,29 +2733,34 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys,
>>>>   		cfe->dentry = dentry;
>>>>   		dentry->d_fsdata = cfe;
>>>>   		list_add_tail(&cfe->node,&parent->files);
>>>> -		cfe = NULL;
>>>>   	}
>>>>   	dput(dentry);
>>>> -out:
>>>> -	kfree(cfe);
>>>> -	return error;
>>>> +
>>>> +	return cfe;
>>>>   }
>>> Are you sure you're not creating a memory leak here?
>> code make the same things, just without goto. move kfree inside if
>> (IS_ERR(dentry), but before dput(dentry)  cfe set to NULL, so
>> kfree(cfe) will do nothing. I think it's without any mem leaks.
> I'm confused.  Who's freeing cfe after cgroup_create_file() failure?
> Also, why are you changing this at all?
>
> Thanks.
>


-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Eric Paris]

On Thu, 2012-05-03 at 13:21 +0400, Alexander Nikiforov wrote:

Can't comment inline because patch was attached rather than inline.  But
if there is ANY way I'd really like to see you using FSNOTIFY_EVENT_PATH
instead of inode.  inode works for dnotify and inotify, but not for
things watched with fanotify.

-Eric

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov]

Hi Eric,

I had try to implement with FSNOTIFY_EVENT_PATH. But it's pretty hard to 
obtain struct path without full chain of the sys_open()
or may be I don't know some simple way. I'll spend some more time on 
this, but for now I have no idea how to do it. If you have some please 
share them.


On 05/04/2012 12:05 AM, Eric Paris wrote:
> On Thu, 2012-05-03 at 13:21 +0400, Alexander Nikiforov wrote:
>
> Can't comment inline because patch was attached rather than inline.  But
> if there is ANY way I'd really like to see you using FSNOTIFY_EVENT_PATH
> instead of inode.  inode works for dnotify and inotify, but not for
> things watched with fanotify.
>
> -Eric
>
>


-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo]

On Fri, May 04, 2012 at 09:24:40AM +0400, Alexander Nikiforov wrote:
> Hi Eric,
> 
> I had try to implement with FSNOTIFY_EVENT_PATH. But it's pretty
> hard to obtain struct path without full chain of the sys_open()
> or may be I don't know some simple way. I'll spend some more time on
> this, but for now I have no idea how to do it. If you have some
> please share them.

Hmm... we have dentry from cfe->dentry and we can either use the first
vfsmount from sb->s_mounts or all of them (ie. generate event on each
vfsmount).  It's kinda silly either way tho.  Eric, it might be a good
idea to provide generic helper for virtual filesystems which want to
generate superblock-wide events?

Thanks.

-- 
tejun

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Al Viro]

On Fri, May 04, 2012 at 10:04:12AM -0700, Tejun Heo wrote:
> On Fri, May 04, 2012 at 09:24:40AM +0400, Alexander Nikiforov wrote:
> > Hi Eric,
> > 
> > I had try to implement with FSNOTIFY_EVENT_PATH. But it's pretty
> > hard to obtain struct path without full chain of the sys_open()
> > or may be I don't know some simple way. I'll spend some more time on
> > this, but for now I have no idea how to do it. If you have some
> > please share them.
> 
> Hmm... we have dentry from cfe->dentry and we can either use the first
> vfsmount from sb->s_mounts or all of them (ie. generate event on each
> vfsmount).

	No, you can not.  There are damn good reasons why that list does not
go through a field of struct vfsmount and preventing that kind of idiocy
is pretty high on the list.

	Consider that preemptively NAKed.  Eric, would you mind explaining
WTF would fsnotify *do* with vfsmount(s) here, seeing that they might very
well be not mounted in anyone's namespace?  And as for generating an event
for each, they might appear and disappear at zero notice.

	*notify: Racy By Design...

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo]

On Fri, May 04, 2012 at 06:43:30PM +0100, Al Viro wrote:
> 	Consider that preemptively NAKed.  Eric, would you mind explaining
> WTF would fsnotify *do* with vfsmount(s) here, seeing that they might very
> well be not mounted in anyone's namespace?  And as for generating an event
> for each, they might appear and disappear at zero notice.

Eric, any ideas on how to proceed?

Thanks.

-- 
tejun

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo]

On Mon, May 07, 2012 at 01:38:48PM -0700, Tejun Heo wrote:
> On Fri, May 04, 2012 at 06:43:30PM +0100, Al Viro wrote:
> > 	Consider that preemptively NAKed.  Eric, would you mind explaining
> > WTF would fsnotify *do* with vfsmount(s) here, seeing that they might very
> > well be not mounted in anyone's namespace?  And as for generating an event
> > for each, they might appear and disappear at zero notice.
> 
> Eric, any ideas on how to proceed?

Ping?

-- 
tejun

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Eric Paris]

On Tue, 2012-05-15 at 08:16 -0700, Tejun Heo wrote:
> On Mon, May 07, 2012 at 01:38:48PM -0700, Tejun Heo wrote:
> > On Fri, May 04, 2012 at 06:43:30PM +0100, Al Viro wrote:
> > > 	Consider that preemptively NAKed.  Eric, would you mind explaining
> > > WTF would fsnotify *do* with vfsmount(s) here, seeing that they might very
> > > well be not mounted in anyone's namespace?  And as for generating an event
> > > for each, they might appear and disappear at zero notice.
> > 
> > Eric, any ideas on how to proceed?
> 
> Ping?

Oh sorry, if Al's correct, and there is just absolutely no way to come
up with any kind of mnt for the path we have no choice.  Do it the way
you were.

-Eric

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Tejun Heo]

Hello,

On Tue, May 15, 2012 at 11:25:17AM -0400, Eric Paris wrote:
> On Tue, 2012-05-15 at 08:16 -0700, Tejun Heo wrote:
> > On Mon, May 07, 2012 at 01:38:48PM -0700, Tejun Heo wrote:
> > > On Fri, May 04, 2012 at 06:43:30PM +0100, Al Viro wrote:
> > > > 	Consider that preemptively NAKed.  Eric, would you mind explaining
> > > > WTF would fsnotify *do* with vfsmount(s) here, seeing that they might very
> > > > well be not mounted in anyone's namespace?  And as for generating an event
> > > > for each, they might appear and disappear at zero notice.
> > > 
> > > Eric, any ideas on how to proceed?
> > 
> > Ping?
> 
> Oh sorry, if Al's correct, and there is just absolutely no way to come
> up with any kind of mnt for the path we have no choice.  Do it the way
> you were.

The thing is that people have been requesting recursive notification
for cgroup events (especially cgroup becoming empty).  IIRC, that
can't be done without path, right?  Why so?  Can we change that?

Thanks.

-- 
tejun

Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)

[Alexander Nikiforov]

On 05/15/2012 07:28 PM, Tejun Heo wrote:
> Hello,
>
> On Tue, May 15, 2012 at 11:25:17AM -0400, Eric Paris wrote:
>> On Tue, 2012-05-15 at 08:16 -0700, Tejun Heo wrote:
>>> On Mon, May 07, 2012 at 01:38:48PM -0700, Tejun Heo wrote:
>>>> On Fri, May 04, 2012 at 06:43:30PM +0100, Al Viro wrote:
>>>>> 	Consider that preemptively NAKed.  Eric, would you mind explaining
>>>>> WTF would fsnotify *do* with vfsmount(s) here, seeing that they might very
>>>>> well be not mounted in anyone's namespace?  And as for generating an event
>>>>> for each, they might appear and disappear at zero notice.
>>>> Eric, any ideas on how to proceed?
>>> Ping?
>> Oh sorry, if Al's correct, and there is just absolutely no way to come
>> up with any kind of mnt for the path we have no choice.  Do it the way
>> you were.
> The thing is that people have been requesting recursive notification
> for cgroup events (especially cgroup becoming empty).  IIRC, that
> can't be done without path, right?  Why so?  Can we change that?
>
> Thanks.
>
PIng...

-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics

Re: [RFD/RFC v2] event about group change

[Alexander Nikiforov]

Hi,

Add to this discussion fsnotify developers. If I forgot someone, please 
add them.

On 04/26/2012 10:02 AM, Alexander Nikiforov wrote:
> Hi,
>
> Main idea of this patch described in
> http://comments.gmane.org/gmane.linux.kernel.cgroups/1448
>
> and according to Tejun advice
> http://permalink.gmane.org/gmane.linux.kernel.cgroups/1753
>
> need your opinion. I think it'll very useful functionality in every 
> user space tool which works with cgroup.
>


-- 
Best regards,
      Alex Nikiforov,
      Mobile SW, Advanced Software Group,
      Moscow R&D center, Samsung Electronics