linux-kernel - Re: [PATCH v4 2/2] mm: make lru_add_drain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 13 Aug 2013 15:18:05 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Tejun Heo <tj@...nel.org>
Cc:	Chris Metcalf <cmetcalf@...era.com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Thomas Gleixner <tglx@...utronix.de>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Cody P Schafer <cody@...ux.vnet.ibm.com>
Subject: Re: [PATCH v4 2/2] mm: make lru_add_drain_all() selective

On Tue, 13 Aug 2013 18:07:00 -0400 Tejun Heo <tj@...nel.org> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/