On Sat, 30 Dec 2017, Thomas Gleixner wrote:
> On Sat, 30 Dec 2017, Linus Torvalds wrote:
> > On Sat, Dec 30, 2017 at 10:20 AM, Thomas Gleixner <t...@linutronix.de>
> > wrote:
> > The __native_flush_tlb() function looks _very_ broken.
> >
> > It does:
> >
> > invalidate_user_asid(this_cpu_read(cpu_tlbstate.loaded_mm_asid));
> > /*
> > * If current->mm == NULL then we borrow a mm which may change
> > * during a task switch and therefore we must not be preempted
> > * while we write CR3 back:
> > */
> > preempt_disable();
> > native_write_cr3(__native_read_cr3());
> > preempt_enable();
> >
> > but why is that preempt-disabled region only around the cr3 write? The
> > invalidate_user_asid() logic seems to be very CPU-sensitive too.
> >
> > And even if there is some reason why invalidate_user_asid() really can
> > do multiple different percpu accesses and it doesn't matter whether
> > the thread is bouncing around on different cpu's while it does it,
> > there doesn't seem any _reason_ not to just extend the preempt-disable
> > over the whole series.
> >
> > It really looks strange how it does multiple reads (and then a final
> > write!) to percpu state, when the cpu can change in between.
> >
> > So I'd suggest moving the preempt_disable() up to the top of that
> > function,
>
> That preempt_disable()/enable() was added with:
>
> commit 5cf0791da5c162ebc14b01eb01631cfa7ed4fa6e
> Author: Sebastian Andrzej Siewior <bige...@linutronix.de>
> Date: Fri Aug 5 15:37:39 2016 +0200
>
> x86/mm: Disable preemption during CR3 read+write
>
> So we need to look at that scenario before removing it.
The UP distinction does not exist anymore since:
commit ce4a4e565f5264909a18c733b864c3f74467f69e
Author: Andy Lutomirski <l...@kernel.org>
Date: Sun May 28 10:00:14 2017 -0700
x86/mm: Remove the UP asm/tlbflush.h code, always use the (formerly) SMP
code
The proper fix for this would have been anyway to add the
preempt_disable()/enable() pair to
static inline void flush_tlb_mm_range(struct mm_struct *mm,
unsigned long start, unsigned long end, unsigned long vmflag)
{
if (mm == current->active_mm)
__flush_tlb_up();
}
and not into __native_flush_tlb().
The common flush_tlb_mm_range() properly disables preemption, so we are
good to remove that.
Thanks,
tglx
