| Message ID | 20210605014216.446867-2-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shoot lazy tlbs | expand |
On Sat, 5 Jun 2021 11:42:13 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > Add explicit _lazy_tlb annotated functions for lazy mm refcounting. > This makes lazy mm references more obvious, and allows explicit > refcounting to be removed if it is not used. > > ... > > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -1314,14 +1314,14 @@ void kthread_use_mm(struct mm_struct *mm) > WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD)); > WARN_ON_ONCE(tsk->mm); > > + mmgrab(mm); > + > task_lock(tsk); > /* Hold off tlb flush IPIs while switching mm's */ > local_irq_disable(); > active_mm = tsk->active_mm; > - if (active_mm != mm) { > - mmgrab(mm); > + if (active_mm != mm) > tsk->active_mm = mm; > - } Looks like a functional change. What's happening here?
Excerpts from Andrew Morton's message of June 8, 2021 9:49 am: > On Sat, 5 Jun 2021 11:42:13 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > >> Add explicit _lazy_tlb annotated functions for lazy mm refcounting. >> This makes lazy mm references more obvious, and allows explicit >> refcounting to be removed if it is not used. >> >> ... >> >> --- a/kernel/kthread.c >> +++ b/kernel/kthread.c >> @@ -1314,14 +1314,14 @@ void kthread_use_mm(struct mm_struct *mm) >> WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD)); >> WARN_ON_ONCE(tsk->mm); >> >> + mmgrab(mm); >> + >> task_lock(tsk); >> /* Hold off tlb flush IPIs while switching mm's */ >> local_irq_disable(); >> active_mm = tsk->active_mm; >> - if (active_mm != mm) { >> - mmgrab(mm); >> + if (active_mm != mm) >> tsk->active_mm = mm; >> - } > > Looks like a functional change. What's happening here? That's kthread_use_mm being clever about the lazy tlb mm. If it happened that the kthread had inherited a the lazy tlb mm that happens to be the one we want to use here, then we already have a refcount to it via the lazy tlb ref. So then it doesn't have to touch the refcount, but rather just converts it from the lazy tlb ref to the returned reference. If the lazy tlb mm doesn't get a reference, we can't do that. Thanks, Nick
On Tue, 08 Jun 2021 11:39:56 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > > Looks like a functional change. What's happening here? > > That's kthread_use_mm being clever about the lazy tlb mm. If it happened > that the kthread had inherited a the lazy tlb mm that happens to be the > one we want to use here, then we already have a refcount to it via the > lazy tlb ref. > > So then it doesn't have to touch the refcount, but rather just converts > it from the lazy tlb ref to the returned reference. If the lazy tlb mm > doesn't get a reference, we can't do that. Please cover this in the changelog and perhaps a code comment.
Excerpts from Andrew Morton's message of June 8, 2021 11:48 am: > On Tue, 08 Jun 2021 11:39:56 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > >> > Looks like a functional change. What's happening here? >> >> That's kthread_use_mm being clever about the lazy tlb mm. If it happened >> that the kthread had inherited a the lazy tlb mm that happens to be the >> one we want to use here, then we already have a refcount to it via the >> lazy tlb ref. >> >> So then it doesn't have to touch the refcount, but rather just converts >> it from the lazy tlb ref to the returned reference. If the lazy tlb mm >> doesn't get a reference, we can't do that. > > Please cover this in the changelog and perhaps a code comment. > Yeah fair enough, I'll even throw in a bug fix as well (your nose was right, and it was too clever for me by half...) Thanks, Nick -- Fix a refcounting bug in kthread_use_mm (the mm reference is increased unconditionally now, but the lazy tlb refcount is still only dropped only if mm != active_mm). And an update for the changelog: If a kernel thread's current lazy tlb mm happens to be the one it wants to use, then kthread_use_mm() cleverly transfers the mm refcount from the lazy tlb mm reference to the returned reference. If the lazy tlb mm reference is no longer identical to a normal reference, this trick does not work, so that is changed to be explicit about the two references. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- kernel/kthread.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/kernel/kthread.c b/kernel/kthread.c index b70e28431a01..5e9797b2d06e 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -1314,6 +1314,11 @@ void kthread_use_mm(struct mm_struct *mm) WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD)); WARN_ON_ONCE(tsk->mm); + /* + * It's possible that tsk->active_mm == mm here, but we must + * still mmgrab(mm) and mmdrop_lazy_tlb(active_mm), because lazy + * mm may not have its own refcount (see mmgrab/drop_lazy_tlb()). + */ mmgrab(mm); task_lock(tsk); @@ -1338,12 +1343,9 @@ void kthread_use_mm(struct mm_struct *mm) * memory barrier after storing to tsk->mm, before accessing * user-space memory. A full memory barrier for membarrier * {PRIVATE,GLOBAL}_EXPEDITED is implicitly provided by - * mmdrop(), or explicitly with smp_mb(). + * mmdrop_lazy_tlb(). */ - if (active_mm != mm) - mmdrop_lazy_tlb(active_mm); - else - smp_mb(); + mmdrop_lazy_tlb(active_mm); to_kthread(tsk)->oldfs = force_uaccess_begin(); }
diff --git a/arch/arm/mach-rpc/ecard.c b/arch/arm/mach-rpc/ecard.c index 827b50f1c73e..1b4a41aad793 100644 --- a/arch/arm/mach-rpc/ecard.c +++ b/arch/arm/mach-rpc/ecard.c @@ -253,7 +253,7 @@ static int ecard_init_mm(void) current->mm = mm; current->active_mm = mm; activate_mm(active_mm, mm); - mmdrop(active_mm); + mmdrop_lazy_tlb(active_mm); ecard_init_pgtables(mm); return 0; } diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index 2e05c783440a..fb0bdfc67366 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -1541,7 +1541,7 @@ void start_secondary(void *unused) { unsigned int cpu = raw_smp_processor_id(); - mmgrab(&init_mm); + mmgrab_lazy_tlb(&init_mm); current->active_mm = &init_mm; smp_store_cpu_info(cpu); diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c index 409e61210789..2962082787c0 100644 --- a/arch/powerpc/mm/book3s64/radix_tlb.c +++ b/arch/powerpc/mm/book3s64/radix_tlb.c @@ -663,10 +663,10 @@ void exit_lazy_flush_tlb(struct mm_struct *mm, bool always_flush) if (current->active_mm == mm) { WARN_ON_ONCE(current->mm != NULL); /* Is a kernel thread and is using mm as the lazy tlb */ - mmgrab(&init_mm); + mmgrab_lazy_tlb(&init_mm); current->active_mm = &init_mm; switch_mm_irqs_off(mm, &init_mm, current); - mmdrop(mm); + mmdrop_lazy_tlb(mm); } /* diff --git a/fs/exec.c b/fs/exec.c index 18594f11c31f..ca0f8b1af23a 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1029,9 +1029,9 @@ static int exec_mmap(struct mm_struct *mm) setmax_mm_hiwater_rss(&tsk->signal->maxrss, old_mm); mm_update_next_owner(old_mm); mmput(old_mm); - return 0; + } else { + mmdrop_lazy_tlb(active_mm); } - mmdrop(active_mm); return 0; } diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h index e24b1fe348e3..bfd1baca5266 100644 --- a/include/linux/sched/mm.h +++ b/include/linux/sched/mm.h @@ -49,6 +49,17 @@ static inline void mmdrop(struct mm_struct *mm) __mmdrop(mm); } +/* Helpers for lazy TLB mm refcounting */ +static inline void mmgrab_lazy_tlb(struct mm_struct *mm) +{ + mmgrab(mm); +} + +static inline void mmdrop_lazy_tlb(struct mm_struct *mm) +{ + mmdrop(mm); +} + /** * mmget() - Pin the address space associated with a &struct mm_struct. * @mm: The address space to pin. diff --git a/kernel/cpu.c b/kernel/cpu.c index e538518556f4..e87a89824e6c 100644 --- a/kernel/cpu.c +++ b/kernel/cpu.c @@ -602,7 +602,7 @@ static int finish_cpu(unsigned int cpu) */ if (mm != &init_mm) idle->active_mm = &init_mm; - mmdrop(mm); + mmdrop_lazy_tlb(mm); return 0; } diff --git a/kernel/exit.c b/kernel/exit.c index fd1c04193e18..8e87ec5f6be2 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -476,7 +476,7 @@ static void exit_mm(void) __set_current_state(TASK_RUNNING); mmap_read_lock(mm); } - mmgrab(mm); + mmgrab_lazy_tlb(mm); BUG_ON(mm != current->active_mm); /* more a memory barrier than a real lock */ task_lock(current); diff --git a/kernel/kthread.c b/kernel/kthread.c index fe3f2a40d61e..b70e28431a01 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -1314,14 +1314,14 @@ void kthread_use_mm(struct mm_struct *mm) WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD)); WARN_ON_ONCE(tsk->mm); + mmgrab(mm); + task_lock(tsk); /* Hold off tlb flush IPIs while switching mm's */ local_irq_disable(); active_mm = tsk->active_mm; - if (active_mm != mm) { - mmgrab(mm); + if (active_mm != mm) tsk->active_mm = mm; - } tsk->mm = mm; membarrier_update_current_mm(mm); switch_mm_irqs_off(active_mm, mm, tsk); @@ -1341,7 +1341,7 @@ void kthread_use_mm(struct mm_struct *mm) * mmdrop(), or explicitly with smp_mb(). */ if (active_mm != mm) - mmdrop(active_mm); + mmdrop_lazy_tlb(active_mm); else smp_mb(); @@ -1375,10 +1375,13 @@ void kthread_unuse_mm(struct mm_struct *mm) local_irq_disable(); tsk->mm = NULL; membarrier_update_current_mm(NULL); + mmgrab_lazy_tlb(mm); /* active_mm is still 'mm' */ enter_lazy_tlb(mm, tsk); local_irq_enable(); task_unlock(tsk); + + mmdrop(mm); } EXPORT_SYMBOL_GPL(kthread_unuse_mm); diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 5226cc26a095..e359c76ea2e2 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -4229,13 +4229,14 @@ static struct rq *finish_task_switch(struct task_struct *prev) * rq->curr, before returning to userspace, so provide them here: * * - a full memory barrier for {PRIVATE,GLOBAL}_EXPEDITED, implicitly - * provided by mmdrop(), + * provided by mmdrop_lazy_tlb(), * - a sync_core for SYNC_CORE. */ if (mm) { membarrier_mm_sync_core_before_usermode(mm); - mmdrop(mm); + mmdrop_lazy_tlb(mm); } + if (unlikely(prev_state == TASK_DEAD)) { if (prev->sched_class->task_dead) prev->sched_class->task_dead(prev); @@ -4299,9 +4300,9 @@ context_switch(struct rq *rq, struct task_struct *prev, /* * kernel -> kernel lazy + transfer active - * user -> kernel lazy + mmgrab() active + * user -> kernel lazy + mmgrab_lazy_tlb() active * - * kernel -> user switch + mmdrop() active + * kernel -> user switch + mmdrop_lazy_tlb() active * user -> user switch */ if (!next->mm) { // to kernel @@ -4309,7 +4310,7 @@ context_switch(struct rq *rq, struct task_struct *prev, next->active_mm = prev->active_mm; if (prev->mm) // from user - mmgrab(prev->active_mm); + mmgrab_lazy_tlb(prev->active_mm); else prev->active_mm = NULL; } else { // to user @@ -4325,7 +4326,7 @@ context_switch(struct rq *rq, struct task_struct *prev, switch_mm_irqs_off(prev->active_mm, next->mm, next); if (!prev->mm) { // from kernel - /* will mmdrop() in finish_task_switch(). */ + /* will mmdrop_lazy_tlb() in finish_task_switch(). */ rq->prev_mm = prev->active_mm; prev->active_mm = NULL; } @@ -8239,7 +8240,7 @@ void __init sched_init(void) /* * The boot idle thread does lazy MMU switching as well: */ - mmgrab(&init_mm); + mmgrab_lazy_tlb(&init_mm); enter_lazy_tlb(&init_mm, current); /*
Add explicit _lazy_tlb annotated functions for lazy mm refcounting. This makes lazy mm references more obvious, and allows explicit refcounting to be removed if it is not used. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/arm/mach-rpc/ecard.c | 2 +- arch/powerpc/kernel/smp.c | 2 +- arch/powerpc/mm/book3s64/radix_tlb.c | 4 ++-- fs/exec.c | 4 ++-- include/linux/sched/mm.h | 11 +++++++++++ kernel/cpu.c | 2 +- kernel/exit.c | 2 +- kernel/kthread.c | 11 +++++++---- kernel/sched/core.c | 15 ++++++++------- 9 files changed, 34 insertions(+), 19 deletions(-)