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

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

[Wanpeng Li]

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

This patch fixes it by replacing sched_clock_cpu() in vtime_delta() by 
local_clock() for effectively raw_smp_processor_id().

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>
---
v1 -> v2:
 * replace sched_clock_cpu() by local_clock()

 kernel/sched/cputime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 6e3ea4a..d86b94e 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 = local_clock();
 	if (clock < vtime->starttime)
 		return 0;
 
-- 
2.7.4

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

[Frederic Weisbecker]

On Fri, Jul 07, 2017 at 02:08:25AM -0700, Wanpeng Li wrote:
> 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
> 
> This patch fixes it by replacing sched_clock_cpu() in vtime_delta() by 
> local_clock() for effectively raw_smp_processor_id().

That's also broken because task_cputime() can be called from a different CPU than
where the target task is running on, even though there shouldn't be practical effect
as the clock must be stable but still the code would be confusing.

No I think you can still use sched_clock(), just make sure you also use it on
arch_vtime_task_switch() and vtime_init_idle().

Thanks.

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

[Wanpeng Li]

2017-07-07 20:01 GMT+08:00 Frederic Weisbecker <fweisbec@gmail.com>:
> On Fri, Jul 07, 2017 at 02:08:25AM -0700, Wanpeng Li wrote:
>> 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
>>
>> This patch fixes it by replacing sched_clock_cpu() in vtime_delta() by
>> local_clock() for effectively raw_smp_processor_id().
>
> That's also broken because task_cputime() can be called from a different CPU than
> where the target task is running on, even though there shouldn't be practical effect

Agreed.

> as the clock must be stable but still the code would be confusing.
>
> No I think you can still use sched_clock(), just make sure you also use it on
> arch_vtime_task_switch() and vtime_init_idle().

Is it acceptable to you, Peterz? :)

Regards,
Wanpeng Li