* Re: [PATCH RESEND v4] x86/split_lock: fix delayed detection enabling
2025-01-15 13:17 [PATCH RESEND v4] x86/split_lock: fix delayed detection enabling Maksim Davydov
@ 2025-02-25 13:44 ` Ingo Molnar
2025-03-18 20:29 ` Maksim Davydov
2025-03-06 12:08 ` [tip: locking/core] x86/split_lock: Fix the delayed detection logic tip-bot2 for Maksim Davydov
2025-03-07 12:09 ` tip-bot2 for Maksim Davydov
2 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2025-02-25 13:44 UTC (permalink / raw)
To: Maksim Davydov
Cc: linux-kernel, x86, den-plotnikov, gpiccoli, tglx, mingo, bp,
dave.hansen, hpa
* Maksim Davydov <davydov-max@yandex-team.ru> wrote:
> If the warn mode with disabled mitigation mode is used, then on each
> CPU where the split lock occurred detection will be disabled in order to
> make progress and delayed work will be scheduled, which then will enable
> detection back. Now it turns out that all CPUs use one global delayed
> work structure. This leads to the fact that if a split lock occurs on
> several CPUs at the same time (within 2 jiffies), only one CPU will
> schedule delayed work, but the rest will not. The return value of
> schedule_delayed_work_on() would have shown this, but it is not checked
> in the code.
>
> A diagram that can help to understand the bug reproduction:
> https://lore.kernel.org/all/2cd54041-253b-4e78-b8ea-dbe9b884ff9b@yandex-team.ru/
>
> In order to fix the warn mode with disabled mitigation mode, delayed work
> has to be a per-CPU.
>
> v4 -> v3:
> * rebased the patch onto the latest master
>
> v3 -> v2:
> * place and time of the per-CPU structure initialization were changed.
> initcall doesn't seem to be a good place for it, so deferred
> initialization is used.
>
> Fixes: 727209376f49 ("x86/split_lock: Add sysctl to control the misery mode")
> Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
> arch/x86/kernel/cpu/bus_lock.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
> index 704e9241b964..b72235c8db3e 100644
> --- a/arch/x86/kernel/cpu/bus_lock.c
> +++ b/arch/x86/kernel/cpu/bus_lock.c
> @@ -192,7 +192,13 @@ static void __split_lock_reenable(struct work_struct *work)
> {
> sld_update_msr(true);
> }
> -static DECLARE_DELAYED_WORK(sl_reenable, __split_lock_reenable);
> +/*
> + * In order for each CPU to schedule itself delayed work independently of the
> + * others, delayed work struct should be per-CPU. This is not required when
> + * sysctl_sld_mitigate is enabled because of the semaphore, that limits
> + * the number of simultaneously scheduled delayed works to 1.
> + */
> +static DEFINE_PER_CPU(struct delayed_work, sl_reenable);
>
> /*
> * If a CPU goes offline with pending delayed work to re-enable split lock
> @@ -213,7 +219,7 @@ static int splitlock_cpu_offline(unsigned int cpu)
>
> static void split_lock_warn(unsigned long ip)
> {
> - struct delayed_work *work;
> + struct delayed_work *work = NULL;
> int cpu;
>
> if (!current->reported_split_lock)
> @@ -235,11 +241,17 @@ static void split_lock_warn(unsigned long ip)
> if (down_interruptible(&buslock_sem) == -EINTR)
> return;
> work = &sl_reenable_unlock;
> - } else {
> - work = &sl_reenable;
> }
>
> cpu = get_cpu();
> +
> + if (!work) {
> + work = this_cpu_ptr(&sl_reenable);
> + /* Deferred initialization of per-CPU struct */
> + if (!work->work.func)
> + INIT_DELAYED_WORK(work, __split_lock_reenable);
The INIT_DELAYED_WORK() call shouldn't be done here, but in some sort
of init function. The control flow is complicated enough already.
Also, this_cpu_ptr() isn't necessary, we already have 'cpu' as the
current CPU. per_cpu() ought to be enough.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH RESEND v4] x86/split_lock: fix delayed detection enabling
2025-02-25 13:44 ` Ingo Molnar
@ 2025-03-18 20:29 ` Maksim Davydov
0 siblings, 0 replies; 5+ messages in thread
From: Maksim Davydov @ 2025-03-18 20:29 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, x86, den-plotnikov, gpiccoli, tglx, mingo, bp,
dave.hansen, hpa
On 2/25/25 16:44, Ingo Molnar wrote:
>
> * Maksim Davydov <davydov-max@yandex-team.ru> wrote:
>
>> If the warn mode with disabled mitigation mode is used, then on each
>> CPU where the split lock occurred detection will be disabled in order to
>> make progress and delayed work will be scheduled, which then will enable
>> detection back. Now it turns out that all CPUs use one global delayed
>> work structure. This leads to the fact that if a split lock occurs on
>> several CPUs at the same time (within 2 jiffies), only one CPU will
>> schedule delayed work, but the rest will not. The return value of
>> schedule_delayed_work_on() would have shown this, but it is not checked
>> in the code.
>>
>> A diagram that can help to understand the bug reproduction:
>> https://lore.kernel.org/all/2cd54041-253b-4e78-b8ea-dbe9b884ff9b@yandex-team.ru/
>>
>> In order to fix the warn mode with disabled mitigation mode, delayed work
>> has to be a per-CPU.
>>
>> v4 -> v3:
>> * rebased the patch onto the latest master
>>
>> v3 -> v2:
>> * place and time of the per-CPU structure initialization were changed.
>> initcall doesn't seem to be a good place for it, so deferred
>> initialization is used.
>>
>> Fixes: 727209376f49 ("x86/split_lock: Add sysctl to control the misery mode")
>> Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
>> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> ---
>> arch/x86/kernel/cpu/bus_lock.c | 20 ++++++++++++++++----
>> 1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
>> index 704e9241b964..b72235c8db3e 100644
>> --- a/arch/x86/kernel/cpu/bus_lock.c
>> +++ b/arch/x86/kernel/cpu/bus_lock.c
>> @@ -192,7 +192,13 @@ static void __split_lock_reenable(struct work_struct *work)
>> {
>> sld_update_msr(true);
>> }
>> -static DECLARE_DELAYED_WORK(sl_reenable, __split_lock_reenable);
>> +/*
>> + * In order for each CPU to schedule itself delayed work independently of the
>> + * others, delayed work struct should be per-CPU. This is not required when
>> + * sysctl_sld_mitigate is enabled because of the semaphore, that limits
>> + * the number of simultaneously scheduled delayed works to 1.
>> + */
>> +static DEFINE_PER_CPU(struct delayed_work, sl_reenable);
>>
>> /*
>> * If a CPU goes offline with pending delayed work to re-enable split lock
>> @@ -213,7 +219,7 @@ static int splitlock_cpu_offline(unsigned int cpu)
>>
>> static void split_lock_warn(unsigned long ip)
>> {
>> - struct delayed_work *work;
>> + struct delayed_work *work = NULL;
>> int cpu;
>>
>> if (!current->reported_split_lock)
>> @@ -235,11 +241,17 @@ static void split_lock_warn(unsigned long ip)
>> if (down_interruptible(&buslock_sem) == -EINTR)
>> return;
>> work = &sl_reenable_unlock;
>> - } else {
>> - work = &sl_reenable;
>> }
>>
>> cpu = get_cpu();
>> +
>> + if (!work) {
>> + work = this_cpu_ptr(&sl_reenable);
>> + /* Deferred initialization of per-CPU struct */
>> + if (!work->work.func)
>> + INIT_DELAYED_WORK(work, __split_lock_reenable);
>
> The INIT_DELAYED_WORK() call shouldn't be done here, but in some sort
> of init function. The control flow is complicated enough already.
>
> Also, this_cpu_ptr() isn't necessary, we already have 'cpu' as the
> current CPU. per_cpu() ought to be enough.
>
Sorry for replying late
I fixed these issues in the new version
https://lore.kernel.org/lkml/20250318144911.10455-1-davydov-max@yandex-team.ru/
> Thanks,
>
> Ingo
--
Best regards,
Maksim Davydov
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip: locking/core] x86/split_lock: Fix the delayed detection logic
2025-01-15 13:17 [PATCH RESEND v4] x86/split_lock: fix delayed detection enabling Maksim Davydov
2025-02-25 13:44 ` Ingo Molnar
@ 2025-03-06 12:08 ` tip-bot2 for Maksim Davydov
2025-03-07 12:09 ` tip-bot2 for Maksim Davydov
2 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for Maksim Davydov @ 2025-03-06 12:08 UTC (permalink / raw)
To: linux-tip-commits
Cc: Maksim Davydov, Ingo Molnar, Guilherme G. Piccoli,
Thomas Gleixner, Ravi Bangoria, Tom Lendacky, x86, linux-kernel
The following commit has been merged into the locking/core branch of tip:
Commit-ID: c85391793f4ef47ba5275c57aea7653ead92632f
Gitweb: https://git.kernel.org/tip/c85391793f4ef47ba5275c57aea7653ead92632f
Author: Maksim Davydov <davydov-max@yandex-team.ru>
AuthorDate: Wed, 15 Jan 2025 16:17:04 +03:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 06 Mar 2025 12:59:51 +01:00
x86/split_lock: Fix the delayed detection logic
If the warning mode with disabled mitigation mode is used, then on each
CPU where the split lock occurred detection will be disabled in order to
make progress and delayed work will be scheduled, which then will enable
detection back.
Now it turns out that all CPUs use one global delayed work structure.
This leads to the fact that if a split lock occurs on several CPUs
at the same time (within 2 jiffies), only one CPU will schedule delayed
work, but the rest will not.
The return value of schedule_delayed_work_on() would have shown this,
but it is not checked in the code.
A diagram that can help to understand the bug reproduction:
- sld_update_msr() enables/disables SLD on both CPUs on the same core
- schedule_delayed_work_on() internally checks WORK_STRUCT_PENDING_BIT.
If a work has the 'pending' status, then schedule_delayed_work_on()
will return an error code and, most importantly, the work will not
be placed in the workqueue.
Let's say we have a multicore system on which split_lock_mitigate=0 and
a multithreaded application is running that calls splitlock in multiple
threads. Due to the fact that sld_update_msr() affects the entire core
(both CPUs), we will consider 2 CPUs from different cores. Let the 2
threads of this application schedule to CPU0 (core 0) and to CPU 2
(core 1), then:
| || |
| CPU 0 (core 0) || CPU 2 (core 1) |
|_________________________________||___________________________________|
| || |
| 1) SPLIT LOCK occured || |
| || |
| 2) split_lock_warn() || |
| || |
| 3) sysctl_sld_mitigate == 0 || |
| (work = &sl_reenable) || |
| || |
| 4) schedule_delayed_work_on() || |
| (reenable will be called || |
| after 2 jiffies on CPU 0) || |
| || |
| 5) disable SLD for core 0 || |
| || |
| ------------------------- || |
| || |
| || 6) SPLIT LOCK occured |
| || |
| || 7) split_lock_warn() |
| || |
| || 8) sysctl_sld_mitigate == 0 |
| || (work = &sl_reenable, |
| || the same address as in 3) ) |
| || |
| 2 jiffies || 9) schedule_delayed_work_on() |
| || fials because the work is in |
| || the pending state since 4). |
| || The work wasn't placed to the |
| || workqueue. reenable won't be |
| || called on CPU 2 |
| || |
| || 10) disable SLD for core 0 |
| || |
| || From now on SLD will |
| || never be reenabled on core 1 |
| || |
| ------------------------- || |
| || |
| 11) enable SLD for core 0 by || |
| __split_lock_reenable || |
| || |
If the application threads can be scheduled to all processor cores,
then over time there will be only one core left, on which SLD will be
enabled and split lock will be able to be detected; and on all other
cores SLD will be disabled all the time.
Most likely, this bug has not been noticed for so long because
sysctl_sld_mitigate default value is 1, and in this case a semaphore
is used that does not allow 2 different cores to have SLD disabled at
the same time, that is, strictly only one work is placed in the
workqueue.
In order to fix the warning mode with disabled mitigation mode,
delayed work has to be per-CPU. Implement it.
Fixes: 727209376f49 ("x86/split_lock: Add sysctl to control the misery mode")
Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Link: https://lore.kernel.org/r/20250115131704.132609-1-davydov-max@yandex-team.ru
---
arch/x86/kernel/cpu/bus_lock.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index 6cba85c..97222ef 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -192,7 +192,13 @@ static void __split_lock_reenable(struct work_struct *work)
{
sld_update_msr(true);
}
-static DECLARE_DELAYED_WORK(sl_reenable, __split_lock_reenable);
+/*
+ * In order for each CPU to schedule its delayed work independently of the
+ * others, delayed work struct must be per-CPU. This is not required when
+ * sysctl_sld_mitigate is enabled because of the semaphore that limits
+ * the number of simultaneously scheduled delayed works to 1.
+ */
+static DEFINE_PER_CPU(struct delayed_work, sl_reenable);
/*
* If a CPU goes offline with pending delayed work to re-enable split lock
@@ -213,7 +219,7 @@ static int splitlock_cpu_offline(unsigned int cpu)
static void split_lock_warn(unsigned long ip)
{
- struct delayed_work *work;
+ struct delayed_work *work = NULL;
int cpu;
if (!current->reported_split_lock)
@@ -235,11 +241,17 @@ static void split_lock_warn(unsigned long ip)
if (down_interruptible(&buslock_sem) == -EINTR)
return;
work = &sl_reenable_unlock;
- } else {
- work = &sl_reenable;
}
cpu = get_cpu();
+
+ if (!work) {
+ work = this_cpu_ptr(&sl_reenable);
+ /* Deferred initialization of per-CPU struct */
+ if (!work->work.func)
+ INIT_DELAYED_WORK(work, __split_lock_reenable);
+ }
+
schedule_delayed_work_on(cpu, work, 2);
/* Disable split lock detection on this CPU to make progress */
^ permalink raw reply related [flat|nested] 5+ messages in thread* [tip: locking/core] x86/split_lock: Fix the delayed detection logic
2025-01-15 13:17 [PATCH RESEND v4] x86/split_lock: fix delayed detection enabling Maksim Davydov
2025-02-25 13:44 ` Ingo Molnar
2025-03-06 12:08 ` [tip: locking/core] x86/split_lock: Fix the delayed detection logic tip-bot2 for Maksim Davydov
@ 2025-03-07 12:09 ` tip-bot2 for Maksim Davydov
2 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for Maksim Davydov @ 2025-03-07 12:09 UTC (permalink / raw)
To: linux-tip-commits
Cc: Maksim Davydov, Ingo Molnar, Guilherme G. Piccoli,
Thomas Gleixner, Ravi Bangoria, Tom Lendacky, x86, linux-kernel
The following commit has been merged into the locking/core branch of tip:
Commit-ID: c929d08df8bee855528b9d15b853c892c54e1eee
Gitweb: https://git.kernel.org/tip/c929d08df8bee855528b9d15b853c892c54e1eee
Author: Maksim Davydov <davydov-max@yandex-team.ru>
AuthorDate: Wed, 15 Jan 2025 16:17:04 +03:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 07 Mar 2025 13:01:02 +01:00
x86/split_lock: Fix the delayed detection logic
If the warning mode with disabled mitigation mode is used, then on each
CPU where the split lock occurred detection will be disabled in order to
make progress and delayed work will be scheduled, which then will enable
detection back.
Now it turns out that all CPUs use one global delayed work structure.
This leads to the fact that if a split lock occurs on several CPUs
at the same time (within 2 jiffies), only one CPU will schedule delayed
work, but the rest will not.
The return value of schedule_delayed_work_on() would have shown this,
but it is not checked in the code.
A diagram that can help to understand the bug reproduction:
- sld_update_msr() enables/disables SLD on both CPUs on the same core
- schedule_delayed_work_on() internally checks WORK_STRUCT_PENDING_BIT.
If a work has the 'pending' status, then schedule_delayed_work_on()
will return an error code and, most importantly, the work will not
be placed in the workqueue.
Let's say we have a multicore system on which split_lock_mitigate=0 and
a multithreaded application is running that calls splitlock in multiple
threads. Due to the fact that sld_update_msr() affects the entire core
(both CPUs), we will consider 2 CPUs from different cores. Let the 2
threads of this application schedule to CPU0 (core 0) and to CPU 2
(core 1), then:
| || |
| CPU 0 (core 0) || CPU 2 (core 1) |
|_________________________________||___________________________________|
| || |
| 1) SPLIT LOCK occured || |
| || |
| 2) split_lock_warn() || |
| || |
| 3) sysctl_sld_mitigate == 0 || |
| (work = &sl_reenable) || |
| || |
| 4) schedule_delayed_work_on() || |
| (reenable will be called || |
| after 2 jiffies on CPU 0) || |
| || |
| 5) disable SLD for core 0 || |
| || |
| ------------------------- || |
| || |
| || 6) SPLIT LOCK occured |
| || |
| || 7) split_lock_warn() |
| || |
| || 8) sysctl_sld_mitigate == 0 |
| || (work = &sl_reenable, |
| || the same address as in 3) ) |
| || |
| 2 jiffies || 9) schedule_delayed_work_on() |
| || fials because the work is in |
| || the pending state since 4). |
| || The work wasn't placed to the |
| || workqueue. reenable won't be |
| || called on CPU 2 |
| || |
| || 10) disable SLD for core 0 |
| || |
| || From now on SLD will |
| || never be reenabled on core 1 |
| || |
| ------------------------- || |
| || |
| 11) enable SLD for core 0 by || |
| __split_lock_reenable || |
| || |
If the application threads can be scheduled to all processor cores,
then over time there will be only one core left, on which SLD will be
enabled and split lock will be able to be detected; and on all other
cores SLD will be disabled all the time.
Most likely, this bug has not been noticed for so long because
sysctl_sld_mitigate default value is 1, and in this case a semaphore
is used that does not allow 2 different cores to have SLD disabled at
the same time, that is, strictly only one work is placed in the
workqueue.
In order to fix the warning mode with disabled mitigation mode,
delayed work has to be per-CPU. Implement it.
Fixes: 727209376f49 ("x86/split_lock: Add sysctl to control the misery mode")
Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Link: https://lore.kernel.org/r/20250115131704.132609-1-davydov-max@yandex-team.ru
---
arch/x86/kernel/cpu/bus_lock.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
index 6cba85c..97222ef 100644
--- a/arch/x86/kernel/cpu/bus_lock.c
+++ b/arch/x86/kernel/cpu/bus_lock.c
@@ -192,7 +192,13 @@ static void __split_lock_reenable(struct work_struct *work)
{
sld_update_msr(true);
}
-static DECLARE_DELAYED_WORK(sl_reenable, __split_lock_reenable);
+/*
+ * In order for each CPU to schedule its delayed work independently of the
+ * others, delayed work struct must be per-CPU. This is not required when
+ * sysctl_sld_mitigate is enabled because of the semaphore that limits
+ * the number of simultaneously scheduled delayed works to 1.
+ */
+static DEFINE_PER_CPU(struct delayed_work, sl_reenable);
/*
* If a CPU goes offline with pending delayed work to re-enable split lock
@@ -213,7 +219,7 @@ static int splitlock_cpu_offline(unsigned int cpu)
static void split_lock_warn(unsigned long ip)
{
- struct delayed_work *work;
+ struct delayed_work *work = NULL;
int cpu;
if (!current->reported_split_lock)
@@ -235,11 +241,17 @@ static void split_lock_warn(unsigned long ip)
if (down_interruptible(&buslock_sem) == -EINTR)
return;
work = &sl_reenable_unlock;
- } else {
- work = &sl_reenable;
}
cpu = get_cpu();
+
+ if (!work) {
+ work = this_cpu_ptr(&sl_reenable);
+ /* Deferred initialization of per-CPU struct */
+ if (!work->work.func)
+ INIT_DELAYED_WORK(work, __split_lock_reenable);
+ }
+
schedule_delayed_work_on(cpu, work, 2);
/* Disable split lock detection on this CPU to make progress */
^ permalink raw reply related [flat|nested] 5+ messages in thread