From: Rik van Riel <[email protected]>
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.
Signed-off-by: Rik van Riel <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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 __HAVE_ARCH_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 __HAVE_ARCH_PTE_ACCESSIBLE
+#define pte_accessible(pte) pte_present(pte)
+#endif
+
#ifndef flush_tlb_fix_spurious_fault
#define flush_tlb_fix_spurious_fault(vma, address) flush_tlb_page(vma, address)
#endif
NAK NAK NAK.
On Thu, Oct 25, 2012 at 5:16 AM, Peter Zijlstra <[email protected]> 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.
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
So NAK on this whole patch. It's bad. It's ugly, it's wrong, and it's
actively buggy.
Linus
* Linus Torvalds <[email protected]> wrote:
> NAK NAK NAK.
>
> On Thu, Oct 25, 2012 at 5:16 AM, Peter Zijlstra <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Fixed-by: Linus Torvalds <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
[ Added Linus's review fixes. ]
Signed-off-by: Ingo Molnar <[email protected]>
---
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
On Thu, Oct 25, 2012 at 02:16:21PM +0200, Peter Zijlstra wrote:
> From: Rik van Riel <[email protected]>
>
> 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.
>
It feels like we are putting the cart before the horse to be taking TLB
flushing optimisations into account this early in the series. That
aside, what was wrong with the following patches?
autonuma: define _PAGE_NUMA
arch-dependant definition of a flag that happens to be PROT_NONE
on x86 but could be anything at all really which would help portability
autonuma: pte_numa() and pmd_numa()
makes pte_present do what you want
adds pte_numa and pmd_numa which potentially could have been
used instead of pte_accessible
autonuma: teach gup_fast about pmd_numa
sortof self-explanatory
and building on those? The arch-dependant nature of _PAGE_NUMA might have
avoided Linus sending the childrens college fund to the swear jar and
avoided this complaint;
===
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
====
The "autonuma: define _PAGE_NUMA" happens to use PROT_NONE but as an
implementation detail rather than by design and as a bonus point describes
what it is doing. The "autonuma" part in the title is misleading, it's
not autonuma-specific at all and could have been dropped or just renamed
"numa:"
--
Mel Gorman
SUSE Labs