On Thu, 2008-04-17 at 08:25 -0700, Linus Torvalds wrote:
>
> On Thu, 17 Apr 2008, Peter Zijlstra wrote:
> >
> > Would this be sufficient to address that comment's conern?
>
> It would be nicer to just add a "native_get_pte()" to x86, to match the
> already-existing "native_set_pte()".
See, I _knew_ I was missing something obvious :-/
> And that "barrier()" should b "smp_rmb()". They may be the same code
> sequence, but from a conceptual angle, "smp_rmb()" makes a whole lot more
> sense.
>
> Finally, I don't think that comment is correct in the first place. It's
> not that simple. The thing is, even *with* the memory barrier in place, we
> may have:
>
> CPU#1 CPU#2
> ===== =====
>
> fast_gup:
> - read low word
>
> native_set_pte_present:
> - set low word to 0
> - set high word to new value
>
> - read high word
>
> - set low word to new value
>
> and so you read a low word that is associated with a *different* high
> word! Notice?
>
> So trivial memory ordering is _not_ enough.
>
> So I think the code literally needs to be something like this
>
> #ifdef CONFIG_X86_PAE
>
> static inline pte_t native_get_pte(pte_t *ptep)
> {
> pte_t pte;
>
> retry:
> pte.pte_low = ptep->pte_low;
> smp_rmb();
> pte.pte_high = ptep->pte_high;
> smp_rmb();
> if (unlikely(pte.pte_low != ptep->pte_low)
> goto retry;
> return pte;
> }
>
> #else
>
> #define native_get_pte(ptep) (*(ptep))
>
> #endif
>
> but I have admittedly not really thought it fully through.
Looks sane here; Clark can you give this a spin?
Jeremy, did I get the paravirt stuff right?
---
Index: linux-2.6/arch/x86/mm/gup.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/gup.c
+++ linux-2.6/arch/x86/mm/gup.c
@@ -36,7 +36,7 @@ static noinline int gup_pte_range(pmd_t
* function that will do this properly, so it is broken on
* 32-bit 3-level for the moment.
*/
- pte_t pte = *ptep;
+ pte_t pte = get_pte(ptep);
struct page *page;
if ((pte_val(pte) & mask) != result)
Index: linux-2.6/arch/x86/kernel/paravirt_32.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/paravirt_32.c
+++ linux-2.6/arch/x86/kernel/paravirt_32.c
@@ -450,6 +450,7 @@ struct pv_mmu_ops pv_mmu_ops = {
#ifdef CONFIG_X86_PAE
.set_pte_atomic = native_set_pte_atomic,
.set_pte_present = native_set_pte_present,
+ .get_pte = native_get_pte,
.set_pud = native_set_pud,
.pte_clear = native_pte_clear,
.pmd_clear = native_pmd_clear,
Index: linux-2.6/include/asm-x86/paravirt.h
===================================================================
--- linux-2.6.orig/include/asm-x86/paravirt.h
+++ linux-2.6/include/asm-x86/paravirt.h
@@ -216,6 +216,7 @@ struct pv_mmu_ops {
void (*set_pte_atomic)(pte_t *ptep, pte_t pteval);
void (*set_pte_present)(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte);
+ void (*get_pte)(struct pte_t *ptep);
void (*set_pud)(pud_t *pudp, pud_t pudval);
void (*pte_clear)(struct mm_struct *mm, unsigned long addr, pte_t *ptep);
void (*pmd_clear)(pmd_t *pmdp);
@@ -886,6 +887,13 @@ static inline void set_pte_present(struc
pv_mmu_ops.set_pte_present(mm, addr, ptep, pte);
}
+static inline pte_t get_pte(struct pte_t *ptep)
+{
+ unsigned long long ret = PVOP_CALL1(unsigned long long, pv_mmu_ops.get_pte, ptep);
+
+ return (pte_t) { ret, ret >> 32 };
+}
+
static inline void set_pmd(pmd_t *pmdp, pmd_t pmdval)
{
PVOP_VCALL3(pv_mmu_ops.set_pmd, pmdp,
@@ -941,6 +949,11 @@ static inline void set_pte_at(struct mm_
PVOP_VCALL4(pv_mmu_ops.set_pte_at, mm, addr, ptep, pteval.pte_low);
}
+static inline pte_t get_pte(struct pte_t *ptep)
+{
+ return PVOP_CALL1(unsigned long, pv_mmu_ops.get_pte, ptep);
+}
+
static inline void set_pmd(pmd_t *pmdp, pmd_t pmdval)
{
PVOP_VCALL2(pv_mmu_ops.set_pmd, pmdp, pmdval.pud.pgd.pgd);
Index: linux-2.6/include/asm-x86/pgtable-2level.h
===================================================================
--- linux-2.6.orig/include/asm-x86/pgtable-2level.h
+++ linux-2.6/include/asm-x86/pgtable-2level.h
@@ -20,6 +20,10 @@ static inline void native_set_pte_at(str
{
native_set_pte(ptep, pte);
}
+static inline pte_t native_get_pte(pte_t *ptep)
+{
+ return *ptep;
+}
static inline void native_set_pmd(pmd_t *pmdp, pmd_t pmd)
{
*pmdp = pmd;
@@ -33,6 +37,8 @@ static inline void native_set_pmd(pmd_t
#define set_pte_atomic(pteptr, pteval) set_pte(pteptr,pteval)
#define set_pte_present(mm,addr,ptep,pteval) set_pte_at(mm,addr,ptep,pteval)
+#define get_pte(ptep) native_get_pte(ptep)
+
#define pte_clear(mm,addr,xp) do { set_pte_at(mm, addr, xp, __pte(0)); } while (0)
#define pmd_clear(xp) do { set_pmd(xp, __pmd(0)); } while (0)
Index: linux-2.6/include/asm-x86/pgtable-3level.h
===================================================================
--- linux-2.6.orig/include/asm-x86/pgtable-3level.h
+++ linux-2.6/include/asm-x86/pgtable-3level.h
@@ -61,6 +61,20 @@ static inline void native_set_pte_presen
ptep->pte_low = pte.pte_low;
}
+static inline pte_t native_get_pte(pte_t *ptep)
+{
+ pte_t pte;
+
+retry:
+ pte.pte_low = ptep->pte_low;
+ smp_rmb();
+ pte.pte_high = ptep->pte_high;
+ smp_rmb();
+ if (unlikely(pte.pte_low != ptep->pte_low))
+ goto retry;
+ return pte;
+}
+
static inline void native_set_pte_atomic(pte_t *ptep, pte_t pte)
{
set_64bit((unsigned long long *)(ptep),native_pte_val(pte));
@@ -99,6 +113,7 @@ static inline void native_pmd_clear(pmd_
#define set_pte_at(mm, addr, ptep, pte) native_set_pte_at(mm, addr, ptep, pte)
#define set_pte_present(mm, addr, ptep, pte) native_set_pte_present(mm, addr, ptep, pte)
#define set_pte_atomic(ptep, pte) native_set_pte_atomic(ptep, pte)
+#define get_pte(ptep) native_get_pte(ptep)
#define set_pmd(pmdp, pmd) native_set_pmd(pmdp, pmd)
#define set_pud(pudp, pud) native_set_pud(pudp, pud)
#define pte_clear(mm, addr, ptep) native_pte_clear(mm, addr, ptep)
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
