Re: [tip:x86/urgent] x86/mm: Add barriers and document switch_mm() -vs-flush synchronization

[Ingo Molnar]

* Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:

> On Mon, Jan 11, 2016 at 10:25 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > On Mon, Jan 11, 2016 at 03:42:40AM -0800, tip-bot for Andy Lutomirski wrote:
> >> --- a/arch/x86/include/asm/mmu_context.h
> >> +++ b/arch/x86/include/asm/mmu_context.h
> >> @@ -116,8 +116,34 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
> >>  #endif
> >>               cpumask_set_cpu(cpu, mm_cpumask(next));
> >>
> >> -             /* Re-load page tables */
> >> +             /*
> >> +              * Re-load page tables.
> >> +              *
> >> +              * This logic has an ordering constraint:
> >> +              *
> >> +              *  CPU 0: Write to a PTE for 'next'
> >> +              *  CPU 0: load bit 1 in mm_cpumask.  if nonzero, send IPI.
> >> +              *  CPU 1: set bit 1 in next's mm_cpumask
> >> +              *  CPU 1: load from the PTE that CPU 0 writes (implicit)
> >> +              *
> >> +              * We need to prevent an outcome in which CPU 1 observes
> >> +              * the new PTE value and CPU 0 observes bit 1 clear in
> >> +              * mm_cpumask.  (If that occurs, then the IPI will never
> >> +              * be sent, and CPU 0's TLB will contain a stale entry.)
> >> +              *
> >> +              * The bad outcome can occur if either CPU's load is
> >> +              * reordered before that CPU's store, so both CPUs much
> >
> > s/much/must/ ?
> 
> Indeed.  Is this worth a follow-up patch?

Absolutely! Any typos in code noticed by humans are worth fixing, especially when 
it's comments around tricky code. Could be done together with improving this part 
of the comments:

> > +
> >                       /*
> >                        * We were in lazy tlb mode and leave_mm disabled
> >                        * tlb flush IPI delivery. We must reload CR3
> >                        * to make sure to use no freed page tables.
> > +                      *
> > +                      * As above, this is a barrier that forces
> > +                      * TLB repopulation to be ordered after the
> > +                      * store to mm_cpumask.
>
> somewhat confused by this comment, cpumask_set_cpu() is a LOCK BTS, that is 
> already fully ordered.

... as pretty much any barriers related comment that confuses Peter needs to be 
improved, by definition. ;-)

Thanks,

	Ingo