From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail190.messagelabs.com (mail190.messagelabs.com [216.82.249.51]) by kanga.kvack.org (Postfix) with ESMTP id 153366B0093 for ; Mon, 29 Nov 2010 05:23:29 -0500 (EST) Date: Mon, 29 Nov 2010 10:23:11 +0000 From: Mel Gorman Subject: Re: [PATCH 18 of 66] add pmd mangling functions to x86 Message-ID: <20101129102310.GC13268@csn.ul.ie> References: <20101118130446.GO8135@csn.ul.ie> <20101126175751.GY6118@random.random> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <20101126175751.GY6118@random.random> Sender: owner-linux-mm@kvack.org To: Andrea Arcangeli Cc: linux-mm@kvack.org, Linus Torvalds , Andrew Morton , linux-kernel@vger.kernel.org, Marcelo Tosatti , Adam Litke , Avi Kivity , Hugh Dickins , Rik van Riel , Dave Hansen , Benjamin Herrenschmidt , Ingo Molnar , Mike Travis , KAMEZAWA Hiroyuki , Christoph Lameter , Chris Wright , bpicco@redhat.com, KOSAKI Motohiro , Balbir Singh , "Michael S. Tsirkin" , Peter Zijlstra , Johannes Weiner , Daisuke Nishimura , Chris Mason , Borislav Petkov List-ID: On Fri, Nov 26, 2010 at 06:57:51PM +0100, Andrea Arcangeli wrote: > On Thu, Nov 18, 2010 at 01:04:46PM +0000, Mel Gorman wrote: > > On Wed, Nov 03, 2010 at 04:27:53PM +0100, Andrea Arcangeli wrote: > > > From: Andrea Arcangeli > > > > > > Add needed pmd mangling functions with simmetry with their pte counterparts. > > > > symmetry > > Fixed. > > > > > > pmdp_freeze_flush is the only exception only present on the pmd side and it's > > > needed to serialize the VM against split_huge_page, it simply atomically clears > > > the present bit in the same way pmdp_clear_flush_young atomically clears the > > > accessed bit (and both need to flush the tlb to make it effective, which is > > > mandatory to happen synchronously for pmdp_freeze_flush). > > > > I don't see a pmdp_freeze_flush defined in the patch. Did yu mean > > pmdp_splitting_flush? Even if it is, it's the splitting bit you are > > dealing with which isn't the same as the present bit. I'm missing > > something. > > Well the comment went out of sync with the code sorry. I updated it: > > ======= > Add needed pmd mangling functions with symmetry with their pte counterparts. > pmdp_splitting_flush() is the only new addition on the pmd_ methods and it's > needed to serialize the VM against split_huge_page. It simply atomically sets > the splitting bit in a similar way pmdp_clear_flush_young atomically clears the > accessed bit. pmdp_splitting_flush() also has to flush the tlb to make it > effective against gup_fast, but it wouldn't really require to flush the tlb > too. Just the tlb flush is the simplest operation we can invoke to serialize > pmdp_splitting_flush() against gup_fast. > ======= > Much clearer, thanks. > > > Signed-off-by: Andrea Arcangeli > > > Acked-by: Rik van Riel > > > --- > > > > > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > > > --- a/arch/x86/include/asm/pgtable.h > > > +++ b/arch/x86/include/asm/pgtable.h > > > @@ -302,15 +302,15 @@ pmd_t *populate_extra_pmd(unsigned long > > > pte_t *populate_extra_pte(unsigned long vaddr); > > > #endif /* __ASSEMBLY__ */ > > > > > > +#ifndef __ASSEMBLY__ > > > +#include > > > + > > > #ifdef CONFIG_X86_32 > > > # include "pgtable_32.h" > > > #else > > > # include "pgtable_64.h" > > > #endif > > > > > > -#ifndef __ASSEMBLY__ > > > -#include > > > - > > > > Stupid quetion: Why is this move necessary? > > That's not a stupid question, it seems to build in all configurations > even with this part backed out. I'll try to reverse this one in the > hope that it won't break build. I suppose some earlier version of the > patchset required this to build (I would never make a gratuitous > change like this if it wasn't needed at some point) but it seems not > be required anymore according to my build tests. If I'm wrong and some > build breaks I'll reintroduce it later. > Ok. > > > static inline int pte_none(pte_t pte) > > > { > > > return !pte.pte; > > > @@ -353,7 +353,7 @@ static inline unsigned long pmd_page_vad > > > * Currently stuck as a macro due to indirect forward reference to > > > * linux/mmzone.h's __section_mem_map_addr() definition: > > > */ > > > -#define pmd_page(pmd) pfn_to_page(pmd_val(pmd) >> PAGE_SHIFT) > > > +#define pmd_page(pmd) pfn_to_page((pmd_val(pmd) & PTE_PFN_MASK) >> PAGE_SHIFT) > > > > > > > Why is it now necessary to use PTE_PFN_MASK? > > Just for the NX bit, that couldn't be set before the pmd could be > marked PSE. > Sorry, I still am missing something. PTE_PFN_MASK is this #define PTE_PFN_MASK ((pteval_t)PHYSICAL_PAGE_MASK) #define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK) I'm not seeing how PTE_PFN_MASK affects the NX bit (bit 63). > > The implementations look fine but I'm having trouble reconsiling what > > the leader says with the patch :( > > Yes because it was out of sync, the new version is above. > Thanks -- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/ Don't email: email@kvack.org