Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756571Ab0DOUGE (ORCPT ); Thu, 15 Apr 2010 16:06:04 -0400 Received: from smtp1.linux-foundation.org ([140.211.169.13]:35782 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756437Ab0DOUGB (ORCPT ); Thu, 15 Apr 2010 16:06:01 -0400 Date: Thu, 15 Apr 2010 13:01:11 -0700 (PDT) From: Linus Torvalds To: Rik van Riel cc: Borislav Petkov , Johannes Weiner , KOSAKI Motohiro , Andrew Morton , Minchan Kim , Linux Kernel Mailing List , Lee Schermerhorn , Nick Piggin , Andrea Arcangeli , Hugh Dickins , sgunderson@bigfoot.com Subject: Re: [PATCH] rmap: add exclusively owned pages to the newest anon_vma In-Reply-To: <20100414175928.5e1bdce2@annuminas.surriel.com> Message-ID: References: <20100411185508.GA4450@liondog.tnic> <20100412072056.GA2432@liondog.tnic> <20100412215027.GA6263@liondog.tnic> <20100413093827.GA6954@liondog.tnic> <20100414175928.5e1bdce2@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 Content-Length: 3754 Lines: 105 On Wed, 14 Apr 2010, Rik van Riel wrote: > - /* > - * We must use the _oldest_ possible anon_vma for the page mapping! > - * > - * So take the last AVC chain entry in the vma, which is the deepest > - * ancestor, and use the anon_vma from that. > - */ > - avc = list_entry(vma->anon_vma_chain.prev, struct anon_vma_chain, same_vma); > - anon_vma = avc->anon_vma; > + if (exclusive) > + anon_vma = vma->anon_vma; > + else { > + /* > + * The page may be shared between multiple processes. > + * We must use the _oldest_ possible anon_vma for the > + * page mapping! That anon_vma is guaranteed to be > + * present in all processes that could share this page. > + * > + * So take the last AVC chain entry in the vma, which is the > + * deepest ancestor, and use the anon_vma from that. > + */ > + avc = list_entry(vma->anon_vma_chain.prev, struct anon_vma_chain, same_vma); > + anon_vma = avc->anon_vma; > + } I really dislike your coding style. If we do this conditionally, we're _much_ better off declaring the variables we only use inside that conditional block inside the block itself. And since we access "vma->anon_vma" in either case, just move that case outside the conditional statement, and avoid a pointless if/then/else. IOW, something like this. Totally untested. Linus --- mm/rmap.c | 26 +++++++++++++++----------- 1 files changed, 15 insertions(+), 11 deletions(-) diff --git a/mm/rmap.c b/mm/rmap.c index 4bad326..78d4730 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -732,21 +732,25 @@ void page_move_anon_rmap(struct page *page, * @address: the user virtual address mapped */ static void __page_set_anon_rmap(struct page *page, - struct vm_area_struct *vma, unsigned long address) + struct vm_area_struct *vma, unsigned long address, int exclusive) { - struct anon_vma_chain *avc; - struct anon_vma *anon_vma; + struct anon_vma *anon_vma = vma->anon_vma; - BUG_ON(!vma->anon_vma); + BUG_ON(!anon_vma); /* - * We must use the _oldest_ possible anon_vma for the page mapping! + * If the page isn't exclusively mapped into this vma, + * we must use the _oldest_ possible anon_vma for the + * page mapping! * - * So take the last AVC chain entry in the vma, which is the deepest - * ancestor, and use the anon_vma from that. + * So take the last AVC chain entry in the vma, which is + * the deepest ancestor, and use the anon_vma from that. */ - avc = list_entry(vma->anon_vma_chain.prev, struct anon_vma_chain, same_vma); - anon_vma = avc->anon_vma; + if (!exclusive) { + struct anon_vma_chain *avc; + avc = list_entry(vma->anon_vma_chain.prev, struct anon_vma_chain, same_vma); + anon_vma = avc->anon_vma; + } anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON; page->mapping = (struct address_space *) anon_vma; @@ -802,7 +806,7 @@ void page_add_anon_rmap(struct page *page, VM_BUG_ON(!PageLocked(page)); VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end); if (first) - __page_set_anon_rmap(page, vma, address); + __page_set_anon_rmap(page, vma, address, 0); else __page_check_anon_rmap(page, vma, address); } @@ -824,7 +828,7 @@ void page_add_new_anon_rmap(struct page *page, SetPageSwapBacked(page); atomic_set(&page->_mapcount, 0); /* increment count (starts at -1) */ __inc_zone_page_state(page, NR_ANON_PAGES); - __page_set_anon_rmap(page, vma, address); + __page_set_anon_rmap(page, vma, address, 1); if (page_evictable(page, vma)) lru_cache_add_lru(page, LRU_ACTIVE_ANON); else -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/