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

From: Ingo Molnar <mingo@kernel.org>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Peter Zijlstra <peterz@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Rik van Riel <riel@redhat.com>, Brian Gerst <brgerst@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Denys Vlasenko <dvlasenk@redhat.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>,
	Andrew Lutomirski <luto@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	"linux-tip-commits@vger.kernel.org" 
	<linux-tip-commits@vger.kernel.org>
Subject: Re: [tip:x86/urgent] x86/mm: Add barriers and document switch_mm() -vs-flush synchronization
Date: Tue, 12 Jan 2016 11:21:05 +0100	[thread overview]
Message-ID: <20160112102105.GA4878@gmail.com> (raw)
In-Reply-To: <CALCETrXLUewtYNQOdiGpKLvp=RL0eMLSj+v7_J0G1a_do+6G8Q@mail.gmail.com>

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Mon, Jan 11, 2016 at 10:25 AM, Peter Zijlstra <peterz@infradead.org> 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