> 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.
> +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
--
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
