From: Steven Rostedt <rostedt@goodmis.org> To: <cl@rock-chips.com> Cc: heiko@sntech.de, mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com, vincent.guittot@linaro.org, dietmar.eggemann@arm.com, bsegall@google.com, mgorman@suse.de, akpm@linux-foundation.org, tglx@linutronix.de, mpe@ellerman.id.au, surenb@google.com, ben.dooks@codethink.co.uk, anshuman.khandual@arm.com, catalin.marinas@arm.com, will@kernel.org, keescook@chromium.org, luto@amacapital.net, wad@chromium.org, mark.rutland@arm.com, geert+renesas@glider.be, george_davis@mentor.com, sudeep.holla@arm.com, linux@armlinux.org.uk, gregkh@linuxfoundation.org, info@metux.net, kstewart@linuxfoundation.org, allison@lohutok.net, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, huangtao@rock-chips.com Subject: Re: [PATCH v3 1/1] kthread: do not preempt current task if it is going to call schedule() Date: Fri, 6 Mar 2020 12:09:45 -0500 [thread overview] Message-ID: <20200306120945.6a197172@gandalf.local.home> (raw) In-Reply-To: <20200306070133.18335-2-cl@rock-chips.com> On Fri, 6 Mar 2020 15:01:33 +0800 <cl@rock-chips.com> wrote: > From: Liang Chen <cl@rock-chips.com> > > when we create a kthread with ktrhead_create_on_cpu(),the child thread > entry is ktread.c:ktrhead() which will be preempted by the parent after > call complete(done) while schedule() is not called yet,then the parent > will call wait_task_inactive(child) but the child is still on the runqueue, > so the parent will schedule_hrtimeout() for 1 jiffy,it will waste a lot of > time,especially on startup. > > parent child > ktrhead_create_on_cpu() > wait_fo_completion(&done) -----> ktread.c:ktrhead() > |----- complete(done);--wakeup and preempted by parent > kthread_bind() <------------| |-> schedule();--dequeue here > wait_task_inactive(child) | > schedule_hrtimeout(1 jiffy) -| > > So we hope the child just wakeup parent but not preempted by parent, and the > child is going to call schedule() soon,then the parent will not call > schedule_hrtimeout(1 jiffy) as the child is already dequeue. > > The same issue for ktrhead_park()&&kthread_parkme(). > This patch can save 120ms on rk312x startup with CONFIG_HZ=300. > > Signed-off-by: Liang Chen <cl@rock-chips.com> > --- > kernel/kthread.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/kernel/kthread.c b/kernel/kthread.c > index b262f47046ca..bfbfa481be3a 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -199,8 +199,15 @@ static void __kthread_parkme(struct kthread *self) > if (!test_bit(KTHREAD_SHOULD_PARK, &self->flags)) > break; > > + /* > + * Thread is going to call schedule(), do not preempt it, > + * or the caller of kthread_park() may spend more time in > + * wait_task_inactive(). > + */ > + preempt_disable(); > complete(&self->parked); I first was concerned that this could break PREEMPT_RT, as complete() calls spin_locks() which are turned into sleeping locks when PREEMPT_RT is enabled. But looking at the latest PREEMPT_RT patch, it appears that it converts the locks in complete() into raw_spin_locks (and using swake). I don't see any other issue with this patch. Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > - schedule(); > + schedule_preempt_disabled(); > + preempt_enable(); > } > __set_current_state(TASK_RUNNING); > } > @@ -245,8 +252,14 @@ static int kthread(void *_create) > /* OK, tell user we're spawned, wait for stop or wakeup */ > __set_current_state(TASK_UNINTERRUPTIBLE); > create->result = current; > + /* > + * Thread is going to call schedule(), do not preempt it, > + * or the creator may spend more time in wait_task_inactive(). > + */ > + preempt_disable(); > complete(done); > - schedule(); > + schedule_preempt_disabled(); > + preempt_enable(); > > ret = -EINTR; > if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
WARNING: multiple messages have this Message-ID (diff)
From: Steven Rostedt <rostedt@goodmis.org> To: <cl@rock-chips.com> Cc: juri.lelli@redhat.com, mark.rutland@arm.com, heiko@sntech.de, geert+renesas@glider.be, peterz@infradead.org, catalin.marinas@arm.com, bsegall@google.com, will@kernel.org, mpe@ellerman.id.au, linux@armlinux.org.uk, dietmar.eggemann@arm.com, ben.dooks@codethink.co.uk, mgorman@suse.de, huangtao@rock-chips.com, keescook@chromium.org, anshuman.khandual@arm.com, tglx@linutronix.de, surenb@google.com, mingo@redhat.com, allison@lohutok.net, linux-arm-kernel@lists.infradead.org, wad@chromium.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, luto@amacapital.net, george_davis@mentor.com, sudeep.holla@arm.com, akpm@linux-foundation.org, info@metux.net, kstewart@linuxfoundation.org Subject: Re: [PATCH v3 1/1] kthread: do not preempt current task if it is going to call schedule() Date: Fri, 6 Mar 2020 12:09:45 -0500 [thread overview] Message-ID: <20200306120945.6a197172@gandalf.local.home> (raw) In-Reply-To: <20200306070133.18335-2-cl@rock-chips.com> On Fri, 6 Mar 2020 15:01:33 +0800 <cl@rock-chips.com> wrote: > From: Liang Chen <cl@rock-chips.com> > > when we create a kthread with ktrhead_create_on_cpu(),the child thread > entry is ktread.c:ktrhead() which will be preempted by the parent after > call complete(done) while schedule() is not called yet,then the parent > will call wait_task_inactive(child) but the child is still on the runqueue, > so the parent will schedule_hrtimeout() for 1 jiffy,it will waste a lot of > time,especially on startup. > > parent child > ktrhead_create_on_cpu() > wait_fo_completion(&done) -----> ktread.c:ktrhead() > |----- complete(done);--wakeup and preempted by parent > kthread_bind() <------------| |-> schedule();--dequeue here > wait_task_inactive(child) | > schedule_hrtimeout(1 jiffy) -| > > So we hope the child just wakeup parent but not preempted by parent, and the > child is going to call schedule() soon,then the parent will not call > schedule_hrtimeout(1 jiffy) as the child is already dequeue. > > The same issue for ktrhead_park()&&kthread_parkme(). > This patch can save 120ms on rk312x startup with CONFIG_HZ=300. > > Signed-off-by: Liang Chen <cl@rock-chips.com> > --- > kernel/kthread.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/kernel/kthread.c b/kernel/kthread.c > index b262f47046ca..bfbfa481be3a 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -199,8 +199,15 @@ static void __kthread_parkme(struct kthread *self) > if (!test_bit(KTHREAD_SHOULD_PARK, &self->flags)) > break; > > + /* > + * Thread is going to call schedule(), do not preempt it, > + * or the caller of kthread_park() may spend more time in > + * wait_task_inactive(). > + */ > + preempt_disable(); > complete(&self->parked); I first was concerned that this could break PREEMPT_RT, as complete() calls spin_locks() which are turned into sleeping locks when PREEMPT_RT is enabled. But looking at the latest PREEMPT_RT patch, it appears that it converts the locks in complete() into raw_spin_locks (and using swake). I don't see any other issue with this patch. Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > - schedule(); > + schedule_preempt_disabled(); > + preempt_enable(); > } > __set_current_state(TASK_RUNNING); > } > @@ -245,8 +252,14 @@ static int kthread(void *_create) > /* OK, tell user we're spawned, wait for stop or wakeup */ > __set_current_state(TASK_UNINTERRUPTIBLE); > create->result = current; > + /* > + * Thread is going to call schedule(), do not preempt it, > + * or the creator may spend more time in wait_task_inactive(). > + */ > + preempt_disable(); > complete(done); > - schedule(); > + schedule_preempt_disabled(); > + preempt_enable(); > > ret = -EINTR; > if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) { _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-03-06 17:09 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-06 7:01 [PATCH v3 0/1] wait_task_inactive() spend too much time on system startup cl 2020-03-06 7:01 ` [PATCH v3 1/1] kthread: do not preempt current task if it is going to call schedule() cl 2020-03-06 17:09 ` Steven Rostedt [this message] 2020-03-06 17:09 ` Steven Rostedt 2020-03-11 15:39 ` Peter Zijlstra 2020-03-11 15:39 ` Peter Zijlstra 2020-03-20 12:58 ` [tip: sched/core] kthread: Do " tip-bot2 for Liang Chen
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=20200306120945.6a197172@gandalf.local.home \ --to=rostedt@goodmis.org \ --cc=akpm@linux-foundation.org \ --cc=allison@lohutok.net \ --cc=anshuman.khandual@arm.com \ --cc=ben.dooks@codethink.co.uk \ --cc=bsegall@google.com \ --cc=catalin.marinas@arm.com \ --cc=cl@rock-chips.com \ --cc=dietmar.eggemann@arm.com \ --cc=geert+renesas@glider.be \ --cc=george_davis@mentor.com \ --cc=gregkh@linuxfoundation.org \ --cc=heiko@sntech.de \ --cc=huangtao@rock-chips.com \ --cc=info@metux.net \ --cc=juri.lelli@redhat.com \ --cc=keescook@chromium.org \ --cc=kstewart@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=luto@amacapital.net \ --cc=mark.rutland@arm.com \ --cc=mgorman@suse.de \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=peterz@infradead.org \ --cc=sudeep.holla@arm.com \ --cc=surenb@google.com \ --cc=tglx@linutronix.de \ --cc=vincent.guittot@linaro.org \ --cc=wad@chromium.org \ --cc=will@kernel.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.