linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible
@ 2017-07-09  7:40 Wanpeng Li
  2017-07-14  6:49 ` Wanpeng Li
  2017-07-14  9:10 ` [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context tip-bot for Wanpeng Li
  0 siblings, 2 replies; 5+ messages in thread
From: Wanpeng Li @ 2017-07-09  7:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Peter Zijlstra, Wanpeng Li, Thomas Gleixner,
	Luiz Capitulino, Frederic Weisbecker, Rik van Riel

From: Wanpeng Li <wanpeng.li@hotmail.com>

 BUG: using smp_processor_id() in preemptible [00000000] code: 99-trinity/181
 caller is debug_smp_processor_id+0x17/0x19
 CPU: 0 PID: 181 Comm: 99-trinity Not tainted 4.12.0-01059-g2a42eb9 #1
 Call Trace:
  dump_stack+0x82/0xb8
  check_preemption_disabled+0xd1/0xe3
  debug_smp_processor_id+0x17/0x19
  vtime_delta+0xd/0x2c
  task_cputime+0x89/0xdb
  thread_group_cputime+0x11b/0x1ed
  thread_group_cputime_adjusted+0x1f/0x47
  wait_consider_task+0x2a9/0xaf9
  ? lock_acquire+0x97/0xa4
  do_wait+0xdf/0x1f4
  SYSC_wait4+0x8e/0xb5
  ? list_add+0x34/0x34
  SyS_wait4+0x9/0xb
  do_syscall_64+0x70/0x82
  entry_SYSCALL64_slow_path+0x25/0x25

As Frederic pointed out:

| Although those sched_clock_cpu() things seem to only matter when the
| sched_clock() is unstable. And that stability is a condition for nohz_full
| to work anyway. So probably sched_clock() alone would be enough.

This patch fixes it by replacing sched_clock_cpu() by sched_clock() to 
avoid to call smp_processor_id() in preemptible context.

Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 kernel/sched/cputime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 6e3ea4a..14d2dbf 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -683,7 +683,7 @@ static u64 vtime_delta(struct vtime *vtime)
 {
 	unsigned long long clock;
 
-	clock = sched_clock_cpu(smp_processor_id());
+	clock = sched_clock();
 	if (clock < vtime->starttime)
 		return 0;
 
@@ -814,7 +814,7 @@ void arch_vtime_task_switch(struct task_struct *prev)
 
 	write_seqcount_begin(&vtime->seqcount);
 	vtime->state = VTIME_SYS;
-	vtime->starttime = sched_clock_cpu(smp_processor_id());
+	vtime->starttime = sched_clock();
 	write_seqcount_end(&vtime->seqcount);
 }
 
@@ -826,7 +826,7 @@ void vtime_init_idle(struct task_struct *t, int cpu)
 	local_irq_save(flags);
 	write_seqcount_begin(&vtime->seqcount);
 	vtime->state = VTIME_SYS;
-	vtime->starttime = sched_clock_cpu(cpu);
+	vtime->starttime = sched_clock();
 	write_seqcount_end(&vtime->seqcount);
 	local_irq_restore(flags);
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible
  2017-07-09  7:40 [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible Wanpeng Li
@ 2017-07-14  6:49 ` Wanpeng Li
       [not found]   ` <CAJzB8QHu3Jfy6+H_qxJd83696qsLBnJp0ke55iaJ-JMqZ0TAGQ@mail.gmail.com>
  2017-07-14  9:10 ` [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context tip-bot for Wanpeng Li
  1 sibling, 1 reply; 5+ messages in thread
From: Wanpeng Li @ 2017-07-14  6:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Peter Zijlstra, Wanpeng Li, Thomas Gleixner,
	Luiz Capitulino, Frederic Weisbecker, Rik van Riel

Ping for the merge window. :)
2017-07-09 15:40 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
>  BUG: using smp_processor_id() in preemptible [00000000] code: 99-trinity/181
>  caller is debug_smp_processor_id+0x17/0x19
>  CPU: 0 PID: 181 Comm: 99-trinity Not tainted 4.12.0-01059-g2a42eb9 #1
>  Call Trace:
>   dump_stack+0x82/0xb8
>   check_preemption_disabled+0xd1/0xe3
>   debug_smp_processor_id+0x17/0x19
>   vtime_delta+0xd/0x2c
>   task_cputime+0x89/0xdb
>   thread_group_cputime+0x11b/0x1ed
>   thread_group_cputime_adjusted+0x1f/0x47
>   wait_consider_task+0x2a9/0xaf9
>   ? lock_acquire+0x97/0xa4
>   do_wait+0xdf/0x1f4
>   SYSC_wait4+0x8e/0xb5
>   ? list_add+0x34/0x34
>   SyS_wait4+0x9/0xb
>   do_syscall_64+0x70/0x82
>   entry_SYSCALL64_slow_path+0x25/0x25
>
> As Frederic pointed out:
>
> | Although those sched_clock_cpu() things seem to only matter when the
> | sched_clock() is unstable. And that stability is a condition for nohz_full
> | to work anyway. So probably sched_clock() alone would be enough.
>
> This patch fixes it by replacing sched_clock_cpu() by sched_clock() to
> avoid to call smp_processor_id() in preemptible context.
>
> Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  kernel/sched/cputime.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 6e3ea4a..14d2dbf 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -683,7 +683,7 @@ static u64 vtime_delta(struct vtime *vtime)
>  {
>         unsigned long long clock;
>
> -       clock = sched_clock_cpu(smp_processor_id());
> +       clock = sched_clock();
>         if (clock < vtime->starttime)
>                 return 0;
>
> @@ -814,7 +814,7 @@ void arch_vtime_task_switch(struct task_struct *prev)
>
>         write_seqcount_begin(&vtime->seqcount);
>         vtime->state = VTIME_SYS;
> -       vtime->starttime = sched_clock_cpu(smp_processor_id());
> +       vtime->starttime = sched_clock();
>         write_seqcount_end(&vtime->seqcount);
>  }
>
> @@ -826,7 +826,7 @@ void vtime_init_idle(struct task_struct *t, int cpu)
>         local_irq_save(flags);
>         write_seqcount_begin(&vtime->seqcount);
>         vtime->state = VTIME_SYS;
> -       vtime->starttime = sched_clock_cpu(cpu);
> +       vtime->starttime = sched_clock();
>         write_seqcount_end(&vtime->seqcount);
>         local_irq_restore(flags);
>  }
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context
  2017-07-09  7:40 [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible Wanpeng Li
  2017-07-14  6:49 ` Wanpeng Li
@ 2017-07-14  9:10 ` tip-bot for Wanpeng Li
  2017-07-17  2:31   ` Frederic Weisbecker
  1 sibling, 1 reply; 5+ messages in thread
From: tip-bot for Wanpeng Li @ 2017-07-14  9:10 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: wanpeng.li, torvalds, riel, peterz, tglx, hpa, lcapitulino,
	linux-kernel, xiaolong.ye, fweisbec, mingo

Commit-ID:  0e4097c3354e2f5a5ad8affd9dc7f7f7d00bb6b9
Gitweb:     http://git.kernel.org/tip/0e4097c3354e2f5a5ad8affd9dc7f7f7d00bb6b9
Author:     Wanpeng Li <wanpeng.li@hotmail.com>
AuthorDate: Sun, 9 Jul 2017 00:40:28 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 14 Jul 2017 10:27:15 +0200

sched/cputime: Don't use smp_processor_id() in preemptible context

Recent kernels trigger this warning:

 BUG: using smp_processor_id() in preemptible [00000000] code: 99-trinity/181
 caller is debug_smp_processor_id+0x17/0x19
 CPU: 0 PID: 181 Comm: 99-trinity Not tainted 4.12.0-01059-g2a42eb9 #1
 Call Trace:
  dump_stack+0x82/0xb8
  check_preemption_disabled()
  debug_smp_processor_id()
  vtime_delta()
  task_cputime()
  thread_group_cputime()
  thread_group_cputime_adjusted()
  wait_consider_task()
  do_wait()
  SYSC_wait4()
  do_syscall_64()
  entry_SYSCALL64_slow_path()

As Frederic pointed out:

| Although those sched_clock_cpu() things seem to only matter when the
| sched_clock() is unstable. And that stability is a condition for nohz_full
| to work anyway. So probably sched_clock() alone would be enough.

This patch fixes it by replacing sched_clock_cpu() with sched_clock() to
avoid calling smp_processor_id() in a preemptible context.

Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1499586028-7402-1-git-send-email-wanpeng.li@hotmail.com
[ Prettified the changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/cputime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 6e3ea4a..14d2dbf 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -683,7 +683,7 @@ static u64 vtime_delta(struct vtime *vtime)
 {
 	unsigned long long clock;
 
-	clock = sched_clock_cpu(smp_processor_id());
+	clock = sched_clock();
 	if (clock < vtime->starttime)
 		return 0;
 
@@ -814,7 +814,7 @@ void arch_vtime_task_switch(struct task_struct *prev)
 
 	write_seqcount_begin(&vtime->seqcount);
 	vtime->state = VTIME_SYS;
-	vtime->starttime = sched_clock_cpu(smp_processor_id());
+	vtime->starttime = sched_clock();
 	write_seqcount_end(&vtime->seqcount);
 }
 
@@ -826,7 +826,7 @@ void vtime_init_idle(struct task_struct *t, int cpu)
 	local_irq_save(flags);
 	write_seqcount_begin(&vtime->seqcount);
 	vtime->state = VTIME_SYS;
-	vtime->starttime = sched_clock_cpu(cpu);
+	vtime->starttime = sched_clock();
 	write_seqcount_end(&vtime->seqcount);
 	local_irq_restore(flags);
 }

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context
  2017-07-14  9:10 ` [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context tip-bot for Wanpeng Li
@ 2017-07-17  2:31   ` Frederic Weisbecker
  0 siblings, 0 replies; 5+ messages in thread
From: Frederic Weisbecker @ 2017-07-17  2:31 UTC (permalink / raw)
  To: riel, peterz, wanpeng.li, torvalds, xiaolong.ye, mingo, tglx,
	hpa, lcapitulino, linux-kernel
  Cc: linux-tip-commits

On Fri, Jul 14, 2017 at 02:10:14AM -0700, tip-bot for Wanpeng Li wrote:
> Commit-ID:  0e4097c3354e2f5a5ad8affd9dc7f7f7d00bb6b9
> Gitweb:     http://git.kernel.org/tip/0e4097c3354e2f5a5ad8affd9dc7f7f7d00bb6b9
> Author:     Wanpeng Li <wanpeng.li@hotmail.com>
> AuthorDate: Sun, 9 Jul 2017 00:40:28 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 14 Jul 2017 10:27:15 +0200
> 
> sched/cputime: Don't use smp_processor_id() in preemptible context
> 
> Recent kernels trigger this warning:
> 
>  BUG: using smp_processor_id() in preemptible [00000000] code: 99-trinity/181
>  caller is debug_smp_processor_id+0x17/0x19
>  CPU: 0 PID: 181 Comm: 99-trinity Not tainted 4.12.0-01059-g2a42eb9 #1
>  Call Trace:
>   dump_stack+0x82/0xb8
>   check_preemption_disabled()
>   debug_smp_processor_id()
>   vtime_delta()
>   task_cputime()
>   thread_group_cputime()
>   thread_group_cputime_adjusted()
>   wait_consider_task()
>   do_wait()
>   SYSC_wait4()
>   do_syscall_64()
>   entry_SYSCALL64_slow_path()
> 
> As Frederic pointed out:
> 
> | Although those sched_clock_cpu() things seem to only matter when the
> | sched_clock() is unstable. And that stability is a condition for nohz_full
> | to work anyway. So probably sched_clock() alone would be enough.
> 
> This patch fixes it by replacing sched_clock_cpu() with sched_clock() to
> avoid calling smp_processor_id() in a preemptible context.
> 
> Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/1499586028-7402-1-git-send-email-wanpeng.li@hotmail.com
> [ Prettified the changelog. ]
> Signed-off-by: Ingo Molnar <mingo@kernel.org>

Thanks! The patch looks good!

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible
       [not found]   ` <CAJzB8QHu3Jfy6+H_qxJd83696qsLBnJp0ke55iaJ-JMqZ0TAGQ@mail.gmail.com>
@ 2017-07-20 22:33     ` Paul McKenney
  0 siblings, 0 replies; 5+ messages in thread
From: Paul McKenney @ 2017-07-20 22:33 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: linux-kernel, Ingo Molnar, Peter Zijlstra, Wanpeng Li,
	Thomas Gleixner, Luiz Capitulino, Frederic Weisbecker,
	Rik van Riel

On Wed, Jul 19, 2017 at 4:38 PM, Paul McKenney <paulmckrcu@gmail.com> wrote:
> On Thu, Jul 13, 2017 at 11:49 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>
>> Ping for the merge window. :)
>> 2017-07-09 15:40 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
>> > From: Wanpeng Li <wanpeng.li@hotmail.com>
>> >
>> >  BUG: using smp_processor_id() in preemptible [00000000] code:
>> > 99-trinity/181
>> >  caller is debug_smp_processor_id+0x17/0x19
>> >  CPU: 0 PID: 181 Comm: 99-trinity Not tainted 4.12.0-01059-g2a42eb9 #1
>> >  Call Trace:
>> >   dump_stack+0x82/0xb8
>> >   check_preemption_disabled+0xd1/0xe3
>> >   debug_smp_processor_id+0x17/0x19
>> >   vtime_delta+0xd/0x2c
>> >   task_cputime+0x89/0xdb
>> >   thread_group_cputime+0x11b/0x1ed
>> >   thread_group_cputime_adjusted+0x1f/0x47
>> >   wait_consider_task+0x2a9/0xaf9
>> >   ? lock_acquire+0x97/0xa4
>> >   do_wait+0xdf/0x1f4
>> >   SYSC_wait4+0x8e/0xb5
>> >   ? list_add+0x34/0x34
>> >   SyS_wait4+0x9/0xb
>> >   do_syscall_64+0x70/0x82
>> >   entry_SYSCALL64_slow_path+0x25/0x25
>> >
>> > As Frederic pointed out:
>> >
>> > | Although those sched_clock_cpu() things seem to only matter when the
>> > | sched_clock() is unstable. And that stability is a condition for
>> > nohz_full
>> > | to work anyway. So probably sched_clock() alone would be enough.
>> >
>> > This patch fixes it by replacing sched_clock_cpu() by sched_clock() to
>> > avoid to call smp_processor_id() in preemptible context.
>
>
> I am hitting this with rcutorture, so have kicked off an overnight
> run with this patch.

Which passed, so:

Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

                                                     Thanx, Paul


>> > Reported-by: Xiaolong Ye <xiaolong.ye@intel.com>
>> > Cc: Thomas Gleixner <tglx@linutronix.de>
>> > Cc: Luiz Capitulino <lcapitulino@redhat.com>
>> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> > Cc: Rik van Riel <riel@redhat.com>
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> > ---
>> >  kernel/sched/cputime.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
>> > index 6e3ea4a..14d2dbf 100644
>> > --- a/kernel/sched/cputime.c
>> > +++ b/kernel/sched/cputime.c
>> > @@ -683,7 +683,7 @@ static u64 vtime_delta(struct vtime *vtime)
>> >  {
>> >         unsigned long long clock;
>> >
>> > -       clock = sched_clock_cpu(smp_processor_id());
>> > +       clock = sched_clock();
>> >         if (clock < vtime->starttime)
>> >                 return 0;
>> >
>> > @@ -814,7 +814,7 @@ void arch_vtime_task_switch(struct task_struct
>> > *prev)
>> >
>> >         write_seqcount_begin(&vtime->seqcount);
>> >         vtime->state = VTIME_SYS;
>> > -       vtime->starttime = sched_clock_cpu(smp_processor_id());
>> > +       vtime->starttime = sched_clock();
>> >         write_seqcount_end(&vtime->seqcount);
>> >  }
>> >
>> > @@ -826,7 +826,7 @@ void vtime_init_idle(struct task_struct *t, int cpu)
>> >         local_irq_save(flags);
>> >         write_seqcount_begin(&vtime->seqcount);
>> >         vtime->state = VTIME_SYS;
>> > -       vtime->starttime = sched_clock_cpu(cpu);
>> > +       vtime->starttime = sched_clock();
>> >         write_seqcount_end(&vtime->seqcount);
>> >         local_irq_restore(flags);
>> >  }
>> > --
>> > 2.7.4
>> >
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-07-20 22:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-09  7:40 [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible Wanpeng Li
2017-07-14  6:49 ` Wanpeng Li
     [not found]   ` <CAJzB8QHu3Jfy6+H_qxJd83696qsLBnJp0ke55iaJ-JMqZ0TAGQ@mail.gmail.com>
2017-07-20 22:33     ` Paul McKenney
2017-07-14  9:10 ` [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context tip-bot for Wanpeng Li
2017-07-17  2:31   ` Frederic Weisbecker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).