All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable
@ 2026-08-18 16:04 Changwoo Min
  2026-08-18 16:18 ` sashiko-bot
  2026-08-18 18:42 ` Tejun Heo
  0 siblings, 2 replies; 3+ messages in thread
From: Changwoo Min @ 2026-08-18 16:04 UTC (permalink / raw)
  To: tj, void, arighi, changwoo; +Cc: kernel-dev, sched-ext, linux-kernel

ops.cgroup_set_bandwidth() is delivered from scx_group_set_bandwidth(),
which runs from the cpu.max cgroup interface write path (tg_set_bandwidth())
in process context. scx_group_set_bandwidth() holds
percpu_down_read(&scx_cgroup_ops_rwsem), whose read side may sleep.
The call site is therefore sleepable, like ops.cgroup_init().

bpf_scx_check_member() rejects a sleepable program on any member not on its
allow-list, so a BPF scheduler cannot allocate -- which is sleepable -- when
a cgroup gains a cpu.max limit at runtime; it must instead pre-reserve memory
for a callback that cannot allocate. Add cgroup_set_bandwidth() to the
allow-list so the callback can allocate on demand, and document that it may
block.

A scheduler must decide at load time whether to mark the callback sleepable,
but the allow-list entry is a verifier property with no symbol to probe. Add
a compatibility marker whose presence in the kernel's BTF lets userspace detect
this support: DEFINE_SCX_COMPAT_MARKER() emits an empty, callerless function,
here scx_compat_marker_cgroup_set_bandwidth_may_sleep(). It is __used
__retain so neither the compiler nor the linker (under
CONFIG_LD_DEAD_CODE_DATA_ELIMINATION) drops it. The markers share the
scx_compat_marker_ prefix and are collected near the end of ext.c so more
can be added as further capabilities appear.

Signed-off-by: Changwoo Min <changwoo@igalia.com>
---
Change in v2:
- Mark the capability marker __retain in addition to __used so the linker
  does not garbage-collect it under CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
  (cf. __bpf_kfunc).
- Give the markers a shared scx_compat_marker_ prefix via a new
  DEFINE_SCX_COMPAT_MARKER() helper, collected near the module init code at
  the end of ext.c so future markers live in one place.
---
 kernel/sched/ext/ext.c      | 14 ++++++++++++++
 kernel/sched/ext/internal.h | 23 ++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 10af28a9f2c0..b646711a45fe 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -8079,6 +8079,7 @@ static int bpf_scx_check_member(const struct btf_type *t,
 	case offsetof(struct sched_ext_ops, cgroup_init):
 	case offsetof(struct sched_ext_ops, cgroup_exit):
 	case offsetof(struct sched_ext_ops, cgroup_prep_move):
+	case offsetof(struct sched_ext_ops, cgroup_set_bandwidth):
 #endif
 	case offsetof(struct sched_ext_ops, cpu_online):
 	case offsetof(struct sched_ext_ops, cpu_offline):
@@ -11041,3 +11042,16 @@ static int __init scx_init(void)
 	return 0;
 }
 __initcall(scx_init);
