Re: [PATCH 3/3] memcg: simplify mem_cgroup_reclaim_iter

[Johannes Weiner <hannes@cmpxchg.org>]

Hi Tejun,

On Wed, Jun 05, 2013 at 01:06:12PM -0700, Tejun Heo wrote:
> On Wed, Jun 05, 2013 at 03:45:52PM -0400, Johannes Weiner wrote:
> > I'm not sure what you are suggesting.  Synchroneously invalidate every
> > individual iterator upwards the hierarchy every time a cgroup is
> > destroyed?
> 
> Yeap.

> > As I said, the weak pointers are only a few lines of code that can be
> > neatly self-contained (see the invalidate, load, store functions
> > below).  Please convince me that your alternative solution will save
> > complexity to such an extent that either the memory waste of
> > indefinite css pinning, or the computational overhead of non-lazy
> > iterator cleanup, is justifiable.
> 
> The biggest issue I see with the weak pointer is that it's special and
> tricky.  If this is something which is absolutely necessary, it should
> be somewhere more generic.  Also, if we can use the usual RCU deref
> with O(depth) cleanup in the cold path, I don't see how this deviation
> is justifiable.
>
> For people who've been looking at it for long enough, it probably
> isn't that different from using plain RCU but that's just because that
> person has spent the time to build that pattern into his/her brain.
> We now have a lot of people accustomed to plain RCU usages which in
> itself is tricky already and introducing new constructs is actively
> deterimental to maintainability.  We sure can do that when there's no
> alternative but I don't think avoiding synchronous cleanup on cgroup
> destruction path is a good enough reason.  It feels like an
> over-engineering to me.
>
> Another thing is that this matters the most when there are continuous
> creation and destruction of cgroups and the weak pointer
> implementation would keep resetting the iteration to the beginning.
> Depending on timing, it'd be able to live-lock reclaim cursor to the
> beginning of iteration even with fairly low rate of destruction,
> right?  It can be pretty bad high up the tree.  With synchronous
> cleanup, depending on how it's implemented, it can be made to keep the
> iteration position.

That could be an advantage, yes.  But keep in mind that every
destruction has to perform this invalidation operation against the
global root_mem_cgroup's nr_node * nr_zone * nr_priority_levels
iterators, so you can't muck around forever, while possibly holding a
lock at this level.  It's not a hot path, but you don't want to turn
it into one, either.

The upshot for me is this: whether you do long-term pinning or greedy
iterator invalidation, the cost of cgroup destruction increases.
Either in terms of memory usage or in terms of compute time.  I would
have loved to see something as simple as the long-term pinning work
out in practice, because it truly would have been simpler.  But at
this point, I don't really care much because the projected margins of
reduction in complexity and increase of cost from your proposal are
too small for me to feel strongly about one solution or the other, or
go ahead and write the code.  I'll look at your patches, though ;-)

Either way, I'll prepare the patch set that includes the barrier fix
and a small cleanup to make the weak pointer management more
palatable.  I'm still open to code proposals, so don't let it distract
you, but we might as well make it a bit more readable in the meantime.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>