Re: [PATCH v2] hlist: drop the node parameter from iterators

[Sasha Levin <sasha.levin@oracle.com>]

On 02/06/2013 07:55 PM, Andrew Morton wrote:
> On Wed, 30 Jan 2013 21:00:22 -0500
> Sasha Levin <sasha.levin@oracle.com> wrote:
> 
>> I'm not sure why, but the hlist for each entry iterators were conceived
>> differently from the list ones. While the list ones are nice and elegant:
>>
>>         list_for_each_entry(pos, head, member)
>>
>> The hlist ones were greedy and wanted an extra parameter:
>>
>>         hlist_for_each_entry(tpos, pos, head, member)
>>
>> Why did they need an extra pos parameter? I'm not quite sure. Not only
>> they don't really need it, it also prevents the iterator from looking
>> exactly like the list iterator, which is unfortunate.
>>
>> Besides the semantic patch, there was some manual work required:
>>
>>  - Fix up the actual hlist iterators in linux/list.h
>>  - Fix up the declaration of other iterators based on the hlist ones.
>>  - A very small amount of places were using the 'node' parameter, this
>>  was modified to use 'obj->member' instead.
>>  - Coccinelle didn't handle the hlist_for_each_entry_safe iterator
>>  properly, so those had to be fixed up manually.
>>
>> ...
>>
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>>
>> ...
>>
>> @@ -4525,7 +4524,7 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
>>  	 * this is all done under the css_set_lock.
>>  	 */
>>  	write_lock(&css_set_lock);
>> -	hash_for_each_safe(css_set_table, i, node, tmp, cg, hlist) {
>> +	hash_for_each_safe(css_set_table, i, tmp, cg, hlist) {
>>  		/* skip entries that we already rehashed */
>>  		if (cg->subsys[ss->subsys_id])
>>  			continue;
> 
> Problems in cgroup_load_subsys().
> 
> In linux-next, that function is now using the `node' storage which your
> patch removes:
> 
> @@ -4503,23 +4525,17 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
>  	 * this is all done under the css_set_lock.
>  	 */
>  	write_lock(&css_set_lock);
> -	for (i = 0; i < CSS_SET_TABLE_SIZE; i++) {
> -		struct css_set *cg;
> -		struct hlist_node *node, *tmp;
> -		struct hlist_head *bucket = &css_set_table[i], *new_bucket;
> -
> -		hlist_for_each_entry_safe(cg, node, tmp, bucket, hlist) {
> -			/* skip entries that we already rehashed */
> -			if (cg->subsys[ss->subsys_id])
> -				continue;
> -			/* remove existing entry */
> -			hlist_del(&cg->hlist);
> -			/* set new value */
> -			cg->subsys[ss->subsys_id] = css;
> -			/* recompute hash and restore entry */
> -			new_bucket = css_set_hash(cg->subsys);
> -			hlist_add_head(&cg->hlist, new_bucket);
> -		}
> +	hash_for_each_safe(css_set_table, i, node, tmp, cg, hlist) {
> +		/* skip entries that we already rehashed */
> +		if (cg->subsys[ss->subsys_id])
> +			continue;
> +		/* remove existing entry */
> +		hash_del(&cg->hlist);
> +		/* set new value */
> +		cg->subsys[ss->subsys_id] = css;
> +		/* recompute hash and restore entry */
> +		key = css_set_hash(cg->subsys);
> +		hash_add(css_set_table, node, key);     <<<<---- here
>  	}
>  	write_unlock(&css_set_lock);
>  
> 
> 
> This didn't show up (apart from a "used unintialized" warning) because
> your patch forgot to remove the definition of `node'.

Yikes, sorry :(

Coccinelle can't properly handle the case of removing just one
variable out of a declaration of two in the same line, and it has
slipped through my manual pass afterwards.

fwiw, I've solved all similar cases in the same way: "node" became
"obj->member".


Thanks,
Sasha

> I did this.  Tejun, could you please opine?
> 
> 
> @@ -4456,7 +4455,7 @@ int __init_or_module cgroup_load_subsys(
>  {
>  	struct cgroup_subsys_state *css;
>  	int i, ret;
> -	struct hlist_node *node, *tmp;
> +	struct hlist_node *tmp;
>  	struct css_set *cg;
>  	unsigned long key;
>  
> @@ -4534,7 +4533,7 @@ int __init_or_module cgroup_load_subsys(
>  		cg->subsys[ss->subsys_id] = css;
>  		/* recompute hash and restore entry */
>  		key = css_set_hash(cg->subsys);
> -		hash_add(css_set_table, node, key);
> +		hash_add(css_set_table, &cg->hlist, key);
>  	}
>  	write_unlock(&css_set_lock);
>  
>