From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753308Ab0ELV6p (ORCPT ); Wed, 12 May 2010 17:58:45 -0400 Received: from smtp1.linux-foundation.org ([140.211.169.13]:34466 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751542Ab0ELV6n (ORCPT ); Wed, 12 May 2010 17:58:43 -0400 Date: Wed, 12 May 2010 14:55:33 -0700 (PDT) From: Linus Torvalds To: Rik van Riel cc: Andrew Morton , Mel Gorman , Andrea Arcangeli , Minchan Kim , Linux-MM , KAMEZAWA Hiroyuki , LKML Subject: Re: [PATCH 4/5] always lock the root (oldest) anon_vma In-Reply-To: <20100512134029.36c286c4@annuminas.surriel.com> Message-ID: References: <20100512133815.0d048a86@annuminas.surriel.com> <20100512134029.36c286c4@annuminas.surriel.com> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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