Message-ID: <20130607082502.GC8117@dhcp22.suse.cz>
Date:	Fri, 7 Jun 2013 10:25:02 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Tejun Heo <tj@...nel.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	linux-mm@...ck.org, cgroups@...r.kernel.org,
	linux-kernel@...r.kernel.org, Ying Han <yinghan@...gle.com>,
	Hugh Dickins <hughd@...gle.com>,
	Glauber Costa <glommer@...allels.com>,
	Michel Lespinasse <walken@...gle.com>,
	Greg Thelen <gthelen@...gle.com>,
	Balbir Singh <bsingharora@...il.com>
Subject: Re: [patch -v4 4/8] memcg: enhance memcg iterator to support
 predicates

On Thu 06-06-13 17:48:24, Tejun Heo wrote:
> On Wed, Jun 05, 2013 at 02:09:38AM -0700, Tejun Heo wrote:
> > On Wed, Jun 05, 2013 at 11:07:39AM +0200, Michal Hocko wrote:
> > > On Wed 05-06-13 01:58:49, Tejun Heo wrote:
> > > [...]
> > > > Anyways, so you aren't gonna try the skipping thing?
> > > 
> > > As I said. I do not consider this a priority for the said reasons (i
> > > will not repeat them).
> > 
> > That's a weird way to respond.  Alright, whatever, let me give it a
> > shot then.
> 
> So, there were some private exchanges and here's my main issue with
> the addition of predicate callback to mem_cgroup_iter_cond().
> 
> There are two common patterns that are used to implement iteration.
> One is the good ol' callback based one - ie. call_fn_on_each(fn) type
> interface.  The other is something which can be used as part of flow
> control by the user - be it give_me_next_elem() or for_each() type
> loop macro.  In majority of cases, especially for anything generic,
> the latter is considered to be the better choice because, while a bit
> more challenging to implement usually, it's a lot less cumbersome for
> the users of the interface.
> 
> mem_cgroup_iter_cond() seems icky to me because the predicate callback
> is essentially visit callback,

OK, I thought that the predicate signature made it clear that its
purpose is to _check_ whether visiting makes sense rather than _visit_
that node and work with the node. That is the reason why I didn't
include state parameter which would be expected for the full visitor.
Maybe using const would make it even more clear. I can update
documentation for the predicate to make it more clear.

> so now we end up with give_me_next_elem() with visit callback, which
> is fundamentally superflous.  If it were properly call_fn_on_each(fn),
> the return values would be CONTINUE, SKIP_SUBTREE or ABORT, which
> makes more sense to me.  Sure, it can be said that the predicate
> callback is for a different purpose but it really doesn't change that
> the interface now is visiting the same node in two different places.
> If it were something remotely widely used, it won't take much time
> developing braindamaged usages where part is being done inside the
> predicate callback and the rest is done outside without clear reason
> why just because of natural code growth.  I don't think this is the
> type of construct that we want in kernel in general.
> 
> That said, it also is true that doing this is the shortest path to
> implementing subtree skip given how the iterator is put together
> currently and the series as a whole reduces significant amount of
> complexity, so it is an acceptable tradeoff to proceed with this
> implementation with later restructuring of the iterator.

Good. As I said many times, memcg iterators could see some clean ups.

> So, let's go ahead as proposed.  

Thanks!

> I'll try to rework the iterator on top of it, and my aplogies to
> Michal for being over-the-top.

Thanks!

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists