Re: [PATCH 7.2 v9 2/2] x86/tlb: skip redundant sync IPIs for native TLB flush

[David Hildenbrand (Arm)]

On 4/23/26 19:56, 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:
> 
>> -       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?

Looks better!

-- 
Cheers,

David