From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua0-x244.google.com (mail-ua0-x244.google.com [IPv6:2607:f8b0:400c:c08::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 40gqBd5s7gzF2MT for ; Wed, 9 May 2018 18:23:53 +1000 (AEST) Received: by mail-ua0-x244.google.com with SMTP id v17so13146705uak.6 for ; Wed, 09 May 2018 01:23:53 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180509065613.14762-1-npiggin@gmail.com> References: <20180509065613.14762-1-npiggin@gmail.com> From: Balbir Singh Date: Wed, 9 May 2018 18:23:48 +1000 Message-ID: Subject: Re: [PATCH] powerpc/64s/radix: reset mm_cpumask for single thread process when possible To: Nicholas Piggin Cc: "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" Content-Type: text/plain; charset="UTF-8" List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, May 9, 2018 at 4:56 PM, Nicholas Piggin wrote: > When a single-threaded process has a non-local mm_cpumask and requires > a full PID tlbie invalidation, use that as an opportunity to reset the > cpumask back to the current CPU we're running on. > > No other thread can concurrently switch to this mm, because it must > have had a reference on mm_users before it could use_mm. mm_users can > be asynchronously incremented e.g., by mmget_not_zero, but those users > must not be doing use_mm. > > Signed-off-by: Nicholas Piggin > --- > arch/powerpc/include/asm/mmu_context.h | 19 +++++++++ > arch/powerpc/include/asm/tlb.h | 8 ++++ > arch/powerpc/mm/tlb-radix.c | 57 +++++++++++++++++++------- > 3 files changed, 70 insertions(+), 14 deletions(-) > > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h > index 1835ca1505d6..df12a994529f 100644 > --- a/arch/powerpc/include/asm/mmu_context.h > +++ b/arch/powerpc/include/asm/mmu_context.h > @@ -6,6 +6,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -201,6 +202,24 @@ static inline void activate_mm(struct mm_struct *prev, struct mm_struct *next) > static inline void enter_lazy_tlb(struct mm_struct *mm, > struct task_struct *tsk) > { > +#ifdef CONFIG_PPC_BOOK3S_64 > + /* > + * Under radix, we do not want to keep lazy PIDs around because > + * even if the CPU does not access userspace, it can still bring > + * in translations through speculation and prefetching. > + * > + * Switching away here allows us to trim back the mm_cpumask in > + * cases where we know the process is not running on some CPUs > + * (see mm/tlb-radix.c). > + */ > + if (radix_enabled() && mm != &init_mm) { > + mmgrab(&init_mm); This is called when a kernel thread decides to unuse a mm, I agree switching to init_mm as active_mm is reasonable thing to do. > + tsk->active_mm = &init_mm; Are we called with irqs disabled? Don't we need it below? > + switch_mm_irqs_off(mm, tsk->active_mm, tsk); > + mmdrop(mm); > + } > +#endif > + > /* 64-bit Book3E keeps track of current PGD in the PACA */ > #ifdef CONFIG_PPC_BOOK3E_64 > get_paca()->pgd = NULL; > diff --git a/arch/powerpc/include/asm/tlb.h b/arch/powerpc/include/asm/tlb.h > index a7eabff27a0f..006fce98c403 100644 > --- a/arch/powerpc/include/asm/tlb.h > +++ b/arch/powerpc/include/asm/tlb.h > @@ -76,6 +76,14 @@ static inline int mm_is_thread_local(struct mm_struct *mm) > return false; > return cpumask_test_cpu(smp_processor_id(), mm_cpumask(mm)); > } > +static inline void mm_reset_thread_local(struct mm_struct *mm) reset_thread_local --> reset_to_thread_local? > +{ > + WARN_ON(atomic_read(&mm->context.copros) > 0); Can we put this under DEBUG_VM, VM_WARN_ON? > + WARN_ON(!(atomic_read(&mm->mm_users) == 1 && current->mm == mm)); > + atomic_set(&mm->context.active_cpus, 1); > + cpumask_clear(mm_cpumask(mm)); > + cpumask_set_cpu(smp_processor_id(), mm_cpumask(mm)); > +} > #else /* CONFIG_PPC_BOOK3S_64 */ > static inline int mm_is_thread_local(struct mm_struct *mm) > { > diff --git a/arch/powerpc/mm/tlb-radix.c b/arch/powerpc/mm/tlb-radix.c > index 5ac3206c51cc..d5593a78702a 100644 > --- a/arch/powerpc/mm/tlb-radix.c > +++ b/arch/powerpc/mm/tlb-radix.c > @@ -504,6 +504,15 @@ void radix__local_flush_tlb_page(struct vm_area_struct *vma, unsigned long vmadd > } > EXPORT_SYMBOL(radix__local_flush_tlb_page); > > +static bool mm_is_singlethreaded(struct mm_struct *mm) > +{ mm_tlb_context_is_local? We should also skip init_mm from these checks > + if (atomic_read(&mm->context.copros) > 0) > + return false; > + if (atomic_read(&mm->mm_users) == 1 && current->mm == mm) > + return true; > + return false; > +} > + > static bool mm_needs_flush_escalation(struct mm_struct *mm) > { > /* > @@ -511,7 +520,9 @@ static bool mm_needs_flush_escalation(struct mm_struct *mm) > * caching PTEs and not flushing them properly when > * RIC = 0 for a PID/LPID invalidate > */ > - return atomic_read(&mm->context.copros) != 0; > + if (atomic_read(&mm->context.copros) > 0) > + return true; > + return false; > } > > #ifdef CONFIG_SMP > @@ -525,12 +536,17 @@ void radix__flush_tlb_mm(struct mm_struct *mm) > > preempt_disable(); > if (!mm_is_thread_local(mm)) { > - if (mm_needs_flush_escalation(mm)) > + if (mm_is_singlethreaded(mm)) { > _tlbie_pid(pid, RIC_FLUSH_ALL); > - else > + mm_reset_thread_local(mm); > + } else if (mm_needs_flush_escalation(mm)) { > + _tlbie_pid(pid, RIC_FLUSH_ALL); > + } else { > _tlbie_pid(pid, RIC_FLUSH_TLB); > - } else > + } > + } else { > _tlbiel_pid(pid, RIC_FLUSH_TLB); > + } > preempt_enable(); > } > EXPORT_SYMBOL(radix__flush_tlb_mm); > @@ -544,10 +560,13 @@ void radix__flush_all_mm(struct mm_struct *mm) > return; > > preempt_disable(); > - if (!mm_is_thread_local(mm)) > + if (!mm_is_thread_local(mm)) { > _tlbie_pid(pid, RIC_FLUSH_ALL); > - else > + if (mm_is_singlethreaded(mm)) > + mm_reset_thread_local(mm); > + } else { > _tlbiel_pid(pid, RIC_FLUSH_ALL); > + } > preempt_enable(); > } > EXPORT_SYMBOL(radix__flush_all_mm); > @@ -644,10 +663,14 @@ void radix__flush_tlb_range(struct vm_area_struct *vma, unsigned long start, > if (local) { > _tlbiel_pid(pid, RIC_FLUSH_TLB); > } else { > - if (mm_needs_flush_escalation(mm)) > + if (mm_is_singlethreaded(mm)) { > + _tlbie_pid(pid, RIC_FLUSH_ALL); > + mm_reset_thread_local(mm); > + } else if (mm_needs_flush_escalation(mm)) { > _tlbie_pid(pid, RIC_FLUSH_ALL); > - else > + } else { > _tlbie_pid(pid, RIC_FLUSH_TLB); > + } > } > } else { > bool hflush = false; > @@ -802,13 +825,19 @@ static inline void __radix__flush_tlb_range_psize(struct mm_struct *mm, > } > > if (full) { > - if (!local && mm_needs_flush_escalation(mm)) > - also_pwc = true; > - > - if (local) > + if (local) { > _tlbiel_pid(pid, also_pwc ? RIC_FLUSH_ALL : RIC_FLUSH_TLB); > - else > - _tlbie_pid(pid, also_pwc ? RIC_FLUSH_ALL: RIC_FLUSH_TLB); > + } else { > + if (mm_is_singlethreaded(mm)) { > + _tlbie_pid(pid, RIC_FLUSH_ALL); > + mm_reset_thread_local(mm); > + } else { > + if (mm_needs_flush_escalation(mm)) > + also_pwc = true; > + > + _tlbie_pid(pid, also_pwc ? RIC_FLUSH_ALL : RIC_FLUSH_TLB); > + } > + } > } else { > if (local) > _tlbiel_va_range(start, end, pid, page_size, psize, also_pwc); Looks good otherwise Reviewed-by: Balbir Singh Balbir Singh.