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

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

> 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