arch_tlbbatch_flush() and flush_tlb_mm_range() have very similar code,
which is effectively the same. Extract the mutual code into a new
function flush_tlb_on_cpus().
There is one functional change, which should not affect correctness:
flush_tlb_mm_range compared loaded_mm and the mm to figure out if local
flush is needed. Instead, the common code would look at the mm_cpumask()
which should give the same result. Performance should not be affected,
since this cpumask should not change in such a frequency that would
introduce cache contention.
Cc: Dave Hansen <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/mm/tlb.c | 62 ++++++++++++++++++++++++++---------------------
1 file changed, 34 insertions(+), 28 deletions(-)
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();
+}
+
+static void flush_tlb_on_cpus(const cpumask_t *cpumask,
+ const struct flush_tlb_info *info)
+{
+ 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);
- 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();
}
@@ -833,13 +851,11 @@ void flush_tlb_kernel_range(unsigned long start, unsigned long end)
} else {
struct flush_tlb_info *info;
- preempt_disable();
info = get_flush_tlb_info(NULL, start, end, 0, false, 0);
on_each_cpu(do_kernel_range_flush, info, 1);
put_flush_tlb_info();
- preempt_enable();
}
}
@@ -857,21 +873,11 @@ static const struct flush_tlb_info full_flush_tlb_info = {
void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
{
- int cpu = get_cpu();
-
- if (cpumask_test_cpu(cpu, &batch->cpumask)) {
- lockdep_assert_irqs_enabled();
- local_irq_disable();
- flush_tlb_func_local(&full_flush_tlb_info, TLB_LOCAL_SHOOTDOWN);
- local_irq_enable();
- }
-
- if (cpumask_any_but(&batch->cpumask, cpu) < nr_cpu_ids)
- flush_tlb_others(&batch->cpumask, &full_flush_tlb_info);
+ preempt_disable();
+ flush_tlb_on_cpus(&batch->cpumask, &full_flush_tlb_info);
+ preempt_enable();
cpumask_clear(&batch->cpumask);
-
- put_cpu();
}
static ssize_t tlbflush_read_file(struct file *file, char __user *user_buf,
--
2.20.1
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 <[email protected]>
> On Jun 25, 2019, at 2:07 PM, Dave Hansen <[email protected]> wrote:
>
> 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.
I’ll add some comments and update the changeling . I see I marked
get_flush_tlb_info() as “inline” for no reason. I’m going to remove it in
this patch, unless you say it should be in a separate patch.
>> +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.
I will add a comment, although smp_processor_id() should anyhow shout at you
if you use it with CONFIG_DEBUG_PREEMPT=y.
>> + 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.
The credit for this assertion goes to Peter who suggested I add it...
>
>> - 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 <[email protected]>
Thanks for the reviews of this patch and the others (don’t worry, I won’t
add the “Reviewed-by” tag to the others).