Re: [PATCH 2/7] cgroups: Allow to bind a subsystem to a cgroup hierarchy

[Li Zefan <lizf@cn.fujitsu.com>]

>> +/*
>> + * cgroup_walk_herarchy - iterate through a cgroup hierarchy
>> + * @process_cgroup: callback called on each cgroup in the hierarchy
>> + * @data: will be passed to @process_cgroup
>> + * @top_cgrp: the root cgroup of the hierarchy
>> + *
>> + * For such a hierarchy:
>> + *        a1        c1
>> + *      /         /
>> + * Root - a2 - b1 - c2
>> + *      \
>> + *        a3
>> + *
>> + * The iterating order is: a1, a2, b1, c1, c2, a3. So a parent will be
>> + * processed before its children.
>> + */
> 
> You could just say it's a depth-first walk except we process the parent before
> its children.
> 

Will revise the comment. A diagram is intuitive. :)

>> +static int cgroup_walk_hierarchy(int (*process_cgroup)(struct cgroup *, void *),
>> +				 void *data, struct cgroup *top_cgrp)
> 
> <snip>
> 
>> +static int hierarchy_populate_dir(struct cgroup *cgrp, void *data)
>> +{
>> +	mutex_lock_nested(&cgrp->dentry->d_inode->i_mutex, I_MUTEX_CHILD);
>> +	cgroup_populate_dir(cgrp);
>> +	mutex_unlock(&cgrp->dentry->d_inode->i_mutex);
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Call with cgroup_mutex held. Drops reference counts on modules, including
>>   * any duplicate ones that parse_cgroupfs_options took. If this function
>> @@ -945,36 +1079,53 @@ static int rebind_subsystems(struct cgroupfs_root *root,
>>  	unsigned long added_bits, removed_bits;
>>  	struct cgroup *cgrp = &root->top_cgroup;
>>  	int i;
>> +	int err;
>>
>>  	BUG_ON(!mutex_is_locked(&cgroup_mutex));
>>
>>  	removed_bits = root->actual_subsys_bits & ~final_bits;
>>  	added_bits = final_bits & ~root->actual_subsys_bits;
>> +
>>  	/* Check that any added subsystems are currently free */
>> -	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>> -		unsigned long bit = 1UL << i;
>> -		struct cgroup_subsys *ss = subsys[i];
>> -		if (!(bit & added_bits))
>> -			continue;
>> +	for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT) {
>>  		/*
>>  		 * Nobody should tell us to do a subsys that doesn't exist:
>>  		 * parse_cgroupfs_options should catch that case and refcounts
>>  		 * ensure that subsystems won't disappear once selected.
>>  		 */
>> -		BUG_ON(ss == NULL);
>> -		if (ss->root != &rootnode) {
>> +		BUG_ON(subsys[i] == NULL);
>> +		if (subsys[i]->root != &rootnode) {
>>  			/* Subsystem isn't free */
>>  			return -EBUSY;
>>  		}
>>  	}
>>
>> -	/* Currently we don't handle adding/removing subsystems when
>> -	 * any child cgroups exist. This is theoretically supportable
>> -	 * but involves complex error handling, so it's being left until
>> -	 * later */
>> -	if (root->number_of_cgroups > 1)
>> +	/* removing will be supported later */
>> +	if (root->number_of_cgroups > 1 && removed_bits)
>>  		return -EBUSY;
>>
>> +	if (root->number_of_cgroups > 1) {
> 
> Is there something wrong with the indentation here? I can't
> see the closing brace for the "if (root->number_of_cgroups > 1)"
> that should precede the for_each_set_bit() loop below.
> 

Oops, I must have forgot to commit the change when I fixed this.

>> +		for_each_set_bit(i, &added_bits, CGROUP_SUBSYS_COUNT)
>> +			if (!subsys[i]->can_bind)
>> +				return -EBUSY;
> 
> I think you could avoid the can_bind flag field entirely and do:
> 
> 			if (!subsys[i]->bind)
> 

Nope. For some subsystems we just set the flag and need not to
provide a bind() callback.

> Also, if we're going with my "split out unbind" suggestion I think
> the part here would be:
> 
> 		for_each_set_bit(i, &removed_bits, CGROUP_SUBSYS_COUNT)
> 			if (!subsys[i]->unbind)
> 				return -EBUSY;
>