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

[Alexander Nikiforov <a.nikiforov-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

On 05/03/2012 07:50 PM, Tejun Heo wrote:
>> 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.
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