+
+/*
+ * Compatibility markers for userspace. Existence of a marker function
+ * represents that the kernel supports that sched-ext feature.
+ */
+
+/*
+ * scx_compat_marker_cgroup_set_bandwidth_may_sleep: advertises that
+ * ops.cgroup_set_bandwidth() may be implemented as a sleepable callback.
+ */
+#ifdef CONFIG_EXT_GROUP_SCHED
+DEFINE_SCX_COMPAT_MARKER(cgroup_set_bandwidth_may_sleep);
+#endif	/* CONFIG_EXT_GROUP_SCHED */
diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h
index 27bbf5e04d90..53e136a47924 100644
--- a/kernel/sched/ext/internal.h
+++ b/kernel/sched/ext/internal.h
@@ -753,7 +753,7 @@ struct sched_ext_ops {
 	 * @burst_us: bandwidth control burst
 	 *
 	 * Update @cgrp's bandwidth control parameters. This is from the cpu.max
-	 * cgroup interface.
+	 * cgroup interface. This operation may block.
 	 *
 	 * @quota_us / @period_us determines the CPU bandwidth @cgrp is entitled
 	 * to. For example, if @period_us is 1_000_000 and @quota_us is
@@ -2001,6 +2001,27 @@ struct scx_bstr_buf {
 	char			line[SCX_EXIT_MSG_LEN];
 };
 
+/* Internal helper for DEFINE_SCX_COMPAT_MARKER(). */
+#define DECLARE_SCX_COMPAT_MARKER(func)						\
+	extern void scx_compat_marker_##func(void)
+
+/**
+ * DEFINE_SCX_COMPAT_MARKER() - define a userspace capability marker
+ * @func: marker suffix; the defined symbol is scx_compat_marker_@func
+ *
+ * Emit an empty, callerless function that is retained in the kernel's BTF.
+ * Its presence is part of the kernel<->userspace contract: userspace probes
+ * scx_compat_marker_@func (e.g. via BTF) to detect that this kernel supports
+ * the corresponding feature.
+ *
+ * The leading declaration suppresses the missing-prototype warning; the
+ * trailing declaration consumes the semicolon at the use site.
+ */
+#define DEFINE_SCX_COMPAT_MARKER(func)						\
+	DECLARE_SCX_COMPAT_MARKER(func);					\
+	__used __retain void scx_compat_marker_##func(void) {}			\
+	DECLARE_SCX_COMPAT_MARKER(func)
+
 extern struct scx_sched __rcu *scx_root;
 DECLARE_PER_CPU(struct rq *, scx_locked_rq_state);
 
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable
  2026-08-18 16:04 [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable Changwoo Min
@ 2026-08-18 16:18 ` sashiko-bot
  2026-08-18 18:42 ` Tejun Heo
  1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-18 16:18 UTC (permalink / raw)
  To: Changwoo Min; +Cc: sched-ext

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Concurrent cgroup writes to `cpu.max`, `cpu.weight`, or `cpu.idle` race to update the cached `tg->scx` state without an exclusive lock, causing the BPF scheduler's state to silently and permanently diverge from the cgroup state.
--

--- Patch [1]: [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable ---
commit dc54116ac36c38be3f813a52900e454dacbf26ad
Author: Changwoo Min <changwoo@igalia.com>

sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable

This commit adds cgroup_set_bandwidth to the allow-list in
bpf_scx_check_member, allowing its BPF callback to sleep. It also adds a
BTF compatibility marker so userspace can detect this capability.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 10af28a9f2c0c..b646711a45fee 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -8079,6 +8079,7 @@ static int bpf_scx_check_member(const struct btf_type *t,
>  	case offsetof(struct sched_ext_ops, cgroup_init):
>  	case offsetof(struct sched_ext_ops, cgroup_exit):
>  	case offsetof(struct sched_ext_ops, cgroup_prep_move):
> +	case offsetof(struct sched_ext_ops, cgroup_set_bandwidth):

[Severity: Medium]
This is a pre-existing issue, but does making this callback sleepable
significantly widen a data race in cgroup state updates?

Looking at concurrent writes to cpu.max, tg_set_bandwidth() drops the
exclusive lock before calling into sched_ext:

kernel/sched/core.c:tg_set_bandwidth() {
	...
#ifdef CONFIG_CFS_BANDWIDTH
	ret = tg_set_cfs_bandwidth(tg, period_us, quota_us, burst_us);
#endif
	if (!ret)
		scx_group_set_bandwidth(tg, period_us, quota_us, burst_us);
}

The cfs_constraints_mutex is dropped inside tg_set_cfs_bandwidth(), so
scx_group_set_bandwidth() runs without an exclusive lock.

Then in scx_group_set_bandwidth(), only a read lock is held while executing
the BPF hook and updating the state:

kernel/sched/ext/ext.c:scx_group_set_bandwidth() {
	percpu_down_read(&scx_cgroup_ops_rwsem);
	...
	SCX_CALL_OP(sch, cgroup_set_bandwidth, NULL, tg_cgrp(tg), ...);
	
	tg->scx.bw_period_us = period_us;
	...
}

Since percpu_down_read() allows concurrent readers, two concurrent writes
can interleave. With the BPF program now allowed to block inside
SCX_CALL_OP(), could this cause the cached tg->scx state to permanently
diverge from the actual cgroup state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818160429.932265-1-changwoo@igalia.com?part=1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable
  2026-08-18 16:04 [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable Changwoo Min
  2026-08-18 16:18 ` sashiko-bot
@ 2026-08-18 18:42 ` Tejun Heo
  1 sibling, 0 replies; 3+ messages in thread
From: Tejun Heo @ 2026-08-18 18:42 UTC (permalink / raw)
  To: Changwoo Min; +Cc: void, arighi, emil, kernel-dev, sched-ext, linux-kernel

Hello,

On Wed, Aug 19, 2026 at 01:04:29AM +0900, Changwoo Min wrote:
> ops.cgroup_set_bandwidth() is delivered from scx_group_set_bandwidth(),
> which runs from the cpu.max cgroup interface write path (tg_set_bandwidth())
> in process context. scx_group_set_bandwidth() holds
> percpu_down_read(&scx_cgroup_ops_rwsem), whose read side may sleep.
> The call site is therefore sleepable, like ops.cgroup_init().

Applied to sched_ext/for-7.3-fixes with the subject capitalization fixed.

Thanks.

--
tejun

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-08-18 18:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-18 16:04 [PATCH v2] sched_ext: allow ops.cgroup_set_bandwidth() to be sleepable Changwoo Min
2026-08-18 16:18 ` sashiko-bot
2026-08-18 18:42 ` Tejun Heo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.