From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757131Ab0EFJrQ (ORCPT ); Thu, 6 May 2010 05:47:16 -0400 Received: from mail-iw0-f197.google.com ([209.85.223.197]:55486 "EHLO mail-iw0-f197.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751365Ab0EFJrN convert rfc822-to-8bit (ORCPT ); Thu, 6 May 2010 05:47:13 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=Jg0b05Q9Ry4tCVSXJOv3Xp5M+bVoM2hVT3R40VMtSJYSynvNdVC6tb36uxKgeU2Rp3 V2Q5Ez1Iv7b/h4LDmZg5ZxUNOx/DVL6YzybGgX0PUMAEVqzTEUa7MGFYfVTy9RIkSP3k GA93bJXrff/SY2e6fwQr7ajdtuXsPkh38eq6A= MIME-Version: 1.0 In-Reply-To: <20100506002255.GY20979@csn.ul.ie> References: <1273065281-13334-1-git-send-email-mel@csn.ul.ie> <1273065281-13334-2-git-send-email-mel@csn.ul.ie> <20100505145620.GP20979@csn.ul.ie> <20100505175311.GU20979@csn.ul.ie> <20100506002255.GY20979@csn.ul.ie> Date: Thu, 6 May 2010 18:47:12 +0900 Message-ID: Subject: Re: [PATCH 1/2] mm,migration: Prevent rmap_walk_[anon|ksm] seeing the wrong VMA information From: Minchan Kim To: Mel Gorman Cc: Linus Torvalds , Andrew Morton , Linux-MM , LKML , KAMEZAWA Hiroyuki , Christoph Lameter , Andrea Arcangeli , Rik van Riel Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 6, 2010 at 9:22 AM, Mel Gorman wrote: > On Wed, May 05, 2010 at 11:02:25AM -0700, Linus Torvalds wrote: >> >> >> On Wed, 5 May 2010, Mel Gorman wrote: >> > >> > If the same_vma list is properly ordered then maybe something like the >> > following is allowed? >> >> Heh. This is the same logic I just sent out. However: >> >> > +   anon_vma = page_rmapping(page); >> > +   if (!anon_vma) >> > +           return NULL; >> > + >> > +   spin_lock(&anon_vma->lock); >> >> RCU should guarantee that this spin_lock() is valid, but: >> >> > +   /* >> > +    * Get the oldest anon_vma on the list by depending on the ordering >> > +    * of the same_vma list setup by __page_set_anon_rmap >> > +    */ >> > +   avc = list_entry(&anon_vma->head, struct anon_vma_chain, same_anon_vma); >> >> We're not guaranteed that the 'anon_vma->head' list is non-empty. >> >> Somebody could have freed the list and the anon_vma and we have a stale >> 'page->anon_vma' (that has just not been _released_ yet). >> >> And shouldn't that be 'list_first_entry'? Or &anon_vma->head.next? >> >> How did that line actually work for you? Or was it just a "it boots", but >> no actual testing of the rmap walk? >> > > This is what I just started testing on a 4-core machine. Lockdep didn't > complain but there are two potential sources of badness in anon_vma_lock_root > marked with XXX. The second is the most important because I can't see how the > local and root anon_vma locks can be safely swapped - i.e. release local and > get the root without the root disappearing. I haven't considered the other > possibilities yet such as always locking the root anon_vma. Going to > sleep on it. > > Any comments? > +/* Given an anon_vma, find the root of the chain, lock it and return the root */ > +struct anon_vma *anon_vma_lock_root(struct anon_vma *anon_vma) > +{ > +       struct anon_vma *root_anon_vma; > +       struct anon_vma_chain *avc, *root_avc; > +       struct vm_area_struct *vma; > + > +       /* Lock the same_anon_vma list and make sure we are on a chain */ > +       spin_lock(&anon_vma->lock); > +       if (list_empty(&anon_vma->head)) { > +               spin_unlock(&anon_vma->lock); > +               return NULL; > +       } > + > +       /* > +        * Get the root anon_vma on the list by depending on the ordering > +        * of the same_vma list setup by __page_set_anon_rmap. Basically > +        * we are doing > +        * > +        * local anon_vma -> local vma -> deepest vma -> anon_vma > +        */ > +       avc = list_first_entry(&anon_vma->head, struct anon_vma_chain, same_anon_vma); Dumb question. I can't understand why we should use list_first_entry. I looked over the code. anon_vma_chain_link uses list_add_tail so I think that's right. But anon_vma_prepare uses list_add. So it's not consistent. How do we make sure list_first_entry returns deepest vma? Sorry if I am missing. -- Kind regards, Minchan Kim