linux-kernel - Re: [PATCH v7 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]

Message-Id: <20130813234629.4ce2ec70.akpm@linux-foundation.org>
Date:	Tue, 13 Aug 2013 23:46:29 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Chris Metcalf <cmetcalf@...era.com>
Cc:	Tejun Heo <tj@...nel.org>, <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 v7 2/2] mm: make lru_add_drain_all() selective

On Tue, 13 Aug 2013 19:32:37 -0400 Chris Metcalf <cmetcalf@...era.com> wrote:

> On 8/13/2013 7:29 PM, Tejun Heo wrote:
> > Hello,
> >
> > On Tue, Aug 13, 2013 at 06:53:32PM -0400, Chris Metcalf wrote:
> >>  int lru_add_drain_all(void)
> >>  {
> >> -	return schedule_on_each_cpu(lru_add_drain_per_cpu);
> >> +	return schedule_on_each_cpu_cond(lru_add_drain_per_cpu,
> >> +					 lru_add_drain_cond, NULL);

This version looks nice to me.  It's missing the conversion of
schedule_on_each_cpu(), but I suppose that will be pretty simple.

> > It won't nest and doing it simultaneously won't buy anything, right?
> 
> Correct on both counts, I think.

I'm glad you understood the question :(

What does "nest" mean?  lru_add_drain_all() calls itself recursively,
presumably via some ghastly alloc_percpu()->alloc_pages(GFP_KERNEL)
route?  If that ever happens then we'd certainly want to know about it.
Hopefully PF_MEMALLOC would prevent infinite recursion.

If "nest" means something else then please enlighten me!

As for "doing it simultaneously", I assume we're referring to
concurrent execution from separate threads.  If so, why would that "buy
us anything"?  Confused.  As long as each thread sees "all pages which
were in pagevecs at the time I called lru_add_drain_all() get spilled
onto the LRU" then we're good.  afaict the implementation will do this.

> > Wouldn't it be better to protect it with a mutex and define all
> > necessary resources statically (yeah, cpumask is pain in the ass and I
> > think we should un-deprecate cpumask_t for static use cases)?  Then,
> > there'd be no allocation to worry about on the path.
> 
> If allocation is a real problem on this path, I think this is probably

Well as you pointed out, alloc_percpu() can already do a GFP_KERNEL
allocation, so adding another GFP_KERNEL allocation won't cause great
problems.  But the patchset demonstrates that the additional allocation
isn't needed.

> OK, though I don't want to speak for Andrew.  You could just guard it
> with a trylock and any caller that tried to start it while it was
> locked could just return happy that it was going on.
> 
> I'll put out a version that does that and see how that looks
> for comparison's sake.

That one's no good.  If thread A is holding the mutex, thread B will
bale out and we broke lru_add_drain_all()'s contract, "all pages which
...", above.

--
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/