Re: [PATCH 1/3] x86/mm/pat: Disable preemption around __flush_tlb_all()

[Peter Zijlstra]

On Wed, Oct 17, 2018 at 09:54:38AM +0000, David Laight wrote:
> From: Sebastian Andrzej Siewior
> > Sent: 16 October 2018 21:25
> > I've seen the WARN_ON_ONCE(__read_cr3() != build_cr3()) in
> > switch_mm_irqs_off() every once in a while during a snapshotted system
> > upgrade.
> > I also saw the warning early during which was introduced in commit
> > decab0888e6e ("x86/mm: Remove preempt_disable/enable() from
> > __native_flush_tlb()"). The callchain is
> > 
> >   get_page_from_freelist() -> post_alloc_hook() -> __kernel_map_pages()
> > 
> > with CONFIG_DEBUG_PAGEALLOC enabled.
> > 
> > Turns out, once I disable preemption around __flush_tlb_all() both
> > warnings do not appear.
> > 
> > Disable preemption during CR3 reset / __flush_tlb_all().
> > 
> > Fixes: decab0888e6e ("x86/mm: Remove preempt_disable/enable() from __native_flush_tlb()")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/mm/pageattr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> > index 51a5a69ecac9f..fe6b21f0a6631 100644
> > --- a/arch/x86/mm/pageattr.c
> > +++ b/arch/x86/mm/pageattr.c
> > @@ -2088,7 +2088,9 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
> >  	 * We should perform an IPI and flush all tlbs,
> >  	 * but that can deadlock->flush only current cpu:
> >  	 */
> > +	preempt_disable();
> >  	__flush_tlb_all();
> > +	preempt_enable();
> 
> Can it make any sense to flush the tlb with preemption enabled?
> Surely preemption must be disabled over something else as well?

This code is fishy anyway, for only doing that local invalidate.

Ideally we'd never ever merge anything that only does local invalidates,
on a global address space, that's just broken.