event about group change
PATCH v2 1/1 event about group change
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.
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsA message that is not in the archive (sent off-list or not yet received): [email protected]
include/linux/cgroup.h +4 -1
@@ -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
kernel/cgroup.c +44 -4
112 lines not on this page · raw message
A message that is not in the archive (sent off-list or not yet received): [email protected]
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.
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsHello,
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?
include/linux/cgroup.h struct cgroup { · line 182
Quoted 5 lines
include/linux/cgroup.h +1
-0@@ -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.
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 703
Quoted 1 line
kernel/cgroup.c +1
-0+static inline void fsnotify_cgroup (struct task_struct *tsk,__u32 mask)
No need to make it inline.
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 707
Quoted 4 lines
kernel/cgroup.c +4
-0+{ +struct cgroupfs_root *root; +struct inode *d_inode; +struct cgroup *cgrp;
What are the locking rules?
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 714
Quoted 6 lines
kernel/cgroup.c +6
-0+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?
kernel/cgroup.c static umode_t cgroup_file_mode(const struct cftype *cft) · line 2628
Quoted 5 lines
int cgroup_add_file(struct cgroup *cgrp,
kernel/cgroup.c +2
-1struct cgroup_subsys *subsys, - const struct cftype *cft) + const struct cftype *cft, + int tasks)
Ugh... this is ugly.
kernel/cgroup.c int cgroup_add_file(struct cgroup *cgrp, · line 2653
Quoted 9 lines
{
kernel/cgroup.c +2
-0struct 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
kernel/cgroup.c int cgroup_add_file(struct cgroup *cgrp, · line 2654
Quoted 1 line
kernel/cgroup.c +1
-0+pr_warn ("%s(): cft name: %s\n" , __func__, name);
Why pr_warn?
Quoted 23 lines
kernel/cgroup.c +7
-2+ 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;
kernel/cgroup.c static int cgroup_populate_dir(struct cgroup *cgrp) · line 3682
Quoted 10 lines
kernel/cgroup.c +3
-1+ + 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?
kernel/cgroup.c static const struct file_operations proc_cgroupstats_operations = { · line 4513
Quoted 5 lines
kernel/cgroup.c +1
-0@@ -4480,6 +4510,7 @@ static const struct file_operations proc_cgroupstats_operations = {*/ void cgroup_fork (struct task_struct *child) { +
Why the new line?
Thanks.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
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
Quoted 6 lines · 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
About for-3.5 - sure. I'll do it. Sorry, but what is SOB? :)
Quoted 2 lines
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.
Quoted 7 lines
include/linux/cgroup.h struct cgroup { · line 182
Quoted 5 lines
include/linux/cgroup.h +1
-0@@ -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.
Quoted 2 lines
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 703
Quoted 1 line
kernel/cgroup.c +1
-0+static inline void fsnotify_cgroup (struct task_struct *tsk,__u32 mask)No need to make it inline.
Ok
Quoted 5 lines
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 707
Quoted 4 lines
kernel/cgroup.c +4
-0+{ +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??
Quoted 9 lines
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 714
Quoted 6 lines
kernel/cgroup.c +6
-0+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
Quoted 6 lines
kernel/cgroup.c static umode_t cgroup_file_mode(const struct cftype *cft) · line 2628
Quoted 5 lines
int cgroup_add_file(struct cgroup *cgrp,
structcgroup_subsys*subsys,kernel/cgroup.c +2
-1- 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.
Quoted 11 lines
kernel/cgroup.c int cgroup_add_file(struct cgroup *cgrp, · line 2653
Quoted 9 lines
{ struct dentry *dir = cgrp->dentry; struct dentry *dentry;kernel/cgroup.c +2
-0@@ -2629,6 +2649,12 @@ int cgroup_add_file(struct cgroup *cgrp,dput (dentry); }else error =PTR_ERR (dentry); + +if (tasks) {^ missing space
Quoted 2 lines
kernel/cgroup.c int cgroup_add_file(struct cgroup *cgrp, · line 2654
Quoted 1 line
kernel/cgroup.c +1
-0+pr_warn ("%s(): cft name: %s\n" , __func__, name);Why
pr_warn?
sorry, I sent this patch RFC, forgot to clean up.
Quoted 29 lines
Quoted 29 lines
kernel/cgroup.c +9
-2+ 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;
Quoted 8 lines
kernel/cgroup.c static int cgroup_populate_dir(struct cgroup *cgrp) · line 3682
Quoted 5 lines
kernel/cgroup.c +1
-1if (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
Quoted 6 lines
kernel/cgroup.c static const struct file_operations proc_cgroupstats_operations = { · line 4513
Quoted 5 lines
kernel/cgroup.c +1
-0@@ -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
Quoted 3 lines
Thanks.
Thanks.
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsA message that is not in the archive (sent off-list or not yet received): [email protected]
Hello,
Quoted 3 lines · Alexander Nikiforov
Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.gitfor-3.5About for-3.5 - sure. I'll do it. Sorry, but what is SOB? :)
Signed-off-by tag. Sorry about the abbreviation.
Quoted 4 lines
Quoted 2 lines
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 [email protected]
Quoted 8 lines
Quoted 7 lines
include/linux/cgroup.h struct cgroup { · line 182
Quoted 5 lines
include/linux/cgroup.h +1
-0@@ -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.
Quoted 6 lines
Are you talking about struct cftype. If yes, I think for now put
tasks_dentryinto 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.
Quoted 7 lines
Quoted 5 lines
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 707
Quoted 4 lines
kernel/cgroup.c +4
-0+{ +struct cgroupfs_root *root; +struct inode *d_inode; +struct cgroup *cgrp;What are the locking rules?
fsnotify_cgroup()called insidecgroup_lock(), is it not sufficient??
I was hoping for lockdep_assert_held() annotation. :)
Quoted 7 lines
Quoted 3 lines
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.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): 20120428214131.GB4586-9pTldWuhBndy/[email protected]
Quoted 6 lines · Tejun Heo
Hello,
Quoted 2 lines · Alexander Nikiforov
Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.5About for-3.5 - sure. I'll do it. Sorry, but what is SOB? :)
Signed-off-by tag. Sorry about the abbreviation.
done
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
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 [email protected]
done
Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
Review struct cgroup { · line 182
Quoted 6 lines
(quoted diff) +1
-0@@ -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.
Quoted 6 lines
Are you talking about struct cftype. If yes, I think for now put
tasks_dentryinto 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.
Quoted 3 lines
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
Quoted 8 lines
Quoted 7 lines
Quoted 6 lines
Review struct cgroup { · line 186
Quoted 5 lines
(quoted diff) +4
-0+{ + struct cgroupfs_root *root; + struct inode *d_inode; + struct cgroup *cgrp;What are the locking rules?
fsnotify_cgroup()called insidecgroup_lock(), is it not sufficient??I was hoping for
lockdep_assert_held()annotation. :)
done
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
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_callbacksI 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
Quoted 3 lines
Thanks.
Please review new version
Thanks
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsPATCH 1/1 cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
In reply to a message not in the archiveinclude/linux/cgroup.h +2 -0
@@ -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 ];
kernel/cgroup.c +52 -16
130 lines not on this page · raw message
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 1 line
Signed-off-by Alex Nikiforov <[email protected]>
Please read how other patches are being posted. You need to explain what the patch does and why.
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 746
Quoted 5 lines
kernel/cgroup.c +5
-0+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.
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 752
Quoted 5 lines
kernel/cgroup.c +5
-0+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?
kernel/cgroup.c int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) · line 1999
Quoted 13 lines
kernel/cgroup.c +6
-0+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.
Quoted 33 lines
kernel/cgroup.c +6
-3+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;
kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2740
Quoted 10 lines
kernel/cgroup.c +2
-4list_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?
Quoted 14 lines
kernel/cgroup.c +2
-6static 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) {
kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2756
Quoted 10 lines
kernel/cgroup.c +7
-3-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.
kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2763
Quoted 7 lines
kernel/cgroup.c +7
-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 ; + }
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.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 3 lines · Tejun Heo
Quoted 1 line
Signed-off-by Alex Nikiforov<[email protected]>Please read how other patches are being posted. You need to explain what the patch does and why.
Sorry, I will.
Quoted 9 lines
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 746
Quoted 6 lines
kernel/cgroup.c +5
-0+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.
Quoted 6 lines
kernel/cgroup.c static struct cgroup *task_cgroup_from_root(struct task_struct *task, · line 752
Quoted 5 lines
kernel/cgroup.c +5
-0+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
Quoted 13 lines
Quoted 13 lines
kernel/cgroup.c +5
-0+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; }
Quoted 2 lines
kernel/cgroup.c int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk) · line 1999
Quoted 1 line
kernel/cgroup.c +1
-0+/* cgroup_attach_task() already guarded by cgroup_lock by caller */ If you have lockdep annotation, I don't think this is necessary.
Ok
Quoted 30 lines
Quoted 30 lines
kernel/cgroup.c +6
-3+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 ; }
Quoted 15 lines
Quoted 4 lines
kernel/cgroup.c +0
-0mode =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;kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2740
Quoted 10 lines
kernel/cgroup.c +2
-4list_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.
Quoted 28 lines
Quoted 15 lines
kernel/cgroup.c +2
-6static 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) {kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2756
Quoted 10 lines
kernel/cgroup.c +7
-3-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.
Quoted 1 line
Please read Documents/CodingStyle.
Sorry, fixed.
Quoted 14 lines
kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2763
Quoted 8 lines
kernel/cgroup.c +7
-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 ; + }Please don't bury this inside
cgroup_addrm_files()withstrcmp()trying to find out again which one is the task file - in general, if you're doingstrcmp()inside kernel which isn't parsing userland input, something is wrong. Just separate out the cftype of task file into a standalone entry and letcgroup_populate_dir()callcgroup_add_file()on it and then record the returned cfe.
done
Quoted 3 lines
Thanks.
Thx, for your time.
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsA message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Hello,
Quoted 36 lines · Alexander Nikiforov
Quoted 20 lines
Quoted 20 lines
kernel/cgroup.c +4
-3-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 ; }Quoted 15 lines
Quoted 4 lines
kernel/cgroup.c +0
-0mode =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;kernel/cgroup.c static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2740
Quoted 10 lines
kernel/cgroup.c +2
-4list_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?
Quoted 3 lines
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.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Hi, Tejun
Quoted 38 lines · Tejun Heo
Hello,
Quoted 35 lines · Alexander Nikiforov
Quoted 35 lines
Quoted 24 lines
(quoted diff) +4
-3-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;Review static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2740
Quoted 10 lines
(quoted diff) +2
-4list_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?
Quoted 5 lines
Quoted 3 lines
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.
Quoted 3 lines
Thanks.
What about other in v4, any suggestion?
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsPATCH v5 1/1 event about group change
In reply to a message not in the archiveFix 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(-)
Signature · 1 line
1.7.5.4
A message that is not in the archive (sent off-list or not yet received): 1336197047-22145-1-git-send-email-a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org
PATCH v5 1/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(-)
kernel/cgroup.c +69 -19
@@ -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) {
Signature · 1 line
1.7.5.4
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
In reply to a message not in the archiveHi
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??
Quoted 46 lines · Tejun Heo
Hello,
Quoted 35 lines · Alexander Nikiforov
Quoted 20 lines
Quoted 20 lines
(quoted diff) +4
-3-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; }Quoted 15 lines
Quoted 4 lines
(quoted diff) +0
-0mode = 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;Review static int cgroup_add_file(struct cgroup *cgrp, struct cgroup_subsys *subsys, · line 2740
Quoted 10 lines
(quoted diff) +2
-4list_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?
Quoted 3 lines
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.
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsRe: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
In reply to a message not in the archiveOn 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)
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.
Quoted 10 lines · 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_PATHinstead of inode. inode works for dnotify and inotify, but not for things watched with fanotify.-Eric
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsA message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 7 lines · 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 thesys_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.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 9 lines · Tejun Heo
Quoted 7 lines · 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 thesys_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.
Quoted 3 lines
Hmm... we have dentry from
cfe->dentryand we can either use the first vfsmount fromsb->s_mountsor 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...
A message that is not in the archive (sent off-list or not yet received): 20120504174330.GS6871-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 4 lines · Al Viro
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.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 7 lines · Tejun Heo
Quoted 4 lines · Al Viro
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?
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 10 lines · Tejun Heo
Quoted 7 lines · Tejun Heo
Quoted 4 lines · Al Viro
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)
Hello,
Quoted 8 lines · Eric Paris
Quoted 7 lines · Tejun Heo
Quoted 6 lines · Tejun Heo
Quoted 4 lines · Al Viro
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.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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.
Signature · 1 line
tejun
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH -V3 1/1] cgroup: Add inotify event on change tasks file (fork, exit, move pid from file)
Quoted 12 lines · Tejun Heo
Hello,
Quoted 9 lines · Eric Paris
Quoted 8 lines · Tejun Heo
Quoted 6 lines · Tejun Heo
Quoted 4 lines · Al Viro
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?
Quoted 9 lines
Quoted 3 lines
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...
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung ElectronicsHi,
Add to this discussion fsnotify developers. If I forgot someone, please add them.
Quoted 10 lines · Alexander Nikiforov
Hi,
Main idea of this patch described in
http://comments.gmane.org/gmane.linux.kernel.cgroups/1448and according to Tejun advice
http://permalink.gmane.org/gmane.linux.kernel.cgroups/1753need your opinion. I think it'll very useful functionality in every user space tool which works with cgroup.
Signature · 4 lines
Best regards,
Alex Nikiforov,
Mobile SW, Advanced Software Group,
Moscow R&D center, Samsung Electronics