Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
warning.
[ 0.000000] lockdep_assert_cpus_held+0x50/0x60
[ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
[ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
[ 0.000000] arch_timer_acpi_init+0x274/0x6ac
[ 0.000000] acpi_table_parse+0x1ac/0x218
[ 0.000000] __acpi_probe_device_table+0x164/0x1ec
[ 0.000000] timer_probe+0x1bc/0x254
[ 0.000000] time_init+0x44/0x98
[ 0.000000] start_kernel+0x4ec/0x7d4
This is due to the commit cb538267ea1e ("jump_label/lockdep: Assert we hold
the hotplug lock for _cpuslocked() operations").
Since it is applying a global workaround to all CPUs here, it did not hold
any CPU locks in this path.
arch_timer_acpi_init
arch_timer_check_ool_workaround(ate_match_acpi_oem_info, table)
arch_timer_enable_workaround(wa, local = false)
for_each_possible_cpu()
per_cpu()
When hot-add a CPU, it will go with a slightly different route.
arch_timer_starting_cpu
__arch_timer_setup
arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL)
arch_timer_enable_workaround(wa, local = true)
__this_cpu_write()
Hence, deal with them differently.
Fixes: 450f9689f294 (clocksource/arm_arch_timer: Use
static_branch_enable_cpuslocked())
Signed-off-by: Qian Cai <[email protected]>
---
Changes since v1:
* Fixed the root cause instead of a workaround.
drivers/clocksource/arm_arch_timer.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 9a7d4dc00b6e..81dca7d31d13 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -492,18 +492,21 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
if (local) {
__this_cpu_write(timer_unstable_counter_workaround, wa);
+
+ /*
+ * Use the locked version, as we're called from the CPU
+ * hotplug framework. Otherwise, we end-up in
+ * deadlock-land.
+ */
+ static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
} else {
for_each_possible_cpu(i)
per_cpu(timer_unstable_counter_workaround, i) = wa;
+ /* A global workaround is not on the CPU hotplug path. */
+ static_branch_enable(&arch_timer_read_ool_enabled);
}
/*
- * Use the locked version, as we're called from the CPU
- * hotplug framework. Otherwise, we end-up in deadlock-land.
- */
- static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
-
- /*
* Don't use the vdso fastpath if errata require using the
* out-of-line counter accessor. We may change our mind pretty
* late in the game (with a per-CPU erratum, for example), so
--
1.8.3.1
On Mon, Dec 10, 2018 at 08:52:28AM -0500, Qian Cai wrote:
> Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
> warning.
>
> [ 0.000000] lockdep_assert_cpus_held+0x50/0x60
> [ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
> [ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
> [ 0.000000] arch_timer_acpi_init+0x274/0x6ac
> [ 0.000000] acpi_table_parse+0x1ac/0x218
> [ 0.000000] __acpi_probe_device_table+0x164/0x1ec
> [ 0.000000] timer_probe+0x1bc/0x254
> [ 0.000000] time_init+0x44/0x98
> [ 0.000000] start_kernel+0x4ec/0x7d4
It seems to me we want something like:
---
kernel/cpu.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 91d5c38eb7e5..e1ee8caf28b5 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -313,6 +313,8 @@ void cpus_write_unlock(void)
void lockdep_assert_cpus_held(void)
{
+ if (system_state < SYSTEM_RUNNING)
+ return;
percpu_rwsem_assert_held(&cpu_hotplug_lock);
}
Hi,
On 10/12/2018 14:07, Peter Zijlstra wrote:
[...]
> ---
> kernel/cpu.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 91d5c38eb7e5..e1ee8caf28b5 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -313,6 +313,8 @@ void cpus_write_unlock(void)
>
> void lockdep_assert_cpus_held(void)
> {
> + if (system_state < SYSTEM_RUNNING)
> + return;
> percpu_rwsem_assert_held(&cpu_hotplug_lock);
> }
>
Kinda hijacking the thread here - is that something we'd want to replace
40fa3780bac2 ("sched/core: Take the hotplug lock in sched_init_smp()")
with?
Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
warning.
[ 0.000000] lockdep_assert_cpus_held+0x50/0x60
[ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
[ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
[ 0.000000] arch_timer_acpi_init+0x274/0x6ac
[ 0.000000] acpi_table_parse+0x1ac/0x218
[ 0.000000] __acpi_probe_device_table+0x164/0x1ec
[ 0.000000] timer_probe+0x1bc/0x254
[ 0.000000] time_init+0x44/0x98
[ 0.000000] start_kernel+0x4ec/0x7d4
This is due to the commit cb538267ea1e ("jump_label/lockdep: Assert we hold
the hotplug lock for _cpuslocked() operations").
Since it is applying a global workaround to all CPUs here, it did not hold
any CPU locks in this path.
arch_timer_acpi_init
arch_timer_check_ool_workaround(ate_match_acpi_oem_info, table)
arch_timer_enable_workaround(wa, local = false)
for_each_possible_cpu()
per_cpu()
There is also another path did not have any CPU lock.
time_init
clocksource_probe
arch_timer_of_init
arch_timer_check_ool_workaround(ate_match_dt, np)
arch_timer_enable_workaround(wa, local = false)
When hot-adding a CPU, it will go with a slightly different route.
arch_timer_starting_cpu
__arch_timer_setup
arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL)
arch_timer_enable_workaround(wa, local = true)
__this_cpu_write()
Hence, deal with them differently.
Fixes: 450f9689f294 (clocksource/arm_arch_timer: Use
static_branch_enable_cpuslocked())
Signed-off-by: Qian Cai <[email protected]>
---
v2: fix the root cause instead of a workaround.
drivers/clocksource/arm_arch_timer.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 9a7d4dc00b6e..81dca7d31d13 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -492,17 +492,20 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
if (local) {
__this_cpu_write(timer_unstable_counter_workaround, wa);
+
+ /*
+ * Use the locked version, as we're called from the CPU
+ * hotplug framework. Otherwise, we end-up in
+ * deadlock-land.
+ */
+ static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
} else {
for_each_possible_cpu(i)
per_cpu(timer_unstable_counter_workaround, i) = wa;
+ /* A global workaround is not on the CPU hotplug path. */
+ static_branch_enable(&arch_timer_read_ool_enabled);
}
- /*
- * Use the locked version, as we're called from the CPU
- * hotplug framework. Otherwise, we end-up in deadlock-land.
- */
- static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
-
/*
* Don't use the vdso fastpath if errata require using the
* out-of-line counter accessor. We may change our mind pretty
--
2.17.2 (Apple Git-113)
On Mon, Dec 10, 2018 at 02:19:53PM +0000, Valentin Schneider wrote:
> Hi,
>
> On 10/12/2018 14:07, Peter Zijlstra wrote:
> [...]
> > ---
> > kernel/cpu.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index 91d5c38eb7e5..e1ee8caf28b5 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -313,6 +313,8 @@ void cpus_write_unlock(void)
> >
> > void lockdep_assert_cpus_held(void)
> > {
> > + if (system_state < SYSTEM_RUNNING)
> > + return;
> > percpu_rwsem_assert_held(&cpu_hotplug_lock);
> > }
> >
>
> Kinda hijacking the thread here - is that something we'd want to replace
>
> 40fa3780bac2 ("sched/core: Take the hotplug lock in sched_init_smp()")
>
> with?
>
Possibly; and I have vague memories of other people running into this
same thing too.
On 10/12/2018 13:52, Qian Cai wrote:
> Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
> warning.
>
> [ 0.000000] lockdep_assert_cpus_held+0x50/0x60
> [ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
> [ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
> [ 0.000000] arch_timer_acpi_init+0x274/0x6ac
> [ 0.000000] acpi_table_parse+0x1ac/0x218
> [ 0.000000] __acpi_probe_device_table+0x164/0x1ec
> [ 0.000000] timer_probe+0x1bc/0x254
> [ 0.000000] time_init+0x44/0x98
> [ 0.000000] start_kernel+0x4ec/0x7d4
>
> This is due to the commit cb538267ea1e ("jump_label/lockdep: Assert we hold
> the hotplug lock for _cpuslocked() operations").
>
> Since it is applying a global workaround to all CPUs here, it did not hold
> any CPU locks in this path.
>
> arch_timer_acpi_init
> arch_timer_check_ool_workaround(ate_match_acpi_oem_info, table)
> arch_timer_enable_workaround(wa, local = false)
> for_each_possible_cpu()
> per_cpu()
>
> There is also another path did not have any CPU lock.
>
> time_init
> clocksource_probe
> arch_timer_of_init
> arch_timer_check_ool_workaround(ate_match_dt, np)
> arch_timer_enable_workaround(wa, local = false)
>
> When hot-adding a CPU, it will go with a slightly different route.
>
> arch_timer_starting_cpu
> __arch_timer_setup
> arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL)
> arch_timer_enable_workaround(wa, local = true)
> __this_cpu_write()
>
> Hence, deal with them differently.
>
> Fixes: 450f9689f294 (clocksource/arm_arch_timer: Use
> static_branch_enable_cpuslocked())
> Signed-off-by: Qian Cai <[email protected]>
> ---
>
> v2: fix the root cause instead of a workaround.
>
> drivers/clocksource/arm_arch_timer.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 9a7d4dc00b6e..81dca7d31d13 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -492,17 +492,20 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
>
> if (local) {
> __this_cpu_write(timer_unstable_counter_workaround, wa);
> +
> + /*
> + * Use the locked version, as we're called from the CPU
> + * hotplug framework. Otherwise, we end-up in
> + * deadlock-land.
> + */
> + static_branch_enable_cpuslocked(&arch_timer_read_ool_enabled);
I have the ugly feeling that it breaks the (equally ugly) big-little
stuff where the boot CPU is affected. In this context, you'd end-up
without the lock being taken and you'll get the same splat.
We could start testing the CPU number, but Peter's approach seem much
more palatable to me.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 12/10/18 9:07 AM, Peter Zijlstra wrote:
> On Mon, Dec 10, 2018 at 08:52:28AM -0500, Qian Cai wrote:
>> Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
>> warning.
>>
>> [ 0.000000] lockdep_assert_cpus_held+0x50/0x60
>> [ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
>> [ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
>> [ 0.000000] arch_timer_acpi_init+0x274/0x6ac
>> [ 0.000000] acpi_table_parse+0x1ac/0x218
>> [ 0.000000] __acpi_probe_device_table+0x164/0x1ec
>> [ 0.000000] timer_probe+0x1bc/0x254
>> [ 0.000000] time_init+0x44/0x98
>> [ 0.000000] start_kernel+0x4ec/0x7d4
>
> It seems to me we want something like:
Tested-by: Qian Cai <[email protected]>
>
> ---
> kernel/cpu.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 91d5c38eb7e5..e1ee8caf28b5 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -313,6 +313,8 @@ void cpus_write_unlock(void)
>
> void lockdep_assert_cpus_held(void)
> {
> + if (system_state < SYSTEM_RUNNING)
> + return;
> percpu_rwsem_assert_held(&cpu_hotplug_lock);
> }
>
>
On Mon, 10 Dec 2018, Peter Zijlstra wrote:
> On Mon, Dec 10, 2018 at 08:52:28AM -0500, Qian Cai wrote:
> > Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
> > warning.
> >
> > [ 0.000000] lockdep_assert_cpus_held+0x50/0x60
> > [ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
> > [ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
> > [ 0.000000] arch_timer_acpi_init+0x274/0x6ac
> > [ 0.000000] acpi_table_parse+0x1ac/0x218
> > [ 0.000000] __acpi_probe_device_table+0x164/0x1ec
> > [ 0.000000] timer_probe+0x1bc/0x254
> > [ 0.000000] time_init+0x44/0x98
> > [ 0.000000] start_kernel+0x4ec/0x7d4
>
> It seems to me we want something like:
>
> ---
> kernel/cpu.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 91d5c38eb7e5..e1ee8caf28b5 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -313,6 +313,8 @@ void cpus_write_unlock(void)
>
> void lockdep_assert_cpus_held(void)
> {
> + if (system_state < SYSTEM_RUNNING)
> + return;
> percpu_rwsem_assert_held(&cpu_hotplug_lock);
> }
Hmm, no. SYSTEM_SCHEDULING is what you want because RUNNING is set way too
late.
Thanks,
tglx
On 12/10/18 4:31 PM, Thomas Gleixner wrote:
> On Mon, 10 Dec 2018, Peter Zijlstra wrote:
>> On Mon, Dec 10, 2018 at 08:52:28AM -0500, Qian Cai wrote:
>>> Booting this Huawei TaiShan 2280 arm64 server generated this lockdep
>>> warning.
>>>
>>> [ 0.000000] lockdep_assert_cpus_held+0x50/0x60
>>> [ 0.000000] static_key_enable_cpuslocked+0x30/0xe8
>>> [ 0.000000] arch_timer_check_ool_workaround+0x128/0x2d0
>>> [ 0.000000] arch_timer_acpi_init+0x274/0x6ac
>>> [ 0.000000] acpi_table_parse+0x1ac/0x218
>>> [ 0.000000] __acpi_probe_device_table+0x164/0x1ec
>>> [ 0.000000] timer_probe+0x1bc/0x254
>>> [ 0.000000] time_init+0x44/0x98
>>> [ 0.000000] start_kernel+0x4ec/0x7d4
>>
>> It seems to me we want something like:
>>
>> ---
>> kernel/cpu.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/cpu.c b/kernel/cpu.c
>> index 91d5c38eb7e5..e1ee8caf28b5 100644
>> --- a/kernel/cpu.c
>> +++ b/kernel/cpu.c
>> @@ -313,6 +313,8 @@ void cpus_write_unlock(void)
>>
>> void lockdep_assert_cpus_held(void)
>> {
>> + if (system_state < SYSTEM_RUNNING)
>> + return;
>> percpu_rwsem_assert_held(&cpu_hotplug_lock);
>> }
>
> Hmm, no. SYSTEM_SCHEDULING is what you want because RUNNING is set way too
> late.
SYSTEM_SCHEDULING works well here too.