Re: [PATCH v3] x86,mm: only trim the mm_cpumask once a second

Mathieu Desnoyers
2024-12-04T20:19:52Z

On 2024-12-04 11:56, Rik van Riel wrote:
[...]
>
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Reported-by: kernel test roboto <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202411282207.6bd28eae-lkp@intel.com/
> ---
> arch/x86/include/asm/mmu.h | 2 ++
> arch/x86/include/asm/mmu_context.h | 1 +
> arch/x86/include/asm/tlbflush.h | 1 +
> arch/x86/mm/tlb.c | 35 +++++++++++++++++++++++++++---
> 4 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
> index ce4677b8b735..3b496cdcb74b 100644
> --- a/arch/x86/include/asm/mmu.h
> +++ b/arch/x86/include/asm/mmu.h
> @@ -37,6 +37,8 @@ typedef struct {
> */
> atomic64_t tlb_gen;
>
> + unsigned long next_trim_cpumask;
> +
> #ifdef CONFIG_MODIFY_LDT_SYSCALL
> struct rw_semaphore ldt_usr_sem;
> struct ldt_struct *ldt;
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index 2886cb668d7f..795fdd53bd0a 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -151,6 +151,7 @@ static inline int init_new_context(struct task_struct *tsk,
>
> mm->context.ctx_id = atomic64_inc_return(&last_mm_ctx_id);
> atomic64_set(&mm->context.tlb_gen, 0);
> + mm->context.next_trim_cpumask = jiffies + HZ;
>
> #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> if (cpu_feature_enabled(X86_FEATURE_OSPKE)) {
> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
> index 69e79fff41b8..02fc2aa06e9e 100644
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -222,6 +222,7 @@ struct flush_tlb_info {
> unsigned int initiating_cpu;
> u8 stride_shift;
> u8 freed_tables;
> + u8 trim_cpumask;
> };
>
> void flush_tlb_local(void);
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 1aac4fa90d3d..a758143afa01 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -892,9 +892,36 @@ static void flush_tlb_func(void *info)
> nr_invalidate);
> }
>
> -static bool tlb_is_not_lazy(int cpu, void *data)
> +static bool should_flush_tlb(int cpu, void *data)
> {
> - return !per_cpu(cpu_tlbstate_shared.is_lazy, cpu);
> + struct flush_tlb_info *info = data;
> +
> + /* Lazy TLB will get flushed at the next context switch. */
> + if (per_cpu(cpu_tlbstate_shared.is_lazy, cpu))
> + return false;
> +
> + /* No mm means kernel memory flush. */
> + if (!info->mm)
> + return true;
> +
> + /* The target mm is loaded, and the CPU is not lazy. */
> + if (per_cpu(cpu_tlbstate.loaded_mm, cpu) == info->mm)
> + return true;
> +
> + /* In cpumask, but not the loaded mm? Periodically remove by flushing. */
> + if (info->trim_cpumask)
> + return true;
> +
> + return false;
> +}
> +
> +static bool should_trim_cpumask(struct mm_struct *mm)
> +{
> + if (time_after(jiffies, mm->context.next_trim_cpumask)) {
> + mm->context.next_trim_cpumask = jiffies + HZ;

AFAIU this should_trim_cpumask can be called from many cpus
concurrently for a given mm, so we'd want READ_ONCE/WRITE_ONCE
on the next_trim_cpumask.

Thanks,

Mathieu

> + return true;
> + }
> + return false;
> }
>
> DEFINE_PER_CPU_SHARED_ALIGNED(struct tlb_state_shared, cpu_tlbstate_shared);
> @@ -928,7 +955,7 @@ STATIC_NOPV void native_flush_tlb_multi(const struct cpumask *cpumask,
> if (info->freed_tables)
> on_each_cpu_mask(cpumask, flush_tlb_func, (void *)info, true);
> else
> - on_each_cpu_cond_mask(tlb_is_not_lazy, flush_tlb_func,
> + on_each_cpu_cond_mask(should_flush_tlb, flush_tlb_func,
> (void *)info, 1, cpumask);
> }
>
> @@ -979,6 +1006,7 @@ static struct flush_tlb_info *get_flush_tlb_info(struct mm_struct *mm,
> info->freed_tables = freed_tables;
> info->new_tlb_gen = new_tlb_gen;
> info->initiating_cpu = smp_processor_id();
> + info->trim_cpumask = 0;
>
> return info;
> }
> @@ -1021,6 +1049,7 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
> * flush_tlb_func_local() directly in this case.
> */
> if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids) {
> + info->trim_cpumask = should_trim_cpumask(mm);
> flush_tlb_multi(mm_cpumask(mm), info);
> } else if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
> lockdep_assert_irqs_enabled();

--
Mathieu Desnoyers
EfficiOS Inc.