From: Yong Zhang <yong.zhang0@gmail.com> To: Peter Zijlstra <peterz@infradead.org> Cc: Oleg Nesterov <oleg@redhat.com>, Marc Zyngier <Marc.Zyngier@arm.com>, Ingo Molnar <mingo@elte.hu>, Frank Rowand <frank.rowand@am.sony.com>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [BUG] "sched: Remove rq->lock from the first half of ttwu()" locks up on ARM Date: Fri, 27 May 2011 15:01:38 +0800 [thread overview] Message-ID: <BANLkTincaA75JR3V2Fd6MDCw6yMhUmDdWA@mail.gmail.com> (raw) In-Reply-To: <1306430633.2497.91.camel@laptop> On Fri, May 27, 2011 at 1:23 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > We'd end up with something like the below, which isn't too different > from what I've now got queued. > > It has the extra cpu == smp_processor_id() check, but I'm not sure this > whole case is worth the trouble. I could go stick some counters in to > verify how often all this happens I guess. > > --- > arch/x86/include/asm/system.h | 2 ++ > kernel/sched.c | 14 +++++++++++--- > kernel/sched_debug.c | 7 +++++++ > 3 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h > index c2ff2a1..2c597e8 100644 > --- a/arch/x86/include/asm/system.h > +++ b/arch/x86/include/asm/system.h > @@ -10,6 +10,8 @@ > #include <linux/kernel.h> > #include <linux/irqflags.h> > > +#define __ARCH_WANT_INTERRUPTS_ON_CTXSW > + > /* entries in ARCH_DLINFO: */ > #if defined(CONFIG_IA32_EMULATION) || !defined(CONFIG_X86_64) > # define AT_VECTOR_SIZE_ARCH 2 > diff --git a/kernel/sched.c b/kernel/sched.c > index 2d12893..e4f7a9f 100644 > --- a/kernel/sched.c > +++ b/kernel/sched.c > @@ -2636,9 +2636,17 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags) > * to spin on ->on_cpu if p is current, since that would > * deadlock. > */ > - if (p == current) { > - ttwu_queue(p, cpu); > - goto stat; > + if (cpu == smp_processor_id()) { > + struct rq *rq; > + > + rq = __task_rq_lock(p); > + if (p->on_cpu) { As Oleg has said, I also think we don't need this check. > + ttwu_activate(rq, p, ENQUEUE_WAKEUP); > + ttwu_do_wakeup(rq, p, wake_flags); And the difference with ttwu_queue() is ttwu_queue() calls ttwu_activate() with another flag ENQUEUE_WAKING, so if we call ->task_waking() before ttwu_queue(), I guess it will work too. But I like this version, because we call ->task_waking() and ttwu_activate() on the local cpu, that means the calculations on vruntime in that two functions are accumulated into noop. Thanks, Yong > + __task_rq_unlock(rq); > + goto stat; > + } > + __task_rq_unlock(rq); > } > #endif > cpu_relax(); > diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c > index a6710a1..f0ff1de 100644 > --- a/kernel/sched_debug.c > +++ b/kernel/sched_debug.c > @@ -332,6 +332,13 @@ static int sched_debug_show(struct seq_file *m, void *v) > (int)strcspn(init_utsname()->version, " "), > init_utsname()->version); > > +#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW > + SEQ_printf(m, "__ARCH_WANT_INTERRUPTS_ON_CTXSW\n"); > +#endif > +#ifdef __ARCH_WANT_UNLOCKED_CTXSW > + SEQ_printf(m, "__ARCH_WANT_UNLOCKED_CTXSW\n"); > +#endif > + > #define P(x) \ > SEQ_printf(m, "%-40s: %Ld\n", #x, (long long)(x)) > #define PN(x) \ > > > -- Only stand for myself
WARNING: multiple messages have this Message-ID (diff)
From: yong.zhang0@gmail.com (Yong Zhang) To: linux-arm-kernel@lists.infradead.org Subject: [BUG] "sched: Remove rq->lock from the first half of ttwu()" locks up on ARM Date: Fri, 27 May 2011 15:01:38 +0800 [thread overview] Message-ID: <BANLkTincaA75JR3V2Fd6MDCw6yMhUmDdWA@mail.gmail.com> (raw) In-Reply-To: <1306430633.2497.91.camel@laptop> On Fri, May 27, 2011 at 1:23 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > We'd end up with something like the below, which isn't too different > from what I've now got queued. > > It has the extra cpu == smp_processor_id() check, but I'm not sure this > whole case is worth the trouble. I could go stick some counters in to > verify how often all this happens I guess. > > --- > ?arch/x86/include/asm/system.h | ? ?2 ++ > ?kernel/sched.c ? ? ? ? ? ? ? ?| ? 14 +++++++++++--- > ?kernel/sched_debug.c ? ? ? ? ?| ? ?7 +++++++ > ?3 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h > index c2ff2a1..2c597e8 100644 > --- a/arch/x86/include/asm/system.h > +++ b/arch/x86/include/asm/system.h > @@ -10,6 +10,8 @@ > ?#include <linux/kernel.h> > ?#include <linux/irqflags.h> > > +#define __ARCH_WANT_INTERRUPTS_ON_CTXSW > + > ?/* entries in ARCH_DLINFO: */ > ?#if defined(CONFIG_IA32_EMULATION) || !defined(CONFIG_X86_64) > ?# define AT_VECTOR_SIZE_ARCH 2 > diff --git a/kernel/sched.c b/kernel/sched.c > index 2d12893..e4f7a9f 100644 > --- a/kernel/sched.c > +++ b/kernel/sched.c > @@ -2636,9 +2636,17 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags) > ? ? ? ? ? ? ? ? * to spin on ->on_cpu if p is current, since that would > ? ? ? ? ? ? ? ? * deadlock. > ? ? ? ? ? ? ? ? */ > - ? ? ? ? ? ? ? if (p == current) { > - ? ? ? ? ? ? ? ? ? ? ? ttwu_queue(p, cpu); > - ? ? ? ? ? ? ? ? ? ? ? goto stat; > + ? ? ? ? ? ? ? if (cpu == smp_processor_id()) { > + ? ? ? ? ? ? ? ? ? ? ? struct rq *rq; > + > + ? ? ? ? ? ? ? ? ? ? ? rq = __task_rq_lock(p); > + ? ? ? ? ? ? ? ? ? ? ? if (p->on_cpu) { As Oleg has said, I also think we don't need this check. > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ttwu_activate(rq, p, ENQUEUE_WAKEUP); > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ttwu_do_wakeup(rq, p, wake_flags); And the difference with ttwu_queue() is ttwu_queue() calls ttwu_activate() with another flag ENQUEUE_WAKING, so if we call ->task_waking() before ttwu_queue(), I guess it will work too. But I like this version, because we call ->task_waking() and ttwu_activate() on the local cpu, that means the calculations on vruntime in that two functions are accumulated into noop. Thanks, Yong > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __task_rq_unlock(rq); > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto stat; > + ? ? ? ? ? ? ? ? ? ? ? } > + ? ? ? ? ? ? ? ? ? ? ? __task_rq_unlock(rq); > ? ? ? ? ? ? ? ?} > ?#endif > ? ? ? ? ? ? ? ?cpu_relax(); > diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c > index a6710a1..f0ff1de 100644 > --- a/kernel/sched_debug.c > +++ b/kernel/sched_debug.c > @@ -332,6 +332,13 @@ static int sched_debug_show(struct seq_file *m, void *v) > ? ? ? ? ? ? ? ?(int)strcspn(init_utsname()->version, " "), > ? ? ? ? ? ? ? ?init_utsname()->version); > > +#ifdef __ARCH_WANT_INTERRUPTS_ON_CTXSW > + ? ? ? SEQ_printf(m, "__ARCH_WANT_INTERRUPTS_ON_CTXSW\n"); > +#endif > +#ifdef __ARCH_WANT_UNLOCKED_CTXSW > + ? ? ? SEQ_printf(m, "__ARCH_WANT_UNLOCKED_CTXSW\n"); > +#endif > + > ?#define P(x) \ > ? ? ? ?SEQ_printf(m, "%-40s: %Ld\n", #x, (long long)(x)) > ?#define PN(x) \ > > > -- Only stand for myself
next prev parent reply other threads:[~2011-05-27 7:01 UTC|newest] Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-24 18:13 [BUG] "sched: Remove rq->lock from the first half of ttwu()" locks up on ARM Marc Zyngier 2011-05-24 18:13 ` Marc Zyngier 2011-05-24 21:32 ` Peter Zijlstra 2011-05-24 21:32 ` Peter Zijlstra 2011-05-24 21:39 ` Ingo Molnar 2011-05-24 21:39 ` Ingo Molnar 2011-05-25 12:23 ` Marc Zyngier 2011-05-25 12:23 ` Marc Zyngier 2011-05-25 17:08 ` Peter Zijlstra 2011-05-25 17:08 ` Peter Zijlstra 2011-05-25 21:15 ` Peter Zijlstra 2011-05-25 21:15 ` Peter Zijlstra 2011-05-26 7:29 ` Yong Zhang 2011-05-26 7:29 ` Yong Zhang 2011-05-26 10:32 ` Peter Zijlstra 2011-05-26 10:32 ` Peter Zijlstra 2011-05-26 11:02 ` Marc Zyngier 2011-05-26 11:02 ` Marc Zyngier 2011-05-26 11:32 ` Peter Zijlstra 2011-05-26 11:32 ` Peter Zijlstra 2011-05-26 12:21 ` Peter Zijlstra 2011-05-26 12:21 ` Peter Zijlstra 2011-05-26 12:26 ` Ingo Molnar 2011-05-26 12:26 ` Ingo Molnar 2011-05-26 12:31 ` Russell King - ARM Linux 2011-05-26 12:31 ` Russell King - ARM Linux 2011-05-26 12:37 ` Peter Zijlstra 2011-05-26 12:37 ` Peter Zijlstra 2011-05-26 12:50 ` Ingo Molnar 2011-05-26 12:50 ` Ingo Molnar 2011-05-26 13:36 ` Russell King - ARM Linux 2011-05-26 13:36 ` Russell King - ARM Linux 2011-05-26 14:45 ` Catalin Marinas 2011-05-26 14:45 ` Catalin Marinas 2011-05-27 12:06 ` Ingo Molnar 2011-05-27 12:06 ` Ingo Molnar 2011-05-27 17:55 ` Russell King - ARM Linux 2011-05-27 17:55 ` Russell King - ARM Linux 2011-05-27 19:41 ` Nicolas Pitre 2011-05-27 19:41 ` Nicolas Pitre 2011-05-27 20:52 ` Russell King - ARM Linux 2011-05-27 20:52 ` Russell King - ARM Linux 2011-05-28 13:13 ` Peter Zijlstra 2011-05-28 13:13 ` Peter Zijlstra 2011-05-31 11:08 ` Michal Simek 2011-05-31 11:08 ` Michal Simek 2011-05-31 13:22 ` Peter Zijlstra 2011-05-31 13:22 ` Peter Zijlstra 2011-05-31 13:37 ` Michal Simek 2011-05-31 13:37 ` Michal Simek 2011-05-31 13:52 ` Peter Zijlstra 2011-05-31 13:52 ` Peter Zijlstra 2011-05-31 14:08 ` Michal Simek 2011-05-31 14:08 ` Michal Simek 2011-05-31 14:29 ` Peter Zijlstra 2011-05-31 14:29 ` Peter Zijlstra 2011-05-29 10:21 ` Catalin Marinas 2011-05-29 10:21 ` Catalin Marinas 2011-05-29 10:26 ` Russell King - ARM Linux 2011-05-29 10:26 ` Russell King - ARM Linux 2011-05-29 12:01 ` Catalin Marinas 2011-05-29 12:01 ` Catalin Marinas 2011-05-29 13:19 ` Russell King - ARM Linux 2011-05-29 13:19 ` Russell King - ARM Linux 2011-05-29 21:21 ` Catalin Marinas 2011-05-29 21:21 ` Catalin Marinas 2011-05-29 9:51 ` Catalin Marinas 2011-05-29 9:51 ` Catalin Marinas 2011-06-06 10:29 ` Pavel Machek 2011-06-06 10:29 ` Pavel Machek 2011-05-26 14:56 ` Marc Zyngier 2011-05-26 14:56 ` Marc Zyngier 2011-05-26 15:45 ` Oleg Nesterov 2011-05-26 15:45 ` Oleg Nesterov 2011-05-26 15:59 ` Peter Zijlstra 2011-05-26 15:59 ` Peter Zijlstra 2011-05-26 16:09 ` Peter Zijlstra 2011-05-26 16:09 ` Peter Zijlstra 2011-05-26 16:20 ` Marc Zyngier 2011-05-26 16:20 ` Marc Zyngier 2011-05-26 16:32 ` Peter Zijlstra 2011-05-26 16:32 ` Peter Zijlstra 2011-05-27 8:01 ` Marc Zyngier 2011-05-27 8:01 ` Marc Zyngier 2011-05-26 16:22 ` Marc Zyngier 2011-05-26 16:22 ` Marc Zyngier 2011-05-26 17:04 ` Oleg Nesterov 2011-05-26 17:04 ` Oleg Nesterov 2011-05-26 17:17 ` Peter Zijlstra 2011-05-26 17:17 ` Peter Zijlstra 2011-05-26 17:23 ` Peter Zijlstra 2011-05-26 17:23 ` Peter Zijlstra 2011-05-26 17:49 ` Oleg Nesterov 2011-05-26 17:49 ` Oleg Nesterov 2011-05-27 7:01 ` Yong Zhang [this message] 2011-05-27 7:01 ` Yong Zhang 2011-05-27 15:23 ` Santosh Shilimkar 2011-05-27 15:23 ` Santosh Shilimkar 2011-05-27 15:29 ` Marc Zyngier 2011-05-27 15:29 ` Marc Zyngier 2011-05-27 15:30 ` Santosh Shilimkar 2011-05-27 15:30 ` Santosh Shilimkar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=BANLkTincaA75JR3V2Fd6MDCw6yMhUmDdWA@mail.gmail.com \ --to=yong.zhang0@gmail.com \ --cc=Marc.Zyngier@arm.com \ --cc=frank.rowand@am.sony.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@elte.hu \ --cc=oleg@redhat.com \ --cc=peterz@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.