Re: [PATCH] x86/hyperv: Account for lazy mm_cpumask maintenance

[Dave Hansen <dave.hansen@intel.com>]

On 10/28/26 23:42, Naman Jain wrote:
> CPUs no longer running an mm may remain in mm_cpumask(). Hyper-V treats
> these CPUs as flush targets and bypasses the native callbacks that trim
> the mask. A single-threaded process can therefore keep issuing
> all-processor flushes after visiting all CPUs and returning to one CPU.
> 
> Share the native target predicate and filter the mask before building
> either hypercall format or selecting HV_FLUSH_ALL_PROCESSORS. Preserve
> lazy-TLB exclusion and unfiltered page-table-freeing flushes, and use
> native callbacks for periodic trimming and ASID transitions.
> 
> When no remote targets remain, request immediate trimming through native
> callbacks so stale memberships do not unnecessarily prevent later
> invalidations from using the local fast path. Leave lazy CPU-mask
> maintenance during context switches unchanged.

There's a lot of stuff in this patch not covered in the changelog. Like
the mm_in_asid_transition() addition.

This reads to me as being highly LLM-generated and not super readable or
relevant.

> Fixes: 209954cbc7d0 ("x86/mm/tlb: Update mm_cpumask lazily")

We usually don't put Fixes: tags on performance improvements.

I don't want this backported.

> -	if (!hv_hypercall_pg)
> +	/*
> +	 * Hypercalls bypass the callbacks that trim mm_cpumask() and complete
> +	 * ASID transitions.
> +	 */
> +	if (!hv_hypercall_pg || info->trim_cpumask ||
> +	    mm_in_asid_transition(info->mm))
>  		goto do_native;

What do ASID transitions have to do with this?

>  	local_irq_save(flags);
> @@ -79,6 +82,30 @@ static void hyperv_flush_tlb_multi(const struct cpumask *cpus,
>  		goto do_native;
>  	}
>  
> +	flush_cpus = this_cpu_ptr(&hyperv_tlb_flush_mask);
> +	if (info->freed_tables) {
> +		cpumask_copy(flush_cpus, cpus);
> +	} else {
> +		cpumask_clear(flush_cpus);
> +		for_each_cpu(cpu, cpus) {
> +			if (cpu_needs_tlb_flush(cpu, info))
> +				__cpumask_set_cpu(cpu, flush_cpus);
> +		}
> +	}

This needs some heavy commenting. Or a helper. Or both.

> +	/*
> +	 * An effectively local mm should not keep taking the remote path until
> +	 * the next periodic trim. Let the native callbacks prune stale bits.
> +	 */
> +	if (cpumask_any_but(flush_cpus, smp_processor_id()) >= nr_cpu_ids) {

Is this condition right? Doesn't ">= nr_cpu_ids" mean "no other CPUs found"?

I'm not following this.

> +		struct flush_tlb_info local_info = *info;

What's wrong with reusing 'info'?

> +		local_info.trim_cpumask = 1;
> +		local_irq_restore(flags);
> +		native_flush_tlb_multi(cpus, &local_info);
> +		return;
> +	}

Gah, so is this hacking "trim_cpumask = 1" to force should_flush_tlb()
or something?

Right now, we have quite clean code around info->trim_cpumask. It's
either 0 or set from:

	info.trim_cpumask = should_trim_cpumask(mm);

It would be nice to keep it that way if possible.

Also, just looking at flush_tlb_mm_range(), there's a very familiar
check there:

        } else if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids) {
                remote_flush = true;

which feeds:

        if (remote_flush) {
                info.trim_cpumask = should_trim_cpumask(mm);
                flush_tlb_multi(mm_cpumask(mm), &info);
                consider_global_asid(mm);
        }

So what if we just added a check to should_trim_cpumask(), like:

	if (cpumask_weight(mm_cpumask(mm)) > atomic_read(&mm->mm_users))
		return true;

Or mm_users*2, or _some_ other heuristic that will trip when
mm_cpumask(mm) gets out of whack from ->mm_users.

That's a pretty cheap check because mm_cpumask() is already hot or is
just about to be read anyway. It might actually be useful to INVLPGB
systems too. That still needs the info->trim_cpumask check at the top of
hyperv_flush_tlb_multi(), btw.

Also, this is *kinda* changing the meaning of ->trim_cpumask. It's
really starting to mean "send IPIs". A rename might be appropriate.

/*
> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
> index 70098d448e999..e1e05cc867d5a 100644
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -264,6 +264,12 @@ void flush_tlb_one_kernel(unsigned long addr);
>  void flush_tlb_multi(const struct cpumask *cpumask,
>  		      const struct flush_tlb_info *info);
>  
> +/*
> + * Do not use for filtering when freeing page tables or during global ASID
> + * transitions.
> + */
> +bool cpu_needs_tlb_flush(int cpu, const struct flush_tlb_info *info);

Please look around this header that's being patched. Are the functions
above this commented? Please match the style.

Comment the definition itself, not the declaration.

>  static inline bool is_dyn_asid(u16 asid)
>  {
>  	return asid < TLB_NR_DYN_ASIDS;
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 0c4da320831cb..f2f40928d56a4 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -1273,10 +1273,9 @@ static void flush_tlb_func(void *info)
>  			nr_invalidate);
>  }
>  
> -static bool should_flush_tlb(int cpu, void *data)
> +bool cpu_needs_tlb_flush(int cpu, const struct flush_tlb_info *info)
>  {
>  	struct mm_struct *loaded_mm = per_cpu(cpu_tlbstate.loaded_mm, cpu);
> -	struct flush_tlb_info *info = data;
>  
>  	/*
>  	 * Order the 'loaded_mm' and 'is_lazy' against their
> @@ -1311,6 +1310,11 @@ static bool should_flush_tlb(int cpu, void *data)
>  	return false;
>  }
>  
> +static bool should_flush_tlb(int cpu, void *data)
> +{
> +	return cpu_needs_tlb_flush(cpu, data);
> +}
> +
>  static bool should_trim_cpumask(struct mm_struct *mm)
>  {
>  	if (time_after(jiffies, READ_ONCE(mm->context.next_trim_cpumask))) {

None of this movement is mentioned in the changelog. The naming change
here is also really silly. Now we have

	should_flush_tlb(int cpu, void *data)
and
	cpu_needs_tlb_flush(int cpu, const struct flush_tlb_info *info)

which are literally the same thing, except for the type. This is
precisely the kind of place we should keep the names _identical_. Either
use an __ or call the "void *" version "_wrapper" or something with a
comment:

/* Wrap foo() with a different type for use as an IPI callback: */

BTW, this "patch" probably needs to be broken up into a 4-5ish patch series.