Messages in this thread |  | | | Date | Thu, 9 Jun 2011 05:26:17 -0400 | | From | Christoph Hellwig <> | | Subject | Re: [patch 2/8] mm: memcg-aware global reclaim |
| |
On Wed, Jun 08, 2011 at 11:30:46AM +0200, Johannes Weiner wrote: > On Tue, Jun 07, 2011 at 08:25:19AM -0400, Christoph Hellwig wrote: > > 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; > > What you are proposing is not equivalent, so... case in point! It's > meant to do the hierarchy walk for when foo->use_hierarchy, obviously, > but ALSO for root_mem_cgroup, which is parent to everyone else even > without use_hierarchy set. I changed it to read like this: > > if (!root) > root = root_mem_cgroup; > if (!root->use_hierarchy && root != root_mem_cgroup) > return root; > /* actually iterate hierarchy */ > > Does that make more sense?
It does, sorry for misparsing it. The thing that I really hated was the conditional assignment of root. Can we clean this up somehow by making the caller pass root_mem_cgroup in the case where it passes root right now, or at least always pass NULL when it means root_mem_cgroup.
Note really that important in the end, it just irked me when I looked over it, especially the conditional assigned of root to root_mem_cgroup, and then a little later checking for the equality of the two.
Thinking about it it's probably better left as-is for now to not complicate the series, and maybe revisit it later once things have settled a bit.
> > It actually is the per-memcg shrinker now, and thus should be called > > shrink_memcg. > > Per-zone per-memcg, actually. shrink_zone_memcg?
Sounds fine to me.
> I have gcc version 4.6.0 20110530 (Red Hat 4.6.0-9) (GCC) on this > machine, and it manages to optimize the loop away completely.
Ok, good enough.
|  |