Messages in this thread | | | Date | Tue, 7 Jun 2011 08:25:19 -0400 | From | Christoph Hellwig <> | Subject | Re: [patch 2/8] mm: memcg-aware global reclaim |
| |
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?
| |