On Fri, Mar 06, 2026 at 10:57:15AM +0100, Thomas Gleixner wrote:
> I tried with tons of test cases which stress test mmcid with threads and
> failed.
Are some of those in tools/testing/selftests ?
Anyway, I was going over that code, and I noticed that there seems to be
inconsistent locking for mm_mm_cid::pcpu.
There's a bunch of sites that state we need rq->lock for remote access;
but then things like sched_mm_cid_fork() and sched_mm_cid_exit() seem to
think that holding mm_cid->lock is sufficient.
This doesn't make sense to me, but maybe I missed something.
Anyway, I cobbled together the below, and that builds and boots and
passes everything in tools/testing/selftests/rseq.
YMMV
---
diff --git a/include/linux/rseq_types.h b/include/linux/rseq_types.h
index da5fa6f40294..df2b4629cbfd 100644
--- a/include/linux/rseq_types.h
+++ b/include/linux/rseq_types.h
@@ -2,9 +2,12 @@
#ifndef _LINUX_RSEQ_TYPES_H
#define _LINUX_RSEQ_TYPES_H
+#include <linux/compiler_types.h>
#include <linux/irq_work_types.h>
#include <linux/types.h>
#include <linux/workqueue_types.h>
+#include <linux/mutex.h>
+#include <asm/percpu.h>
#ifdef CONFIG_RSEQ
struct rseq;
@@ -145,8 +148,14 @@ struct sched_mm_cid {
* while a task with a CID is running
*/
struct mm_cid_pcpu {
- unsigned int cid;
-}____cacheline_aligned_in_smp;
+ unsigned int cid;
+} ____cacheline_aligned_in_smp;
+
+/*
+ * See helpers in kernel/sched/sched.h that convert
+ * from __rq_lockp(rq) to RQ_LOCK.
+ */
+token_context_lock(RQ_LOCK);
/**
* struct mm_mm_cid - Storage for per MM CID data
@@ -167,7 +176,7 @@ struct mm_cid_pcpu {
*/
struct mm_mm_cid {
/* Hotpath read mostly members */
- struct mm_cid_pcpu __percpu *pcpu;
+ struct mm_cid_pcpu __percpu *pcpu __guarded_by(RQ_LOCK);
unsigned int mode;
unsigned int max_cids;
@@ -179,11 +188,11 @@ struct mm_mm_cid {
struct mutex mutex;
/* Low frequency modified */
- unsigned int nr_cpus_allowed;
- unsigned int users;
- unsigned int pcpu_thrs;
- unsigned int update_deferred;
-}____cacheline_aligned_in_smp;
+ unsigned int nr_cpus_allowed __guarded_by(&lock);
+ unsigned int users __guarded_by(&lock);
+ unsigned int pcpu_thrs __guarded_by(&lock);
+ unsigned int update_deferred __guarded_by(&lock);
+} ____cacheline_aligned_in_smp;
#else /* CONFIG_SCHED_MM_CID */
struct mm_mm_cid { };
struct sched_mm_cid { };
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2b571e640372..f7c03c9c4fd0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5335,7 +5335,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
}
}
- mm_cid_switch_to(prev, next);
+ mm_cid_switch_to(rq, prev, next);
/*
* Tell rseq that the task was scheduled in. Must be after
@@ -10511,6 +10511,7 @@ void call_trace_sched_update_nr_running(struct rq *rq,
int count)
* fork(), exit() and affinity changes
*/
static void __mm_update_max_cids(struct mm_mm_cid *mc)
+ __must_hold(&mc->lock)
{
unsigned int opt_cids, max_cids;
@@ -10523,15 +10524,17 @@ static void __mm_update_max_cids(struct mm_mm_cid *mc)
}
static inline unsigned int mm_cid_calc_pcpu_thrs(struct mm_mm_cid *mc)
+ __must_hold(&mc->lock)
{
unsigned int opt_cids;
opt_cids = min(mc->nr_cpus_allowed, mc->users);
/* Has to be at least 1 because 0 indicates PCPU mode off */
- return max(min(opt_cids - opt_cids / 4, num_possible_cpus() / 2), 1);
+ return max(min(opt_cids - (opt_cids / 4), num_possible_cpus() / 2), 1);
}
static bool mm_update_max_cids(struct mm_struct *mm)
+ __must_hold(&mm->mm_cid.lock)
{
struct mm_mm_cid *mc = &mm->mm_cid;
bool percpu = cid_on_cpu(mc->mode);
@@ -10558,6 +10561,7 @@ static bool mm_update_max_cids(struct mm_struct *mm)
return false;
/* Flip the mode and set the transition flag to bridge the transfer */
+ WARN_ON_ONCE(mc->mode & MM_CID_TRANSIT);
WRITE_ONCE(mc->mode, mc->mode ^ (MM_CID_TRANSIT | MM_CID_ONCPU));
/*
* Order the store against the subsequent fixups so that
@@ -10568,16 +10572,28 @@ static bool mm_update_max_cids(struct mm_struct *mm)
return true;
}
+/*
+ * Silly helper because we cannot express that mm_mm_cid::users is updated
+ * while holding both mutex and lock and can thus be read while holding
+ * either.
+ */
+static __always_inline unsigned int mm_cid_users(struct mm_struct *mm)
+ __must_hold(&mm->mm_cid.mutex)
+{
+ __assume_ctx_lock(&mm->mm_cid.lock);
+ return mm->mm_cid.users;
+}
+
static inline void mm_update_cpus_allowed(struct mm_struct *mm, const struct
cpumask *affmsk)
{
struct cpumask *mm_allowed;
struct mm_mm_cid *mc;
unsigned int weight;
- if (!mm || !READ_ONCE(mm->mm_cid.users))
+ if (!mm || !data_race(READ_ONCE(mm->mm_cid.users)))
return;
/*
- * mm::mm_cid::mm_cpus_allowed is the superset of each threads
+ * mm::mm_cid::mm_cpus_allowed is the superset of each thread's
* allowed CPUs mask which means it can only grow.
*/
mc = &mm->mm_cid;
@@ -10609,6 +10625,7 @@ static inline void mm_update_cpus_allowed(struct
mm_struct *mm, const struct cpu
static inline void mm_cid_complete_transit(struct mm_struct *mm, unsigned int
mode)
{
+ WARN_ON_ONCE(!(mm->mm_cid.mode & MM_CID_TRANSIT));
/*
* Ensure that the store removing the TRANSIT bit cannot be
* reordered by the CPU before the fixups have been completed.
@@ -10633,11 +10650,12 @@ static void mm_cid_fixup_cpus_to_tasks(struct
mm_struct *mm)
/* Walk the CPUs and fixup all stale CIDs */
for_each_possible_cpu(cpu) {
- struct mm_cid_pcpu *pcp = per_cpu_ptr(mm->mm_cid.pcpu, cpu);
struct rq *rq = cpu_rq(cpu);
+ struct mm_cid_pcpu *pcp;
/* Remote access to mm::mm_cid::pcpu requires rq_lock */
guard(rq_lock_irq)(rq);
+ pcp = mm_cid_pcpu(&mm->mm_cid, rq);
/* Is the CID still owned by the CPU? */
if (cid_on_cpu(pcp->cid)) {
/*
@@ -10675,6 +10693,7 @@ static bool mm_cid_fixup_task_to_cpu(struct task_struct
*t, struct mm_struct *mm
{
/* Remote access to mm::mm_cid::pcpu requires rq_lock */
guard(task_rq_lock)(t);
+ __assume_ctx_lock(RQ_LOCK);
/* If the task is not active it is not in the users count */
if (!t->mm_cid.active)
return false;
@@ -10689,6 +10708,7 @@ static bool mm_cid_fixup_task_to_cpu(struct task_struct
*t, struct mm_struct *mm
}
static void mm_cid_do_fixup_tasks_to_cpus(struct mm_struct *mm)
+ __must_hold(&mm->mm_cid.mutex)
{
struct task_struct *p, *t;
unsigned int users;
@@ -10703,7 +10723,7 @@ static void mm_cid_do_fixup_tasks_to_cpus(struct
mm_struct *mm)
* The caller has already transferred. The newly incoming task is
* already accounted for, but not yet visible.
*/
- users = mm->mm_cid.users - 2;
+ users = mm_cid_users(mm) - 2;
if (!users)
return;
@@ -10727,18 +10747,19 @@ static void mm_cid_do_fixup_tasks_to_cpus(struct
mm_struct *mm)
}
}
-static void mm_cid_fixup_tasks_to_cpus(void)
+static void mm_cid_fixup_tasks_to_cpus(struct mm_struct *mm)
+ __must_hold(mm->mm_cid.mutex)
{
- struct mm_struct *mm = current->mm;
-
mm_cid_do_fixup_tasks_to_cpus(mm);
mm_cid_complete_transit(mm, MM_CID_ONCPU);
}
static bool sched_mm_cid_add_user(struct task_struct *t, struct mm_struct *mm)
+ __must_hold(&mm->mm_cid.mutex)
+ __must_hold(&mm->mm_cid.lock)
{
t->mm_cid.active = 1;
- mm->mm_cid.users++;
+ mm->mm_cid.users++; /* mutex && lock */
return mm_update_max_cids(mm);
}
@@ -10750,8 +10771,9 @@ void sched_mm_cid_fork(struct task_struct *t)
WARN_ON_ONCE(!mm || t->mm_cid.cid != MM_CID_UNSET);
guard(mutex)(&mm->mm_cid.mutex);
- scoped_guard(raw_spinlock_irq, &mm->mm_cid.lock) {
- struct mm_cid_pcpu *pcp = this_cpu_ptr(mm->mm_cid.pcpu);
+ scoped_guard(rq_lock_irq, this_rq()) {
+ struct mm_cid_pcpu *pcp = mm_cid_pcpu(&mm->mm_cid, this_rq());
+ guard(raw_spinlock)(&mm->mm_cid.lock);
/* First user ? */
if (!mm->mm_cid.users) {
@@ -10777,7 +10799,7 @@ void sched_mm_cid_fork(struct task_struct *t)
}
if (percpu) {
- mm_cid_fixup_tasks_to_cpus();
+ mm_cid_fixup_tasks_to_cpus(mm);
} else {
mm_cid_fixup_cpus_to_tasks(mm);
t->mm_cid.cid = mm_get_cid(mm);
@@ -10785,6 +10807,8 @@ void sched_mm_cid_fork(struct task_struct *t)
}
static bool sched_mm_cid_remove_user(struct task_struct *t)
+ __must_hold(t->mm->mm_cid.mutex)
+ __must_hold(t->mm->mm_cid.lock)
{
t->mm_cid.active = 0;
scoped_guard(preempt) {
@@ -10792,11 +10816,13 @@ static bool sched_mm_cid_remove_user(struct
task_struct *t)
t->mm_cid.cid = cid_from_transit_cid(t->mm_cid.cid);
mm_unset_cid_on_task(t);
}
- t->mm->mm_cid.users--;
+ t->mm->mm_cid.users--; /* mutex && lock */
return mm_update_max_cids(t->mm);
}
static bool __sched_mm_cid_exit(struct task_struct *t)
+ __must_hold(t->mm->mm_cid.mutex)
+ __must_hold(t->mm->mm_cid.lock)
{
struct mm_struct *mm = t->mm;
@@ -10837,8 +10863,9 @@ void sched_mm_cid_exit(struct task_struct *t)
*/
scoped_guard(mutex, &mm->mm_cid.mutex) {
/* mm_cid::mutex is sufficient to protect mm_cid::users */
- if (likely(mm->mm_cid.users > 1)) {
- scoped_guard(raw_spinlock_irq, &mm->mm_cid.lock) {
+ if (likely(mm_cid_users(mm) > 1)) {
+ scoped_guard(rq_lock_irq, this_rq()) {
+ guard(raw_spinlock)(&mm->mm_cid.lock);
if (!__sched_mm_cid_exit(t))
return;
/*
@@ -10847,16 +10874,17 @@ void sched_mm_cid_exit(struct task_struct *t)
* TRANSIT bit. If the CID is owned by the CPU
* then drop it.
*/
- mm_drop_cid_on_cpu(mm,
this_cpu_ptr(mm->mm_cid.pcpu));
+ mm_drop_cid_on_cpu(mm, mm_cid_pcpu(&mm->mm_cid,
this_rq()));
}
mm_cid_fixup_cpus_to_tasks(mm);
return;
}
/* Last user */
- scoped_guard(raw_spinlock_irq, &mm->mm_cid.lock) {
+ scoped_guard(rq_lock_irq, this_rq()) {
+ guard(raw_spinlock)(&mm->mm_cid.lock);
/* Required across execve() */
if (t == current)
- mm_cid_transit_to_task(t,
this_cpu_ptr(mm->mm_cid.pcpu));
+ mm_cid_transit_to_task(t,
mm_cid_pcpu(&mm->mm_cid, this_rq()));
/* Ignore mode change. There is nothing to do. */
sched_mm_cid_remove_user(t);
}
@@ -10893,7 +10921,7 @@ static void mm_cid_work_fn(struct work_struct *work)
guard(mutex)(&mm->mm_cid.mutex);
/* Did the last user task exit already? */
- if (!mm->mm_cid.users)
+ if (!mm_cid_users(mm))
return;
scoped_guard(raw_spinlock_irq, &mm->mm_cid.lock) {
@@ -10924,13 +10952,14 @@ static void mm_cid_irq_work(struct irq_work *work)
void mm_init_cid(struct mm_struct *mm, struct task_struct *p)
{
- mm->mm_cid.max_cids = 0;
mm->mm_cid.mode = 0;
- mm->mm_cid.nr_cpus_allowed = p->nr_cpus_allowed;
- mm->mm_cid.users = 0;
- mm->mm_cid.pcpu_thrs = 0;
- mm->mm_cid.update_deferred = 0;
- raw_spin_lock_init(&mm->mm_cid.lock);
+ mm->mm_cid.max_cids = 0;
+ scoped_guard (raw_spinlock_init, &mm->mm_cid.lock) {
+ mm->mm_cid.nr_cpus_allowed = p->nr_cpus_allowed;
+ mm->mm_cid.users = 0;
+ mm->mm_cid.pcpu_thrs = 0;
+ mm->mm_cid.update_deferred = 0;
+ }
mutex_init(&mm->mm_cid.mutex);
mm->mm_cid.irq_work = IRQ_WORK_INIT_HARD(mm_cid_irq_work);
INIT_WORK(&mm->mm_cid.work, mm_cid_work_fn);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index fd36ae390520..8c761822d6b2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3870,13 +3870,39 @@ static __always_inline void
mm_cid_update_task_cid(struct task_struct *t, unsign
}
}
-static __always_inline void mm_cid_update_pcpu_cid(struct mm_struct *mm,
unsigned int cid)
+/*
+ * Helpers to convert from __rq_lockp(rq) to RQ_LOCK intermediate.
+ */
+static __always_inline
+void mm_cid_update_pcpu_cid(struct rq *rq, struct mm_struct *mm, unsigned int
cid)
+ __must_hold(__rq_lockp(rq))
{
+ __assume_ctx_lock(RQ_LOCK);
+ lockdep_assert(rq->cpu == smp_processor_id());
__this_cpu_write(mm->mm_cid.pcpu->cid, cid);
}
-static __always_inline void mm_cid_from_cpu(struct task_struct *t, unsigned
int cpu_cid,
- unsigned int mode)
+static __always_inline
+unsigned int mm_cid_pcpu_cid(struct rq *rq, struct mm_struct *mm)
+ __must_hold(__rq_lockp(rq))
+{
+ __assume_ctx_lock(RQ_LOCK);
+ lockdep_assert(rq->cpu == smp_processor_id());
+ return __this_cpu_read(mm->mm_cid.pcpu->cid);
+}
+
+static __always_inline
+struct mm_cid_pcpu *mm_cid_pcpu(struct mm_mm_cid *mc, struct rq *rq)
+ __must_hold(__rq_lockp(rq))
+
+{
+ __assume_ctx_lock(RQ_LOCK);
+ return per_cpu_ptr(mc->pcpu, rq->cpu);
+}
+
+static __always_inline void mm_cid_from_cpu(struct rq *rq, struct task_struct
*t,
+ unsigned int cpu_cid, unsigned int
mode)
+ __must_hold(__rq_lockp(rq))
{
unsigned int max_cids, tcid = t->mm_cid.cid;
struct mm_struct *mm = t->mm;
@@ -3906,12 +3932,13 @@ static __always_inline void mm_cid_from_cpu(struct
task_struct *t, unsigned int
if (mode & MM_CID_TRANSIT)
cpu_cid = cpu_cid_to_cid(cpu_cid) | MM_CID_TRANSIT;
}
- mm_cid_update_pcpu_cid(mm, cpu_cid);
+ mm_cid_update_pcpu_cid(rq, mm, cpu_cid);
mm_cid_update_task_cid(t, cpu_cid);
}
-static __always_inline void mm_cid_from_task(struct task_struct *t, unsigned
int cpu_cid,
- unsigned int mode)
+static __always_inline void mm_cid_from_task(struct rq *rq, struct task_struct
*t,
+ unsigned int cpu_cid, unsigned int
mode)
+ __must_hold(__rq_lockp(rq))
{
unsigned int max_cids, tcid = t->mm_cid.cid;
struct mm_struct *mm = t->mm;
@@ -3920,7 +3947,7 @@ static __always_inline void mm_cid_from_task(struct
task_struct *t, unsigned int
/* Optimize for the common case, where both have the ONCPU bit clear */
if (likely(cid_on_task(tcid | cpu_cid))) {
if (likely(tcid < max_cids)) {
- mm_cid_update_pcpu_cid(mm, tcid);
+ mm_cid_update_pcpu_cid(rq, mm, tcid);
return;
}
/* Try to converge into the optimal CID space */
@@ -3929,7 +3956,7 @@ static __always_inline void mm_cid_from_task(struct
task_struct *t, unsigned int
/* Hand over or drop the CPU owned CID */
if (cid_on_cpu(cpu_cid)) {
if (cid_on_task(tcid))
- mm_drop_cid_on_cpu(mm,
this_cpu_ptr(mm->mm_cid.pcpu));
+ mm_drop_cid_on_cpu(mm, mm_cid_pcpu(&mm->mm_cid,
rq));
else
tcid = cpu_cid_to_cid(cpu_cid);
}
@@ -3939,11 +3966,12 @@ static __always_inline void mm_cid_from_task(struct
task_struct *t, unsigned int
/* Set the transition mode flag if required */
tcid |= mode & MM_CID_TRANSIT;
}
- mm_cid_update_pcpu_cid(mm, tcid);
+ mm_cid_update_pcpu_cid(rq, mm, tcid);
mm_cid_update_task_cid(t, tcid);
}
-static __always_inline void mm_cid_schedin(struct task_struct *next)
+static __always_inline void mm_cid_schedin(struct rq *rq, struct task_struct
*next)
+ __must_hold(__rq_lockp(rq))
{
struct mm_struct *mm = next->mm;
unsigned int cpu_cid, mode;
@@ -3951,15 +3979,16 @@ static __always_inline void mm_cid_schedin(struct
task_struct *next)
if (!next->mm_cid.active)
return;
- cpu_cid = __this_cpu_read(mm->mm_cid.pcpu->cid);
+ cpu_cid = mm_cid_pcpu_cid(rq, mm);
mode = READ_ONCE(mm->mm_cid.mode);
if (likely(!cid_on_cpu(mode)))
- mm_cid_from_task(next, cpu_cid, mode);
+ mm_cid_from_task(rq, next, cpu_cid, mode);
else
- mm_cid_from_cpu(next, cpu_cid, mode);
+ mm_cid_from_cpu(rq, next, cpu_cid, mode);
}
-static __always_inline void mm_cid_schedout(struct task_struct *prev)
+static __always_inline void mm_cid_schedout(struct rq *rq, struct task_struct
*prev)
+ __must_hold(__rq_lockp(rq))
{
struct mm_struct *mm = prev->mm;
unsigned int mode, cid;
@@ -3980,7 +4009,7 @@ static __always_inline void mm_cid_schedout(struct
task_struct *prev)
cid = cid_to_cpu_cid(cid);
/* Update both so that the next schedule in goes into the fast
path */
- mm_cid_update_pcpu_cid(mm, cid);
+ mm_cid_update_pcpu_cid(rq, mm, cid);
prev->mm_cid.cid = cid;
} else {
mm_drop_cid(mm, cid);
@@ -3988,10 +4017,12 @@ static __always_inline void mm_cid_schedout(struct
task_struct *prev)
}
}
-static inline void mm_cid_switch_to(struct task_struct *prev, struct
task_struct *next)
+static inline void mm_cid_switch_to(struct rq *rq, struct task_struct *prev,
+ struct task_struct *next)
+ __must_hold(__rq_lockp(rq))
{
- mm_cid_schedout(prev);
- mm_cid_schedin(next);
+ mm_cid_schedout(rq, prev);
+ mm_cid_schedin(rq, next);
}
#else /* !CONFIG_SCHED_MM_CID: */
