Re: [PATCH v4 2/2] mm: make lru_add_drain_all() selective

[Andrew Morton]

On Tue, 13 Aug 2013 18:07:00 -0400 Tejun Heo <tj@xxxxxxxxxx> wrote:

> Hello,
> 
> On Tue, Aug 13, 2013 at 02:16:21PM -0700, Andrew Morton wrote:
> > I've yet to see any evidence that callback APIs have been abused and
> > I've yet to see any reasoning which makes me believe that this one will
> > be abused.
> 
> Well, off the top of my head.
> 
> * In general, it's clunkier.  Callbacks become artificial boundaries
>   across which context has to be carried over explicitly.  It often
>   involves packing data into a temporary struct.  The artificial
>   barrier also generally makes the logic more difficult to follow.
>   This is pretty general problem with callback based interface and why
>   many programming languages / conventions prefer iterator style
>   interface over callback based ones.  It makes the code a lot easier
>   to organize around the looping construct.  Here, it isn't as
>   pronounced because the thing naturally requires a callback anyway.
> 
> * From the API itself, it often isn't clear what restrictions the
>   context the callback is called under would have.  It sure is partly
>   documentation problem but is pretty easy to get wrong inadvertantly
>   as the code evolves and can be difficult to spot as the context
>   isn't apparent.
> 
> Moving away from callbacks started with higher level languages but the
> kernel sure is on the boat too where possible.  This one is muddier as
> the interface is async in nature but still it's at least partially
> applicable.

I don't buy it.  The callback simply determines whether "we need to
schuedule work on this cpu".  It's utterly simple.  Nobody will have
trouble understanding or using such a thing.

> > >  It feels a bit silly to me to push the API
> > > that way when doing so doesn't even solve the allocation problem.
> > 
> > It removes the need to perform a cpumask allocation in
> > lru_add_drain_all().
> 
> But that doesn't really solve anything, does it?

It removes one memory allocation and initialisation per call.  It
removes an entire for_each_online_cpu() loop.

> > >  It doesn't really buy us much while making the interface more complex.
> > 
> > It's a superior interface.
> 
> It is more flexible but at the same time clunkier.

The callback predicate is a quite natural thing in this case.

>  I wouldn't call it
> superior and the flexibility doesn't buy us much here.

It buys quite a lot and demonstrates why a callback interface is better.


I really don't understand what's going on here.  You're advocating for
a weaker kernel interface and for inferior kernel runtime behaviour. 
Forcing callers to communicate their needs via a large,
dynamically-allocated temporary rather than directly.  And what do we
get in return for all this?  Some stuff about callbacks which frankly
has me scratching my head.

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