[PATCH 04/31, v2] x86/mm: Introduce pte_accessible()

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> NAK NAK NAK.
> 
> On Thu, Oct 25, 2012 at 5:16 AM, Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> wrote:
> >
> > +#define __HAVE_ARCH_PTE_ACCESSIBLE
> > +static inline int pte_accessible(pte_t a)
> 
> Stop doing this f*cking crazy ad-hoc "I have some other name
> available" #defines.
> 
> Use the same name, for chissake! Don't make up new random names.
> 
> Just do
> 
>    #define pte_accessible pte_accessible
> 
> and then you can use
> 
>    #ifndef pte_accessible
> 
> to define the generic thing. Instead of having this INSANE "two
> different names for the same f*cking thing" crap.

Yeah...

> Stop it. Really.
> 
> Also, this:
> 
> > +#ifndef __HAVE_ARCH_PTE_ACCESSIBLE
> > +#define pte_accessible(pte)            pte_present(pte)
> > +#endif
> 
> looks unsafe and like a really bad idea.
> 
> You should probably do
> 
>   #ifndef pte_accessible
>     #define pte_accessible(pte) ((void)(pte),1)
>   #endif
> 
> because you have no idea if other architectures do
> 
>  (a) the same trick as x86 does for PROT_NONE (I can already tell you
> from a quick grep that ia64, m32r, m68k and sh do it)
>  (b) might not perhaps be caching non-present pte's anyway

Indeed that's much safer and each arch can opt-in consciously 
instead of us offering a potentially unsafe optimization.

> So NAK on this whole patch. It's bad. It's ugly, it's wrong, 
> and it's actively buggy.

I have fixed it as per the updated patch below. Only very 
lightly tested.

Thanks,

	Ingo

----------------------------->
Subject: x86/mm: Introduce pte_accessible()
From: Rik van Riel <riel@xxxxxxxxxx>
Date: Tue, 9 Oct 2012 15:31:12 +0200

We need pte_present to return true for _PAGE_PROTNONE pages, to indicate that
the pte is associated with a page.

However, for TLB flushing purposes, we would like to know whether the pte
points to an actually accessible page.  This allows us to skip remote TLB
flushes for pages that are not actually accessible.

Fill in this method for x86 and provide a safe (but slower) method
on other architectures.

Signed-off-by: Rik van Riel <riel@xxxxxxxxxx>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
Fixed-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
Link: http://lkml.kernel.org/n/tip-66p11te4uj23gevgh4j987ip@xxxxxxxxxxxxxx
[ Added Linus's review fixes. ]
Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
---
 arch/x86/include/asm/pgtable.h |    6 ++++++
 include/asm-generic/pgtable.h  |    4 ++++
 2 files changed, 10 insertions(+)

Index: tip/arch/x86/include/asm/pgtable.h
===================================================================
--- tip.orig/arch/x86/include/asm/pgtable.h
+++ tip/arch/x86/include/asm/pgtable.h
@@ -408,6 +408,12 @@ static inline int pte_present(pte_t a)
 	return pte_flags(a) & (_PAGE_PRESENT | _PAGE_PROTNONE);
 }
 
+#define pte_accessible pte_accessible
+static inline int pte_accessible(pte_t a)
+{
+	return pte_flags(a) & _PAGE_PRESENT;
+}
+
 static inline int pte_hidden(pte_t pte)
 {
 	return pte_flags(pte) & _PAGE_HIDDEN;
Index: tip/include/asm-generic/pgtable.h
===================================================================
--- tip.orig/include/asm-generic/pgtable.h
+++ tip/include/asm-generic/pgtable.h
@@ -219,6 +219,10 @@ static inline int pmd_same(pmd_t pmd_a,
 #define move_pte(pte, prot, old_addr, new_addr)	(pte)
 #endif
 
+#ifndef pte_accessible
+# define pte_accessible(pte)		((void)(pte),1)
+#endif
+
 #ifndef flush_tlb_fix_spurious_fault
 #define flush_tlb_fix_spurious_fault(vma, address) flush_tlb_page(vma, address)
 #endif
--
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/