clock: Fix smp_processor_id() in preemptible bug
PATCH 1/1 clock: Fix smp_processor_id() in preemptible bug
The v4.11-rc1 kernel emits the following splat in some configurations:
[ 43.681891] BUG: using smp_processor_id() in preemptible [00000000] code: kworker/3:1/49 [ 43.682511] caller is debug_smp_processor_id+0x17/0x20 [ 43.682893] CPU: 0 PID: 49 Comm: kworker/3:1 Not tainted 4.11.0-rc1+ #1 [ 43.683382] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 43.683497] Workqueue: events __clear_sched_clock_stable [ 43.683497] Call Trace: [ 43.683497] dump_stack+0x4f/0x69 [ 43.683497] check_preemption_disabled+0xd9/0xf0 [ 43.683497] debug_smp_processor_id+0x17/0x20 [ 43.683497] __clear_sched_clock_stable+0x11/0x60 [ 43.683497] process_one_work+0x146/0x430 [ 43.683497] worker_thread+0x126/0x490 [ 43.683497] kthread+0xfc/0x130 [ 43.683497] ? process_one_work+0x430/0x430 [ 43.683497] ? kthread_create_on_node+0x40/0x40 [ 43.683497] ? umh_complete+0x30/0x30 [ 43.683497] ? call_usermodehelper_exec_async+0x12a/0x130 [ 43.683497] ret_from_fork+0x29/0x40 [ 43.689244] sched_clock: Marking unstable (43688244724, 179505618)<-(43867750342, 0)
This happens because workqueue handlers run with preemption enabled by default and the new this_scd() function accesses per-CPU variables. This commit therefore disables preemption across this call to this_scd() and to the uses of the pointer that it returns. Lightly tested successfully on x86.
kernel/sched/clock.c +4 -1
@@ -143,7 +143,7 @@ static void __set_sched_clock_stable(void)static void __clear_sched_clock_stable (struct work_struct *work) { -struct sched_clock_data *scd =this_scd (); +struct sched_clock_data *scd;/* * Attempt to make the stable->unstable transition continuous.
@@ -154,7 +154,10 @@ static void __clear_sched_clock_stable(struct work_struct *work)* * Still do what we can. */ +preempt_disable (); + scd =this_scd (); gtod_offset = (scd->tick_raw + raw_offset) - (scd->tick_gtod); +preempt_enable (); printk(KERN_INFO "sched_clock: Marking unstable (%lld, %lld)<-(%lld, %lld)\n", scd->tick_gtod, gtod_offset,
Peter Zijlstra
maintainerQuoted 22 lines · Paul E. McKenney
The v4.11-rc1 kernel emits the following splat in some configurations:
[ 43.681891] BUG: using smp_processor_id() in preemptible [00000000] code: kworker/3:1/49 [ 43.682511] caller is debug_smp_processor_id+0x17/0x20 [ 43.682893] CPU: 0 PID: 49 Comm: kworker/3:1 Not tainted 4.11.0-rc1+ #1 [ 43.683382] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 43.683497] Workqueue: events __clear_sched_clock_stable [ 43.683497] Call Trace: [ 43.683497] dump_stack+0x4f/0x69 [ 43.683497] check_preemption_disabled+0xd9/0xf0 [ 43.683497] debug_smp_processor_id+0x17/0x20 [ 43.683497] __clear_sched_clock_stable+0x11/0x60 [ 43.683497] process_one_work+0x146/0x430 [ 43.683497] worker_thread+0x126/0x490 [ 43.683497] kthread+0xfc/0x130 [ 43.683497] ? process_one_work+0x430/0x430 [ 43.683497] ? kthread_create_on_node+0x40/0x40 [ 43.683497] ? umh_complete+0x30/0x30 [ 43.683497] ? call_usermodehelper_exec_async+0x12a/0x130 [ 43.683497] ret_from_fork+0x29/0x40 [ 43.689244] sched_clock: Marking unstable (43688244724, 179505618)<-(43867750342, 0)
Quoted 5 lines
This happens because workqueue handlers run with preemption enabled by default and the new
this_scd()function accesses per-CPU variables. This commit therefore disables preemption across this call tothis_scd()and to the uses of the pointer that it returns. Lightly tested successfully on x86.
Does this also work?
--- kernel/sched/clock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched/clock.c +1 -1
@@ -173,7 +173,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +schedule_work_on (smp_processor_id (), &sched_clock_work); }void sched_clock_init_late (void )
Quoted 23 lines · Peter Zijlstra
Quoted 22 lines · Paul E. McKenney
The v4.11-rc1 kernel emits the following splat in some configurations:
[ 43.681891] BUG: using smp_processor_id() in preemptible [00000000] code: kworker/3:1/49 [ 43.682511] caller is debug_smp_processor_id+0x17/0x20 [ 43.682893] CPU: 0 PID: 49 Comm: kworker/3:1 Not tainted 4.11.0-rc1+ #1 [ 43.683382] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 43.683497] Workqueue: events __clear_sched_clock_stable [ 43.683497] Call Trace: [ 43.683497] dump_stack+0x4f/0x69 [ 43.683497] check_preemption_disabled+0xd9/0xf0 [ 43.683497] debug_smp_processor_id+0x17/0x20 [ 43.683497] __clear_sched_clock_stable+0x11/0x60 [ 43.683497] process_one_work+0x146/0x430 [ 43.683497] worker_thread+0x126/0x490 [ 43.683497] kthread+0xfc/0x130 [ 43.683497] ? process_one_work+0x430/0x430 [ 43.683497] ? kthread_create_on_node+0x40/0x40 [ 43.683497] ? umh_complete+0x30/0x30 [ 43.683497] ? call_usermodehelper_exec_async+0x12a/0x130 [ 43.683497] ret_from_fork+0x29/0x40 [ 43.689244] sched_clock: Marking unstable (43688244724, 179505618)<-(43867750342, 0)
Quoted 7 lines
Quoted 5 lines
This happens because workqueue handlers run with preemption enabled by default and the new
this_scd()function accesses per-CPU variables. This commit therefore disables preemption across this call tothis_scd()and to the uses of the pointer that it returns. Lightly tested successfully on x86.Does this also work?
Thank you! I will give it a shot after the other tests complete.
Quoted 18 lines
--- kernel/sched/clock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/sched/clock.c +1
-1@@ -173,7 +173,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +schedule_work_on (smp_processor_id (), &sched_clock_work); }void sched_clock_init_late (void )
Quoted 30 lines · Paul E. McKenney
Quoted 29 lines · Peter Zijlstra
Quoted 27 lines · Paul E. McKenney
The v4.11-rc1 kernel emits the following splat in some configurations:
[ 43.681891] BUG: using smp_processor_id() in preemptible [00000000] code: kworker/3:1/49 [ 43.682511] caller is debug_smp_processor_id+0x17/0x20 [ 43.682893] CPU: 0 PID: 49 Comm: kworker/3:1 Not tainted 4.11.0-rc1+ #1 [ 43.683382] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 43.683497] Workqueue: events __clear_sched_clock_stable [ 43.683497] Call Trace: [ 43.683497] dump_stack+0x4f/0x69 [ 43.683497] check_preemption_disabled+0xd9/0xf0 [ 43.683497] debug_smp_processor_id+0x17/0x20 [ 43.683497] __clear_sched_clock_stable+0x11/0x60 [ 43.683497] process_one_work+0x146/0x430 [ 43.683497] worker_thread+0x126/0x490 [ 43.683497] kthread+0xfc/0x130 [ 43.683497] ? process_one_work+0x430/0x430 [ 43.683497] ? kthread_create_on_node+0x40/0x40 [ 43.683497] ? umh_complete+0x30/0x30 [ 43.683497] ? call_usermodehelper_exec_async+0x12a/0x130 [ 43.683497] ret_from_fork+0x29/0x40 [ 43.689244] sched_clock: Marking unstable (43688244724, 179505618)<-(43867750342, 0)This happens because workqueue handlers run with preemption enabled by default and the new
this_scd()function accesses per-CPU variables. This commit therefore disables preemption across this call tothis_scd()and to the uses of the pointer that it returns. Lightly tested successfully on x86.
Quoted 3 lines
Quoted 1 line
Does this also work?
Thank you! I will give it a shot after the other tests complete.
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Thanx, Paul
Quoted 18 lines
Quoted 18 lines
--- kernel/sched/clock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/sched/clock.c +1
-1@@ -173,7 +173,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +schedule_work_on (smp_processor_id (), &sched_clock_work); }void sched_clock_init_late (void )
Paul E. McKenney
Tested-byQuoted 31 lines · Paul E. McKenney
Quoted 30 lines · Paul E. McKenney
Quoted 29 lines · Peter Zijlstra
Quoted 27 lines · Paul E. McKenney
The v4.11-rc1 kernel emits the following splat in some configurations:
[ 43.681891] BUG: using smp_processor_id() in preemptible [00000000] code: kworker/3:1/49 [ 43.682511] caller is debug_smp_processor_id+0x17/0x20 [ 43.682893] CPU: 0 PID: 49 Comm: kworker/3:1 Not tainted 4.11.0-rc1+ #1 [ 43.683382] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 43.683497] Workqueue: events __clear_sched_clock_stable [ 43.683497] Call Trace: [ 43.683497] dump_stack+0x4f/0x69 [ 43.683497] check_preemption_disabled+0xd9/0xf0 [ 43.683497] debug_smp_processor_id+0x17/0x20 [ 43.683497] __clear_sched_clock_stable+0x11/0x60 [ 43.683497] process_one_work+0x146/0x430 [ 43.683497] worker_thread+0x126/0x490 [ 43.683497] kthread+0xfc/0x130 [ 43.683497] ? process_one_work+0x430/0x430 [ 43.683497] ? kthread_create_on_node+0x40/0x40 [ 43.683497] ? umh_complete+0x30/0x30 [ 43.683497] ? call_usermodehelper_exec_async+0x12a/0x130 [ 43.683497] ret_from_fork+0x29/0x40 [ 43.689244] sched_clock: Marking unstable (43688244724, 179505618)<-(43867750342, 0)This happens because workqueue handlers run with preemption enabled by default and the new
this_scd()function accesses per-CPU variables. This commit therefore disables preemption across this call tothis_scd()and to the uses of the pointer that it returns. Lightly tested successfully on x86.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Does this also work?
Thank you! I will give it a shot after the other tests complete.
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
And the diffs below passed overnight testing, so:
------------------------------------------------------------------------
commit e87df240b9e6a24635a1c8674f715554f27df6cc
Author: Paul E. McKenney <[email protected]> Date: Thu Mar 9 07:51:25 2017 -0800
EXPERIMENTAL: Peter Zijlstra hotplug fix
kernel/sched/clock.c +2 -2
@@ -172,8 +172,8 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ -if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +if (sched_clock_running ==2 &&sched_clock_stable ()) +schedule_work_on (smp_processor_id (), &sched_clock_work); }void sched_clock_init_late (void )
Peter Zijlstra
maintainerQuoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Some clear_sched_clock_stable() vs hotplug wobbles
Paul reported two independent problems with clear_sched_clock_stable().
- if we tickle it during hotplug (even though the sched_clock was already marked unstable) we'll attempt to schedule_work() and this explodes because RCU isn't watching the new CPU quite yet.
- since we run all of __clear_sched_clock_stable() from workqueue context, there's a preempt problem.
Cure both by only doing the static_branch_disable() from a workqueue, and only when it's still stable.
This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
--- kernel/sched/clock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
kernel/sched/clock.c +12 -5
@@ -141,7 +141,14 @@ static void __set_sched_clock_stable(void)tick_dep_clear (TICK_DEP_BIT_CLOCK_UNSTABLE ); } -static void __clear_sched_clock_stable (struct work_struct *work) +static void __sched_clock_work (struct work_struct *work) +{ +static_branch_disable (&__sched_clock_stable); +} + +static DECLARE_WORK(sched_clock_work, __sched_clock_work); + +static void __clear_sched_clock_stable (void ) {struct sched_clock_data *scd =this_scd ();
@@ -160,11 +167,11 @@ static void __clear_sched_clock_stable(struct work_struct *work) scd->tick_gtod, gtod_offset, scd->tick_raw, raw_offset); -static_branch_disable (&__sched_clock_stable);tick_dep_set (TICK_DEP_BIT_CLOCK_UNSTABLE ); -} -static DECLARE_WORK(sched_clock_work, __clear_sched_clock_stable); +if (sched_clock_stable ()) +schedule_work (&sched_clock_work); +}void clear_sched_clock_stable (void ) {
@@ -173,7 +180,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +__clear_sched_clock_stable (); }void sched_clock_init_late (void )
Paul E. McKenney
Tested-byQuoted 22 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.
Quoted 6 lines
This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Reported-by "Paul E. McKenney" <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>
This passes initial testing. I will hammer it harder overnight, but in the meantime:
Quoted 49 lines
--- kernel/sched/clock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)kernel/sched/clock.c +12
-5@@ -141,7 +141,14 @@ static void __set_sched_clock_stable(void)tick_dep_clear (TICK_DEP_BIT_CLOCK_UNSTABLE ); } -static void __clear_sched_clock_stable (struct work_struct *work) +static void __sched_clock_work (struct work_struct *work) +{ +static_branch_disable (&__sched_clock_stable); +} + +static DECLARE_WORK(sched_clock_work, __sched_clock_work); + +static void __clear_sched_clock_stable (void ) {struct sched_clock_data *scd =this_scd ();@@ -160,11 +167,11 @@ static void __clear_sched_clock_stable(struct work_struct *work) scd->tick_gtod, gtod_offset, scd->tick_raw, raw_offset); -static_branch_disable (&__sched_clock_stable);tick_dep_set (TICK_DEP_BIT_CLOCK_UNSTABLE ); -} -static DECLARE_WORK(sched_clock_work, __clear_sched_clock_stable); +if (sched_clock_stable ()) +schedule_work (&sched_clock_work); +}void clear_sched_clock_stable (void ) {@@ -173,7 +180,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +__clear_sched_clock_stable (); }void sched_clock_init_late (void )
Quoted 27 lines · Paul E. McKenney
Quoted 26 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Quoted 5 lines
Quoted 2 lines
Reported-by "Paul E. McKenney" <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>This passes initial testing. I will hammer it harder overnight, but in the meantime:
And it did just fine overnight, thank you!
Quoted 1 line
Tested-by "Paul E. McKenney" <[email protected]>
I am guessing that you will be pushing this one for -rc3, but please let me know.
Thanx, Paul
Quoted 49 lines
Quoted 49 lines
--- kernel/sched/clock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)kernel/sched/clock.c +12
-5@@ -141,7 +141,14 @@ static void __set_sched_clock_stable(void)tick_dep_clear (TICK_DEP_BIT_CLOCK_UNSTABLE ); } -static void __clear_sched_clock_stable (struct work_struct *work) +static void __sched_clock_work (struct work_struct *work) +{ +static_branch_disable (&__sched_clock_stable); +} + +static DECLARE_WORK(sched_clock_work, __sched_clock_work); + +static void __clear_sched_clock_stable (void ) {struct sched_clock_data *scd =this_scd ();@@ -160,11 +167,11 @@ static void __clear_sched_clock_stable(struct work_struct *work) scd->tick_gtod, gtod_offset, scd->tick_raw, raw_offset); -static_branch_disable (&__sched_clock_stable);tick_dep_set (TICK_DEP_BIT_CLOCK_UNSTABLE ); -} -static DECLARE_WORK(sched_clock_work, __clear_sched_clock_stable); +if (sched_clock_stable ()) +schedule_work (&sched_clock_work); +}void clear_sched_clock_stable (void ) {@@ -173,7 +180,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +__clear_sched_clock_stable (); }void sched_clock_init_late (void )
Quoted 19 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.
Quoted 6 lines
Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Would it help to do the same trick tglx applied to the hot-unplug path, that is IPIing some other CPU to schedule the workqueue?
Thanx, Paul
Quoted 51 lines
Reported-by "Paul E. McKenney" <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/sched/clock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)kernel/sched/clock.c +12
-5@@ -141,7 +141,14 @@ static void __set_sched_clock_stable(void)tick_dep_clear (TICK_DEP_BIT_CLOCK_UNSTABLE ); } -static void __clear_sched_clock_stable (struct work_struct *work) +static void __sched_clock_work (struct work_struct *work) +{ +static_branch_disable (&__sched_clock_stable); +} + +static DECLARE_WORK(sched_clock_work, __sched_clock_work); + +static void __clear_sched_clock_stable (void ) {struct sched_clock_data *scd =this_scd ();@@ -160,11 +167,11 @@ static void __clear_sched_clock_stable(struct work_struct *work) scd->tick_gtod, gtod_offset, scd->tick_raw, raw_offset); -static_branch_disable (&__sched_clock_stable);tick_dep_set (TICK_DEP_BIT_CLOCK_UNSTABLE ); -} -static DECLARE_WORK(sched_clock_work, __clear_sched_clock_stable); +if (sched_clock_stable ()) +schedule_work (&sched_clock_work); +}void clear_sched_clock_stable (void ) {@@ -173,7 +180,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +__clear_sched_clock_stable (); }void sched_clock_init_late (void )
Peter Zijlstra
maintainerQuoted 23 lines · Paul E. McKenney
Quoted 22 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.
Quoted 7 lines
Quoted 3 lines
This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Would it help to do the same trick tglx applied to the hot-unplug path, that is IPIing some other CPU to schedule the workqueue?
So I've been looking again; and I don't think its a problem anymore.
The problem you reported here:
https://lkml.kernel.org/r/[email protected]
Should not happen after commit:
f94c8d116997 ("sched/clock, x86/tsc: Rework the x86 'unstable' sched_clock() interface")
which landed around 4.11-rc2; so _after_ your kernel (which reported itself as -rc1).
Because since that commit we'll never call clear_sched_clock_stable() if tsc_unstable is set.
So I'll have to amend the Changelog somewhat.
Quoted 35 lines · Peter Zijlstra
Quoted 23 lines · Paul E. McKenney
Quoted 22 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.Quoted 6 lines
Quoted 3 lines
This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Would it help to do the same trick tglx applied to the hot-unplug path, that is IPIing some other CPU to schedule the workqueue?
So I've been looking again; and I don't think its a problem anymore.
The problem you reported here:
Quoted 8 lines
https://lkml.kernel.org/r/[email protected]
Should not happen after commit:
f94c8d116997("sched/clock, x86/tsc: Rework the x86 'unstable'sched_clock()interface")which landed around 4.11-rc2; so _after_ your kernel (which reported itself as -rc1).
Very good! I am re-runnning rcutorture on -rc2 (without your patch) and will let you know how it goes.
Quoted 4 lines
Because since that commit we'll never call
clear_sched_clock_stable()iftsc_unstableis set.So I'll have to amend the Changelog somewhat.
Sounds good!
Thanx, Paul
Quoted 46 lines · Paul E. McKenney
Quoted 45 lines · Paul E. McKenney
Quoted 43 lines · Peter Zijlstra
Quoted 23 lines · Paul E. McKenney
Quoted 22 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.Quoted 6 lines
Quoted 3 lines
This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Would it help to do the same trick tglx applied to the hot-unplug path, that is IPIing some other CPU to schedule the workqueue?
So I've been looking again; and I don't think its a problem anymore.
The problem you reported here:
https://lkml.kernel.org/r/[email protected]
Should not happen after commit:
f94c8d116997("sched/clock, x86/tsc: Rework the x86 'unstable'sched_clock()interface")which landed around 4.11-rc2; so _after_ your kernel (which reported itself as -rc1).
Quoted 13 lines
Quoted 2 lines
Very good! I am re-runnning rcutorture on -rc2 (without your patch) and will let you know how it goes.
And -rc2 works just fine on rcutorture, thank you again!
Thanx, Paul
Quoted 6 lines
Quoted 4 lines
Because since that commit we'll never call
clear_sched_clock_stable()iftsc_unstableis set.So I'll have to amend the Changelog somewhat.
Sounds good!
Ah, and I dropped this patch when rebasing to v4.11-rc2 in anticipation of your Changelog amendments:
sched/clock: Some clear_sched_clock_stable() vs hotplug wobbles
Please let me know if you would instead prefer me to keep carrying the current version.
Thanx, Paul
Quoted 40 lines · Paul E. McKenney
Quoted 39 lines · Peter Zijlstra
Quoted 23 lines · Paul E. McKenney
Quoted 22 lines · Peter Zijlstra
Quoted 3 lines · Paul E. McKenney
And it does pass light testing. I will hammer it harder this evening.
So please send a formal patch!
Changed it a bit...
---
Subject: sched/clock: Someclear_sched_clock_stable()vs hotplug wobblesPaul reported two independent problems with
clear_sched_clock_stable().- if we tickle it during hotplug (even though the
sched_clockwas already marked unstable) we'll attempt toschedule_work()and this explodes because RCU isn't watching the new CPU quite yet.- since we run all of
__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Cure both by only doing the
static_branch_disable()from a workqueue, and only when it's still stable.Quoted 6 lines
Quoted 3 lines
This leaves the problem what to do about hotplug actually wrecking TSC though, because if it was stable and now isn't, then we will want to run that work, which then will prod RCU the wrong way. Bloody hotplug.
Would it help to do the same trick tglx applied to the hot-unplug path, that is IPIing some other CPU to schedule the workqueue?
So I've been looking again; and I don't think its a problem anymore.
The problem you reported here:
https://lkml.kernel.org/r/[email protected]
Should not happen after commit:
Quoted 7 lines
Quoted 4 lines
f94c8d116997("sched/clock, x86/tsc: Rework the x86 'unstable'sched_clock()interface")which landed around 4.11-rc2; so _after_ your kernel (which reported itself as -rc1).
Very good! I am re-runnning rcutorture on -rc2 (without your patch) and will let you know how it goes.
And -rc2 works just fine on rcutorture, thank you again!
Thanx, Paul
Quoted 8 lines
Quoted 4 lines
Because since that commit we'll never call
clear_sched_clock_stable()iftsc_unstableis set.So I'll have to amend the Changelog somewhat.
Sounds good!
Thanx, Paul
tip-bot for Peter Zijlstra
merge bot[tip:sched/urgent] sched/clock: Fix clear_sched_clock_stable() preempt wobbly
In reply to Peter Zijlstrasched/clock: Fix clear_sched_clock_stable() preempt wobbly
Commit-ID: 71fdb70eb48784c1f28cdf2e67c4c587dd7f2594 Gitweb: http://git.kernel.org/tip/71fdb70eb48784c1f28cdf2e67c4c587dd7f2594 Author: Peter Zijlstra <[email protected]> AuthorDate: Mon, 13 Mar 2017 13:46:21 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 23 Mar 2017 07:31:48 +0100
sched/clock: Fix clear_sched_clock_stable() preempt wobbly
Paul reported a problems with clear_sched_clock_stable(). Since we run all of __clear_sched_clock_stable() from workqueue context, there's a preempt problem.
Solve it by only running the static_key_disable() from workqueue.
--- kernel/sched/clock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
kernel/sched/clock.c +12 -5
@@ -141,7 +141,14 @@ static void __set_sched_clock_stable(void)tick_dep_clear (TICK_DEP_BIT_CLOCK_UNSTABLE ); } -static void __clear_sched_clock_stable (struct work_struct *work) +static void __sched_clock_work (struct work_struct *work) +{ +static_branch_disable (&__sched_clock_stable); +} + +static DECLARE_WORK(sched_clock_work, __sched_clock_work); + +static void __clear_sched_clock_stable (void ) {struct sched_clock_data *scd =this_scd ();
@@ -160,11 +167,11 @@ static void __clear_sched_clock_stable(struct work_struct *work) scd->tick_gtod, gtod_offset, scd->tick_raw, raw_offset); -static_branch_disable (&__sched_clock_stable);tick_dep_set (TICK_DEP_BIT_CLOCK_UNSTABLE ); -} -static DECLARE_WORK(sched_clock_work, __clear_sched_clock_stable); +if (sched_clock_stable ()) +schedule_work (&sched_clock_work); +}void clear_sched_clock_stable (void ) {
@@ -173,7 +180,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +__clear_sched_clock_stable (); }void sched_clock_init_late (void )
Paul E. McKenney
Tested-byRe: [tip:sched/urgent] sched/clock: Fix clear_sched_clock_stable() preempt wobbly
Quoted 15 lines · tip-bot for Peter Zijlstra
Commit-ID: 71fdb70eb48784c1f28cdf2e67c4c587dd7f2594 Gitweb: http://git.kernel.org/tip/71fdb70eb48784c1f28cdf2e67c4c587dd7f2594 Author: Peter Zijlstra <[email protected]> AuthorDate: Mon, 13 Mar 2017 13:46:21 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 23 Mar 2017 07:31:48 +0100sched/clock: Fix
clear_sched_clock_stable()preempt wobblyPaul reported a problems with
clear_sched_clock_stable(). Since we run all of__clear_sched_clock_stable()from workqueue context, there's a preempt problem.Solve it by only running the
static_key_disable()from workqueue.
Quoted 9 lines
Reported-by Paul E. McKenney <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>Cc Linus Torvalds <[email protected]>Cc Mike Galbraith <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Ingo Molnar <[email protected]>
For whatever it is worth given that it is already in -tip:
Quoted 49 lines
--- kernel/sched/clock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)kernel/sched/clock.c +12
-5@@ -141,7 +141,14 @@ static void __set_sched_clock_stable(void)tick_dep_clear (TICK_DEP_BIT_CLOCK_UNSTABLE ); } -static void __clear_sched_clock_stable (struct work_struct *work) +static void __sched_clock_work (struct work_struct *work) +{ +static_branch_disable (&__sched_clock_stable); +} + +static DECLARE_WORK(sched_clock_work, __sched_clock_work); + +static void __clear_sched_clock_stable (void ) {struct sched_clock_data *scd =this_scd ();@@ -160,11 +167,11 @@ static void __clear_sched_clock_stable(struct work_struct *work) scd->tick_gtod, gtod_offset, scd->tick_raw, raw_offset); -static_branch_disable (&__sched_clock_stable);tick_dep_set (TICK_DEP_BIT_CLOCK_UNSTABLE ); -} -static DECLARE_WORK(sched_clock_work, __clear_sched_clock_stable); +if (sched_clock_stable ()) +schedule_work (&sched_clock_work); +}void clear_sched_clock_stable (void ) {@@ -173,7 +180,7 @@ void clear_sched_clock_stable(void)smp_mb ();/* matches sched_clock_init_late() */ if (sched_clock_running ==2 ) -schedule_work (&sched_clock_work); +__clear_sched_clock_stable (); }void sched_clock_init_late (void )