Skip to content

Commit 6f8d510

Browse files
Maksim Davydovgregkh
Maksim Davydov
authored andcommitted
x86/split_lock: Fix the delayed detection logic
commit c929d08 upstream. 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: 7272093 ("x86/split_lock: Add sysctl to control the misery mode") Signed-off-by: Maksim Davydov <[email protected]> Signed-off-by: Ingo Molnar <[email protected]> Tested-by: Guilherme G. Piccoli <[email protected]> Cc: Thomas Gleixner <[email protected]> Cc: Ravi Bangoria <[email protected]> Cc: Tom Lendacky <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 29f040d commit 6f8d510

File tree

1 file changed

+16
-4
lines changed

1 file changed

+16
-4
lines changed

arch/x86/kernel/cpu/intel.c

+16-4
Original file line numberDiff line numberDiff line change
@@ -1168,7 +1168,13 @@ static void __split_lock_reenable(struct work_struct *work)
11681168
{
11691169
sld_update_msr(true);
11701170
}
1171-
static DECLARE_DELAYED_WORK(sl_reenable, __split_lock_reenable);
1171+
/*
1172+
* In order for each CPU to schedule its delayed work independently of the
1173+
* others, delayed work struct must be per-CPU. This is not required when
1174+
* sysctl_sld_mitigate is enabled because of the semaphore that limits
1175+
* the number of simultaneously scheduled delayed works to 1.
1176+
*/
1177+
static DEFINE_PER_CPU(struct delayed_work, sl_reenable);
11721178

11731179
/*
11741180
* If a CPU goes offline with pending delayed work to re-enable split lock
@@ -1189,7 +1195,7 @@ static int splitlock_cpu_offline(unsigned int cpu)
11891195

11901196
static void split_lock_warn(unsigned long ip)
11911197
{
1192-
struct delayed_work *work;
1198+
struct delayed_work *work = NULL;
11931199
int cpu;
11941200

11951201
if (!current->reported_split_lock)
@@ -1211,11 +1217,17 @@ static void split_lock_warn(unsigned long ip)
12111217
if (down_interruptible(&buslock_sem) == -EINTR)
12121218
return;
12131219
work = &sl_reenable_unlock;
1214-
} else {
1215-
work = &sl_reenable;
12161220
}
12171221

12181222
cpu = get_cpu();
1223+
1224+
if (!work) {
1225+
work = this_cpu_ptr(&sl_reenable);
1226+
/* Deferred initialization of per-CPU struct */
1227+
if (!work->work.func)
1228+
INIT_DELAYED_WORK(work, __split_lock_reenable);
1229+
}
1230+
12191231
schedule_delayed_work_on(cpu, work, 2);
12201232

12211233
/* Disable split lock detection on this CPU to make progress */

0 commit comments

Comments
 (0)