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

[Li Zefan <lizefan@huawei.com>]

On 2013/2/7 9:00, Tejun Heo wrote:
> (cc'ing Li)
> 
> On Wed, Feb 6, 2013 at 4:55 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> 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'.
>>
>> 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);
> 
> Yeah, looks correct & better to me.  Li?
> 

obviously correct.