On 05/03/2012 07:50 PM, Tejun Heo wrote:
Signed-off-by: Alex Nikiforov<a.nikiforov@xxxxxxxxxxx>
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
--
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
