Re: [PATCH v9 10/12] x86/mm: do targeted broadcast flushing from tlbbatch code

[Rik van Riel <riel@surriel.com>]

On Mon, 2025-02-10 at 16:27 +0100, Brendan Jackman wrote:
> On Thu, 6 Feb 2025 at 05:46, Rik van Riel <riel@surriel.com> wrote:
> >  /* Wait for INVLPGB originated by this CPU to complete. */
> > -static inline void tlbsync(void)
> > +static inline void __tlbsync(void)
> >  {
> > -       cant_migrate();
> 
> Why does this have to go away?

I'm not sure the current task in sched_init() has
all the correct bits set to prevent the warning
from firing, but on the flip side it won't have
called INVLPGB yet at that point, so the call to
enter_lazy_tlb() won't actually end up here.

I'll put it back.

> 
> > diff --git a/arch/x86/include/asm/tlbflush.h
> > b/arch/x86/include/asm/tlbflush.h
> > index 234277a5ef89..bf167e215e8e 100644
> > --- a/arch/x86/include/asm/tlbflush.h
> > +++ b/arch/x86/include/asm/tlbflush.h
> > @@ -106,6 +106,7 @@ struct tlb_state {
> >          * need to be invalidated.
> >          */
> >         bool invalidate_other;
> > +       bool need_tlbsync;
> 
> The ifdeffery is missing here.

Added.

> 
> > @@ -794,6 +825,8 @@ void switch_mm_irqs_off(struct mm_struct
> > *unused, struct mm_struct *next,
> >         if (IS_ENABLED(CONFIG_PROVE_LOCKING))
> >                 WARN_ON_ONCE(!irqs_disabled());
> > 
> > +       tlbsync();
> > +
> >         /*
> >          * Verify that CR3 is what we think it is.  This will catch
> >          * hypothetical buggy code that directly switches to
> > swapper_pg_dir
> > @@ -973,6 +1006,8 @@ void switch_mm_irqs_off(struct mm_struct
> > *unused, struct mm_struct *next,
> >   */
> >  void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
> >  {
> > +       tlbsync();
> > +
> 
> I have a feeling I'll look stupid for asking this, but why do we need
> this and the one in switch_mm_irqs_off()?

This is an architectural thing: TLBSYNC waits for
the INVLPGB flushes to finish that were issued
from the same CPU.

That means if we have pending flushes (from the
pageout code), we need to wait for them at context
switch time, before the task could potentially be
migrated to another CPU.

> 
> > @@ -1661,12 +1694,53 @@ void arch_tlbbatch_flush(struct
> > arch_tlbflush_unmap_batch *batch)
> >                 local_irq_enable();
> >         }
> > 
> > +       /*
> > +        * If we issued (asynchronous) INVLPGB flushes, wait for
> > them here.
> > +        * The cpumask above contains only CPUs that were running
> > tasks
> > +        * not using broadcast TLB flushing.
> > +        */
> > +       if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
> 
> It feels wrong that we check the cpufeature here but not in e.g.
> enter_lazy_tlb().
> 
> > +               tlbsync();
> > +
We no longer need to check it here, with the change
to tlbsync. Good catch.

-- 
All Rights Reversed.