Re: [PATCH RFC 2/3] x86/mm: implement redundant IPI elimination for

[Lance Yang <lance.yang@linux.dev>]

On 2025/12/23 17:44, David Hildenbrand (Red Hat) wrote:
> On 12/22/25 04:19, Lance Yang wrote:
>> From: Lance Yang <lance.yang@linux.dev>
>>
>>
>> On Thu, 18 Dec 2025 14:08:07 +0100, David Hildenbrand (Red Hat) wrote:
>>> On 12/13/25 09:00, Lance Yang wrote:
>>>> From: Lance Yang <lance.yang@linux.dev>
>>>>
>>>> Pass both freed_tables and unshared_tables to flush_tlb_mm_range() to
>>>> ensure lazy-TLB CPUs receive IPIs and flush their paging-structure 
>>>> caches:
>>>>
>>>>     flush_tlb_mm_range(..., freed_tables || unshared_tables);
>>>>
>>>> Implement tlb_table_flush_implies_ipi_broadcast() for x86: on native 
>>>> x86
>>>> without paravirt or INVLPGB, the TLB flush IPI already provides 
>>>> necessary
>>>> synchronization, allowing the second IPI to be skipped. For paravirt 
>>>> with
>>>> non-native flush_tlb_multi and for INVLPGB, conservatively keep both 
>>>> IPIs.
>>>>
>>>> Suggested-by: David Hildenbrand (Red Hat) <david@kernel.org>
>>>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>>>> ---
>>>>    arch/x86/include/asm/tlb.h | 17 ++++++++++++++++-
>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/tlb.h b/arch/x86/include/asm/tlb.h
>>>> index 866ea78ba156..96602b7b7210 100644
>>>> --- a/arch/x86/include/asm/tlb.h
>>>> +++ b/arch/x86/include/asm/tlb.h
>>>> @@ -5,10 +5,24 @@
>>>>    #define tlb_flush tlb_flush
>>>>    static inline void tlb_flush(struct mmu_gather *tlb);
>>>> +#define tlb_table_flush_implies_ipi_broadcast 
>>>> tlb_table_flush_implies_ipi_broadcast
>>>> +static inline bool tlb_table_flush_implies_ipi_broadcast(void);
>>>> +
>>>>    #include <asm-generic/tlb.h>
>>>>    #include <linux/kernel.h>
>>>>    #include <vdso/bits.h>
>>>>    #include <vdso/page.h>
>>>> +#include <asm/paravirt.h>
>>>> +
>>>> +static inline bool tlb_table_flush_implies_ipi_broadcast(void)
>>>> +{
>>>> +#ifdef CONFIG_PARAVIRT
>>>> +    /* Paravirt may use hypercalls that don't send real IPIs. */
>>>> +    if (pv_ops.mmu.flush_tlb_multi != native_flush_tlb_multi)
>>>> +        return false;
>>>> +#endif
>>>> +    return !cpu_feature_enabled(X86_FEATURE_INVLPGB);
>>>
>>> Right, here I was wondering whether we should have a new pv_ops callback
>>> to indicate that instead.
>>>
>>> pv_ops.mmu.tlb_table_flush_implies_ipi_broadcast()
>>>
>>> Or a simple boolean property that pv init code properly sets.
>>
>> Cool!
>>
>>>
>>> Something for x86 folks to give suggestions for. :)
>>
>> I prefer to use a boolean property instead of comparing function 
>> pointers.
>> Something like this:
>>
>> ----8<----
>> diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c
>> index cfcb60468b01..90e9da33f2c7 100644
>> --- a/arch/x86/hyperv/mmu.c
>> +++ b/arch/x86/hyperv/mmu.c
>> @@ -243,4 +243,5 @@ void hyperv_setup_mmu_ops(void)
>>
>>       pr_info("Using hypercall for remote TLB flush\n");
>>       pv_ops.mmu.flush_tlb_multi = hyperv_flush_tlb_multi;
>> +    pv_ops.mmu.tlb_flush_implies_ipi_broadcast = false;
>>   }
>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/ 
>> asm/paravirt_types.h
>> index 3502939415ad..f9756df6f3f6 100644
>> --- a/arch/x86/include/asm/paravirt_types.h
>> +++ b/arch/x86/include/asm/paravirt_types.h
>> @@ -133,6 +133,19 @@ struct pv_mmu_ops {
>>       void (*flush_tlb_multi)(const struct cpumask *cpus,
>>                   const struct flush_tlb_info *info);
>>
>> +    /*
>> +     * Indicates whether TLB flush IPIs provide sufficient 
>> synchronization
>> +     * for GUP-fast when freeing or unsharing page tables.
>> +     *
>> +     * Set to true only when the TLB flush guarantees:
>> +     * - IPIs reach all CPUs with potentially stale paging-structure 
>> caches
>> +     * - Synchronization with IRQ-disabled code like GUP-fast
>> +     *
>> +     * Paravirt implementations that use hypercalls (which may not send
>> +     * real IPIs) should set this to false.
>> +     */
>> +    bool tlb_flush_implies_ipi_broadcast;
>> +
>>       /* Hook for intercepting the destruction of an mm_struct. */
>>       void (*exit_mmap)(struct mm_struct *mm);
>>       void (*notify_page_enc_status_changed)(unsigned long pfn, int 
>> npages, bool enc);
>> diff --git a/arch/x86/include/asm/tlb.h b/arch/x86/include/asm/tlb.h
>> index 96602b7b7210..9d20ad4786cc 100644
>> --- a/arch/x86/include/asm/tlb.h
>> +++ b/arch/x86/include/asm/tlb.h
>> @@ -18,7 +18,7 @@ static inline bool 
>> tlb_table_flush_implies_ipi_broadcast(void)
>>   {
>>   #ifdef CONFIG_PARAVIRT
>>       /* Paravirt may use hypercalls that don't send real IPIs. */
>> -    if (pv_ops.mmu.flush_tlb_multi != native_flush_tlb_multi)
>> +    if (!pv_ops.mmu.tlb_flush_implies_ipi_broadcast)
>>           return false;
>>   #endif
>>       return !cpu_feature_enabled(X86_FEATURE_INVLPGB);
> 
> I'd have thought that the X86_FEATURE_INVLPGB heck should then also be 
> taken care of by whoever sets tlb_flush_implies_ipi_broadcast.

Makes sense!

Let's have the INVLPGB check happen at setup time, not at use time :P

Cheers,
Lance