linux-kernel - Re: [PATCH 4/5] always lock the root (oldest) anon

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:	Wed, 12 May 2010 14:55:33 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Rik van Riel <riel@...hat.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Mel Gorman <mel@....ul.ie>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Minchan Kim <minchan.kim@...il.com>,
	Linux-MM <linux-mm@...ck.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/5] always lock the root (oldest) anon_vma

On Wed, 12 May 2010, Rik van Riel wrote:
> 
> Always (and only) lock the root (oldest) anon_vma whenever we do 
> something in an anon_vma.  The recently introduced anon_vma scalability 
> is due to the rmap code scanning only the VMAs that need to be scanned.  
> Many common operations still took the anon_vma lock on the root 
> anon_vma, so always taking that lock is not expected to introduce any 
> scalability issues.

Ack for this (and the whole series, for that matter - looks fine to me). 

Somebody should run the performance numbers with AIM7 or whatever, just to 
check that the lock isn't a problem, but this approach certainly gets rid 
of all my objections about crazy locking. 

That patch #5 is pretty ugly, though. And I think this part (in 
drop_anon_vma) is approaching being wrong:

+       if (atomic_dec_and_lock(&anon_vma->ksm_refcount, &anon_vma->root->lock)) {

because I do _not_ believe that you need to decrement that ksm_refcount 
under the lock, do you? It's just a refcount, isn't it?

Wouldn't it be sufficient to do

	if (atomic_dec_and_test(&anon_vma->ksm_refcount)) {
		anon_vma_lock(anon_vma);

instead? The "atomic_dec_and_lock()" semantics are _much_ stricter than a 
regular "decrement and test and then lock", and that strictness means that 
it's way more complicated and expensive. So if you don't need the 
semantics, you shouldn't use them.

But maybe we do need those "lock before decrementing to zero" semantics. 
The old ksm.c code had it too, although I suspect it's just being 
confused.

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