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

[Tejun Heo]

(cc'ing Li)

On Wed, Feb 6, 2013 at 4:55 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> 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?

Maybe we want to audit all the cases where the @node variable isn't
removed together?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/