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

[Brendan Jackman <jackmanb@google.com>]

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?

> 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.

> +static inline void invlpgb_flush_user_nr_nosync(unsigned long pcid,
> +                                               unsigned long addr,
> +                                               u16 nr, bool pmd_stride)
> +{
> +       __invlpgb_flush_user_nr_nosync(pcid, addr, nr, pmd_stride);
> +       if (!this_cpu_read(cpu_tlbstate.need_tlbsync))
> +               this_cpu_write(cpu_tlbstate.need_tlbsync, true);

 Why do we need the conditional here?

I always thought code like this was about avoiding cacheline
contention, but given this is a percpu and it's really only of
interest to the present CPU, is this worthwhile?

I guess it might be sharing a cacheline with some other percpu that is
more shared?

Anyway, not a big deal, I'm mostly asking for my own education.

> @@ -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()?

> @@ -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();
> +
>         cpumask_clear(&batch->cpumask);
>
>         put_flush_tlb_info();
>         put_cpu();
>  }
>
> +void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *batch,
> +                                            struct mm_struct *mm,
> +                                            unsigned long uaddr)
> +{
> +       u16 asid = mm_global_asid(mm);
> +
> +       if (asid) {
> +               invlpgb_flush_user_nr_nosync(kern_pcid(asid), uaddr, 1, false);
> +               /* Do any CPUs supporting INVLPGB need PTI? */

Not today, but

1. I don't think avoiding static_cpu_has() is worth the cost of making
the reader take that logical step.

2.  AFAIK a new CPU bug never led to enabling KPTI on a CPU that
didn't have it before, and I think that would be a pretty dystopian
future (and hopefully by then we'll have ASI instead...). But we can't
really rule it out.