Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Andi Kleen]

Linux Kernel Mailing List <linux-kernel@vger.kernel.org> writes:
> +/*
> + * For PTEs and PDEs, we must clear the P-bit first when clearing a page table
> + * entry, so clear the bottom half first and enforce ordering with a compiler
> + * barrier.
> + */
> +static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
> +{
> +	ptep->pte_low = 0;
> +	smp_wmb();
> +	ptep->pte_high = 0;
> +}
> +
> +static inline void pmd_clear(pmd_t *pmd)
> +{
> +	u32 *tmp = (u32 *)pmd;
> +	*tmp = 0;
> +	smp_wmb();
> +	*(tmp + 1) = 0;
> +}

I think that's still wrong - it should be wmb() not smp_wmb because this
problem can happen on a UP kernel already.

-Andi

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> > +static inline void pmd_clear(pmd_t *pmd)
> > +{
> > +	u32 *tmp = (u32 *)pmd;
> > +	*tmp = 0;
> > +	smp_wmb();
> > +	*(tmp + 1) = 0;
> > +}
> 
> I think that's still wrong - it should be wmb() not smp_wmb because this
> problem can happen on a UP kernel already.

I thought the barrier is to keep compiler from reordering not processor.

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Andi Kleen]

On Friday 28 April 2006 07:23, Chris Wright wrote:
> * Andi Kleen (ak@suse.de) wrote:
> > > +static inline void pmd_clear(pmd_t *pmd)
> > > +{
> > > +	u32 *tmp = (u32 *)pmd;
> > > +	*tmp = 0;
> > > +	smp_wmb();
> > > +	*(tmp + 1) = 0;
> > > +}
> > 
> > I think that's still wrong - it should be wmb() not smp_wmb because this
> > problem can happen on a UP kernel already.
> 
> I thought the barrier is to keep compiler from reordering not processor.

Yes, but with smp_wmb() it will go away on UP. And even on UP the
CPU is free to speculate.

-Andi

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> On Friday 28 April 2006 07:23, Chris Wright wrote:
> > * Andi Kleen (ak@suse.de) wrote:
> > > > +static inline void pmd_clear(pmd_t *pmd)
> > > > +{
> > > > +	u32 *tmp = (u32 *)pmd;
> > > > +	*tmp = 0;
> > > > +	smp_wmb();
> > > > +	*(tmp + 1) = 0;
> > > > +}
> > > 
> > > I think that's still wrong - it should be wmb() not smp_wmb because this
> > > problem can happen on a UP kernel already.
> > 
> > I thought the barrier is to keep compiler from reordering not processor.
> 
> Yes, but with smp_wmb() it will go away on UP. And even on UP the
> CPU is free to speculate.

I must be confused.  Doesn't that become a barrier() on UP?

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Andi Kleen]

On Friday 28 April 2006 08:27, Chris Wright wrote:
> * Andi Kleen (ak@suse.de) wrote:
> > On Friday 28 April 2006 07:23, Chris Wright wrote:
> > > * Andi Kleen (ak@suse.de) wrote:
> > > > > +static inline void pmd_clear(pmd_t *pmd)
> > > > > +{
> > > > > +	u32 *tmp = (u32 *)pmd;
> > > > > +	*tmp = 0;
> > > > > +	smp_wmb();
> > > > > +	*(tmp + 1) = 0;
> > > > > +}
> > > > 
> > > > I think that's still wrong - it should be wmb() not smp_wmb because this
> > > > problem can happen on a UP kernel already.
> > > 
> > > I thought the barrier is to keep compiler from reordering not processor.
> > 
> > Yes, but with smp_wmb() it will go away on UP. And even on UP the
> > CPU is free to speculate.
> 
> I must be confused.  Doesn't that become a barrier() on UP?

No it was me who was confused sorry. Somehow i thought it was defined
away for !SMP

(which would make sense because why would you want a compile barrier
for a barrier that is only needed on SMP?) 

-Andi

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Nick Piggin]

Andi Kleen wrote:

> No it was me who was confused sorry. Somehow i thought it was defined
> away for !SMP
> 
> (which would make sense because why would you want a compile barrier
> for a barrier that is only needed on SMP?) 

It is maybe not clearly named. smp_wmb() is a memory barrier to the
regular (eg. RAM) cache coherency domain AFAICT. wmb() is also a
barrier to io memory.

There is nothing to distinguish SMP and UP. I guess sometimes smp_
barriers would not even have to be a barrier() on UP, but other
times they would have to be (eg. in the case of concurrent
interrupts, context switches).

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] x86/PAE: Fix pte_clear for the >4GB RAM case

[Linus Torvalds]

On Fri, 28 Apr 2006, Andi Kleen wrote:
> > 
> > I must be confused.  Doesn't that become a barrier() on UP?
> 
> No it was me who was confused sorry. Somehow i thought it was defined
> away for !SMP
> 
> (which would make sense because why would you want a compile barrier
> for a barrier that is only needed on SMP?) 

If the write barrier is needed on SMP, then UP needs a compiler barrier. 
Even UP has interrupts (and preemption) that can expose ordering of the 
interrupted code.

		Linus