Re: [patch 2/8] mm: memcg-aware global reclaim

Christoph Hellwig
2011-06-07T12:25:37Z

A few small nitpicks:

> +struct mem_cgroup *mem_cgroup_hierarchy_walk(struct mem_cgroup *root,
> + struct mem_cgroup *prev)
> +{
> + struct mem_cgroup *mem;
> +
> + if (mem_cgroup_disabled())
> + return NULL;
> +
> + if (!root)
> + root = root_mem_cgroup;
> + /*
> + * Even without hierarchy explicitely enabled in the root
> + * memcg, it is the ultimate parent of all memcgs.
> + */
> + if (!(root == root_mem_cgroup || root->use_hierarchy))
> + return root;

The logic here reads a bit weird, why not simply:

/*
* Even without hierarchy explicitely enabled in the root
* memcg, it is the ultimate parent of all memcgs.
*/
if (!root || root == root_mem_cgroup)
return root_mem_cgroup;
if (root->use_hierarchy)
return root;

> /*
> * This is a basic per-zone page freer. Used by both kswapd and direct reclaim.
> */
> -static void shrink_zone(int priority, struct zone *zone,
> - struct scan_control *sc)
> +static void do_shrink_zone(int priority, struct zone *zone,
> + struct scan_control *sc)

It actually is the per-memcg shrinker now, and thus should be called
shrink_memcg.

> + sc->mem_cgroup = mem;
> + do_shrink_zone(priority, zone, sc);

Any passing the mem_cgroup explicitly instead of hiding it in the
scan_control would make that much more obvious. If there's a good
reason to pass it in the structure the same probably applies to the
zone and priority, too.

Shouldn't we also have a non-cgroups stub of shrink_zone to directly
call do_shrink_zone/shrink_memcg with a NULL memcg and thus optimize
the whole loop away for it?