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
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
