Hi Dave, On Thu, Apr 23, 2026 at 10:56:03AM -0700, Dave Hansen wrote: [...] >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:
Cool, thanks! I was trying to keep the change small by passing unshared_tables through the exsiting freed_tables argument, but that made the code a bit harder to follow ... > >> - 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? [...] Yeah, renaming the flush_tlb_info bit to wake_lazy_cpus reads much better. Will fold this into v10, Thanks for spelling it out! Lance
