On Tue 04-06-13 12:36:19, Tejun Heo wrote: > Hey, Michal. > > On Tue, Jun 04, 2013 at 03:45:23PM +0200, Michal Hocko wrote: > > Is this something that you find serious enough to block this series? > > I do not want to push hard but I would like to settle with something > > finally. This is taking way longer than I would like. > > I really don't think memcg can afford to add more mess than there > already is. Let's try to get things right with each change, please. Is this really about inside vs. outside skipping? I think this is a general improvement to the code. I really prefer not duplicating common code and skipping handling is such a code (we have a visitor which can control the walk). With a side bonus that it doesn't have to pollute vmscan more than necessary. Please be more specific about _what_ is so ugly about this interface so that it matters so much. > Can we please see how the other approach would look like? I have a > suspicion that it's likely be simpler but the devils are in the > details and all... > > > > The iteration only depends on the current position. Can't you factor > > > out skipping part outside the function rather than rolling into this > > > monstery thing with predicate callback? Just test the condition > > > outside and call a function to skip whatever is necessary? > > > > > > Also, cgroup_rightmost_descendant() can be pretty expensive depending > > > on how your tree looks like. > > > > I have no problem using something else. This was just the easiest to > > use and it behaves more-or-less good for hierarchies which are more or > > less balanced. If this turns out to be a problem we can introduce a > > new cgroup_skip_subtree which would get to last->sibling or go up the > > parent chain until there is non-NULL sibling. But what would be the next > > selling point here if we made it perfect right now? ;) > > Yeah, sure thing. I was just worried because the skipping here might > not be as good as the code seems to indicate. There will be cases, > which aren't too uncommon, where the skipping doesn't save much > compared to just continuing the pre-order walk, so.... And nobody > would really notice it unless [s]he looks really hard for it, which is > the more worrisome part for me. Maybe just stick a comment there > explaining that we probably want something better in the future? Sure thing. I will stick there a comment: diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 91740f7..43e955a 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1073,6 +1073,14 @@ skip_node: prev_cgroup = next_cgroup; goto skip_node; case SKIP_TREE: + /* + * cgroup_rightmost_descendant is not an optimal way to + * skip through a subtree (especially for imbalanced + * trees leaning to right) but that's what we have right + * now. More effective solution would be traversing + * right-up for first non-NULL without calling + * cgroup_next_descendant_pre afterwards. + */ prev_cgroup = cgroup_rightmost_descendant(next_cgroup); goto skip_node; case VISIT: Better? -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>