Re: [PATCH 4/5] always lock the root (oldest) anon_vma

[Linus Torvalds]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/