2017-07-09 07:40:40

by Wanpeng Li

Subject: [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible

From: Wanpeng Li <[email protected]>

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 <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Luiz Capitulino <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
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

2017-07-14 06:49:56

by Wanpeng Li

Subject: Re: [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible

Ping for the merge window. :)
2017-07-09 15:40 GMT+08:00 Wanpeng Li <[email protected]>:
> From: Wanpeng Li <[email protected]>
>
> 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 <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Luiz Capitulino <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> 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
>

2017-07-14 09:15:01

by tip-bot for Vasyl Gomonovych

Subject: [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context

Commit-ID: 0e4097c3354e2f5a5ad8affd9dc7f7f7d00bb6b9
Gitweb: http://git.kernel.org/tip/0e4097c3354e2f5a5ad8affd9dc7f7f7d00bb6b9
Author: Wanpeng Li <[email protected]>
AuthorDate: Sun, 9 Jul 2017 00:40:28 -0700
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Luiz Capitulino <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ Prettified the changelog. ]
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
}

2017-07-17 02:31:18

by Frederic Weisbecker

Subject: Re: [tip:sched/urgent] sched/cputime: Don't use smp_processor_id() in preemptible context

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 <[email protected]>
> AuthorDate: Sun, 9 Jul 2017 00:40:28 -0700
> Committer: Ingo Molnar <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Luiz Capitulino <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> [ Prettified the changelog. ]
> Signed-off-by: Ingo Molnar <[email protected]>

Thanks! The patch looks good!

2017-07-20 22:33:03

by Paul McKenney

Subject: Re: [PATCH v3] sched/cputime: Fix using smp_processor_id() in preemptible

On Wed, Jul 19, 2017 at 4:38 PM, Paul McKenney <[email protected]> wrote:
> On Thu, Jul 13, 2017 at 11:49 PM, Wanpeng Li <[email protected]> wrote:
>>
>> Ping for the merge window. :)
>> 2017-07-09 15:40 GMT+08:00 Wanpeng Li <[email protected]>:
>> > From: Wanpeng Li <[email protected]>
>> >
>> > 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 <[email protected]>

Thanx, Paul


>> > Reported-by: Xiaolong Ye <[email protected]>
>> > Cc: Thomas Gleixner <[email protected]>
>> > Cc: Luiz Capitulino <[email protected]>
>> > Cc: Frederic Weisbecker <[email protected]>
>> > Cc: Rik van Riel <[email protected]>
>> > Cc: Peter Zijlstra <[email protected]>
>> > Cc: Ingo Molnar <[email protected]>
>> > Signed-off-by: Wanpeng Li <[email protected]>
>> > ---
>> > 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
>> >
>
>