On 4/19/26 20:08, Lance Yang wrote:
> -	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, tlb->freed_tables);
> +	/*
> +	 * Treat unshared_tables just like freed_tables, such that lazy-TLB
> +	 * CPUs also receive IPIs during unsharing of page tables, allowing
> +	 * us to safely implement tlb_table_flush_implies_ipi_broadcast().
> +	 */
> +	flush_tlb_mm_range(tlb->mm, start, end, stride_shift,
> +			   tlb->freed_tables || tlb->unshared_tables);
>  }

I've been staring at this trying to make sense of it for too long.

Right now, flush_tlb_mm_range() literally has an argument named
"freed_tables" and "tlb->freed_tables" is passed there. That seems
totally sane. It's 100% straightforward to follow.

But it makes zero logical sense to me to now mix "tlb->unshared_tables"
in there. Sure, what you _want_ is the freed_tables==1 behavior from
tlb->unshared_tables==1, and this obviously hacks that in there, but
it's not explained well enough and not maintainable like this. IOW, it's
still just hack.

I think what's happened here is that info->freed_tables is being
modified from being strictly related to page table freeing, and moved
over to a bit which tells TLB flushing implementations whether they can
respect CPUs in lazy TLB mode.

It's mentioned in the comment, but then ever reflected into the code.

Shouldn't we be doing something like the attached patch? Look at how
that maps over to the flushing side, like in the hyperv code:

> -       bool do_lazy = !info->freed_tables;
> +       bool do_lazy = !info->wake_lazy_cpus;
>  
>         trace_hyperv_mmu_flush_tlb_multi(cpus, info);
>  
> @@ -198,7 +198,7 @@ static u64 hyperv_flush_tlb_others_ex(co
>  
>         flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
>         nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
> -                       info->freed_tables ? NULL : cpu_is_lazy);
> +                       info->wake_lazy_cpus ? NULL : cpu_is_lazy);

That even makes the hyperv code easier to read over what was there
before, IMNHO.

Thoughts?