Re: [patch 2/2]: introduce fast_gup

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Linus Torvalds wrote:
> 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;
> 	}
>
>   

I think this is still broken.  Suppose that after reading pte_high 
native_set_pte() is called again on another cpu, changing pte_low back 
to the original value (but with a different pte_high).  You now have 
pte_low from second native_set_pte() but pte_high from the first 
native_set_pte().

You could use cmpxchg8b to atomically load the pte, but at the expense 
of taking the cacheline for exclusive access.

-- 
error compiling committee.c: too many arguments to function