Re: [PATCH 3/9] x86/mm/tlb: Refactor common code into flush_tlb_on_cpus()

[Dave Hansen]

On 6/12/19 11:48 PM, Nadav Amit wrote:
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 91f6db92554c..c34bcf03f06f 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -734,7 +734,11 @@ static inline struct flush_tlb_info *get_flush_tlb_info(struct mm_struct *mm,
>  			unsigned int stride_shift, bool freed_tables,
>  			u64 new_tlb_gen)
>  {
> -	struct flush_tlb_info *info = this_cpu_ptr(&flush_tlb_info);
> +	struct flush_tlb_info *info;
> +
> +	preempt_disable();
> +
> +	info = this_cpu_ptr(&flush_tlb_info);
>  
>  #ifdef CONFIG_DEBUG_VM
>  	/*
> @@ -762,6 +766,23 @@ static inline void put_flush_tlb_info(void)
>  	barrier();
>  	this_cpu_dec(flush_tlb_info_idx);
>  #endif
> +	preempt_enable();
> +}

The addition of this disable/enable pair is unchangelogged and
uncommented.  I think it makes sense since we do need to make sure we
stay on this CPU, but it would be nice to mention.

> +static void flush_tlb_on_cpus(const cpumask_t *cpumask,
> +			      const struct flush_tlb_info *info)
> +{

Might be nice to mention that preempt must be disabled.  It's kinda
implied from the smp_processor_id(), but being explicit is always nice too.

> +	int this_cpu = smp_processor_id();
> +
> +	if (cpumask_test_cpu(this_cpu, cpumask)) {
> +		lockdep_assert_irqs_enabled();
> +		local_irq_disable();
> +		flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN);
> +		local_irq_enable();
> +	}
> +
> +	if (cpumask_any_but(cpumask, this_cpu) < nr_cpu_ids)
> +		flush_tlb_others(cpumask, info);
>  }
>  
>  void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
> @@ -770,9 +791,6 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
>  {
>  	struct flush_tlb_info *info;
>  	u64 new_tlb_gen;
> -	int cpu;
> -
> -	cpu = get_cpu();
>  
>  	/* Should we flush just the requested range? */
>  	if ((end == TLB_FLUSH_ALL) ||
> @@ -787,18 +805,18 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
>  	info = get_flush_tlb_info(mm, start, end, stride_shift, freed_tables,
>  				  new_tlb_gen);
>  
> -	if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
> -		lockdep_assert_irqs_enabled();
> -		local_irq_disable();
> -		flush_tlb_func_local(info, TLB_LOCAL_MM_SHOOTDOWN);
> -		local_irq_enable();
> -	}
> +	/*
> +	 * Assert that mm_cpumask() corresponds with the loaded mm. We got one
> +	 * exception: for init_mm we do not need to flush anything, and the
> +	 * cpumask does not correspond with loaded_mm.
> +	 */
> +	VM_WARN_ON_ONCE(cpumask_test_cpu(smp_processor_id(), mm_cpumask(mm)) !=
> +			(mm == this_cpu_read(cpu_tlbstate.loaded_mm)) &&
> +			mm != &init_mm);

Very very cool.  You thought "these should be equivalent", and you added
a corresponding warning to ensure they are.

> -	if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
> -		flush_tlb_others(mm_cpumask(mm), info);
> +	flush_tlb_on_cpus(mm_cpumask(mm), info);
>  
>  	put_flush_tlb_info();
> -	put_cpu();
>  }



Reviewed-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>