Re: [PATCH 2/2] sched_ext: scx_qmap: Reset a child's cpuperf targets once its PERF revoke takes effect

[Andrea Righi <arighi@nvidia.com>]

Hi Tejun,

On Wed, Oct 07, 2026 at 11:32:28PM -1000, Tejun Heo wrote:
> A child that ran a cid at a low cpuperf target leaves the target there when
> PERF is revoked, since the kernel resets targets only at root enable. Reset
> the target from ops.sub_child_ecaps_updated() once the revoke is in effect.
> A detach reports nothing, so ops.sub_detach() resets the child's cids, the
> pool included. Count the deliveries in the hier stats.
> 
> v2: Reset from ops.sub_detach() for a detaching child's cids.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  tools/sched_ext/scx_qmap.bpf.c | 33 +++++++++++++++++++++++++++++++--
>  tools/sched_ext/scx_qmap.c     |  7 +++++--
>  tools/sched_ext/scx_qmap.h     |  1 +
>  3 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/sched_ext/scx_qmap.bpf.c b/tools/sched_ext/scx_qmap.bpf.c
> index 7f69394556b6..4c152b44bc91 100644
> --- a/tools/sched_ext/scx_qmap.bpf.c
> +++ b/tools/sched_ext/scx_qmap.bpf.c
> @@ -2167,12 +2167,30 @@ s32 BPF_STRUCT_OPS(qmap_sub_attach, struct scx_sub_attach_args *args)
>  
>  void BPF_STRUCT_OPS(qmap_sub_detach, struct scx_sub_detach_args *args)
>  {
> -	s32 i;
> +	u64 cgid = args->ops->sub_cgroup_id;
> +	s32 nr_cids = qa.nr_cids;
> +	s32 i, cid;
> +
> +	if (nr_cids < 0 || nr_cids > SCX_QMAP_MAX_CPUS) {
> +		scx_bpf_error("-ERANGE");
> +		return;
> +	}
>  
>  	for (i = 0; i < MAX_SUB_SCHEDS; i++) {
> -		if (qa.sub_sched_ctxs[i].cgroup_id != args->ops->sub_cgroup_id)
> +		if (qa.sub_sched_ctxs[i].cgroup_id != cgid)
>  			continue;
>  
> +		/*
> +		 * The child's caps are gone without a report, see
> +		 * ops.sub_detach(). cpuperf targets persist until the next
> +		 * write, reset the child's excl cids and the pool it may have
> +		 * held.
> +		 */
> +		bpf_arena_for(cid, 0, nr_cids)
> +			if (cmask_test(cid, &qa.sub_sched_ctxs[i].granted_cids.mask) ||
> +			    cmask_test(cid, &qa.rr_cids.mask))
> +				scx_bpf_cidperf_set(cid, SCX_CPUPERF_ONE);
> +

qa.rr_cids covers the entire shared pool, regardless of its current holder,
right? If child A detaches during child B's turn, this resets the pool's cpuperf
target even if B still holds PERF and has written its own target.

Should we avoid resetting the current holder's target here?

Thanks,
-Andrea

>  		qa.sub_sched_ctxs[i].cgroup_id = 0;
>  		qa.sub_sched_ctxs[i].weight = 100;
>  		cmask_init(&qa.sub_sched_ctxs[i].granted_cids.mask, 0, qa.nr_cids);
> @@ -2208,6 +2226,16 @@ void BPF_STRUCT_OPS(qmap_sub_ecaps_updated, s32 cid, u64 before, u64 after)
>  	execute_partition();
>  }
>  
> +void BPF_STRUCT_OPS(qmap_sub_child_ecaps_updated, u64 cgroup_id, s32 cid, u64 before,
> +		    u64 after)
> +{
> +	__sync_fetch_and_add(&qa.nr_child_ecaps, 1);
> +
> +	/* a child's last target must not outlive its PERF cap */
> +	if ((before & ~after) & SCX_CAP_PERF)
> +		scx_bpf_cidperf_set(cid, SCX_CPUPERF_ONE);
> +}
> +
>  SCX_OPS_CID_DEFINE(qmap_ops,
>  	       .flags			= SCX_OPS_ENQ_EXITING | SCX_OPS_TID_TO_TASK,
>  	       .select_cid		= (void *)qmap_select_cid,
> @@ -2232,6 +2260,7 @@ SCX_OPS_CID_DEFINE(qmap_ops,
>  	       .sub_caps_updated	= (void *)qmap_sub_caps_updated,
>  	       .sub_ecaps_updated	= (void *)qmap_sub_ecaps_updated,
>  	       .sub_cid_sched_updated	= (void *)qmap_sub_cid_sched_updated,
> +	       .sub_child_ecaps_updated	= (void *)qmap_sub_child_ecaps_updated,
>  	       .init_cids		= (void *)qmap_init_cids,
>  	       .init			= (void *)qmap_init,
>  	       .exit			= (void *)qmap_exit,
> diff --git a/tools/sched_ext/scx_qmap.c b/tools/sched_ext/scx_qmap.c
> index e83d5e2147dd..17cf584a2a3d 100644
> --- a/tools/sched_ext/scx_qmap.c
> +++ b/tools/sched_ext/scx_qmap.c
> @@ -113,6 +113,7 @@ struct hier_prev {
>  	u64 nr_enq_blocked;
>  	u64 nr_inject_attempts;
>  	u64 nr_rescue_dsp;
> +	u64 nr_child_ecaps;
>  };
>  
>  /* current wall-clock time as "HH:MM:SS" for the startup and interval headers */
> @@ -208,18 +209,20 @@ static void print_hier(struct qmap_arena *qa, struct hier_prev *prev, u64 own_cg
>  	}
>  
>  	format_cid_ranges(qa, CID_SHARED, ranges, sizeof(ranges));
> -	printf("hier   : nsub=%llu excl=%u shared=%s rr=%s reenq cap/immed +%llu/+%llu blocked=+%llu inj=+%llu rescue=+%llu\n",
> +	printf("hier   : nsub=%llu excl=%u shared=%s rr=%s reenq cap/immed +%llu/+%llu blocked=+%llu inj=+%llu rescue=+%llu child_ecaps=+%llu\n",
>  	       (unsigned long long)qa->nr_sub_scheds, qa->part.nr_excl, ranges, rr,
>  	       (unsigned long long)(qa->nr_reenq_cap - prev->nr_reenq_cap),
>  	       (unsigned long long)(qa->nr_reenq_immed - prev->nr_reenq_immed),
>  	       (unsigned long long)(qa->nr_enq_blocked - prev->nr_enq_blocked),
>  	       (unsigned long long)(qa->nr_inject_attempts - prev->nr_inject_attempts),
> -	       (unsigned long long)(qa->nr_rescue_dsp - prev->nr_rescue_dsp));
> +	       (unsigned long long)(qa->nr_rescue_dsp - prev->nr_rescue_dsp),
> +	       (unsigned long long)(qa->nr_child_ecaps - prev->nr_child_ecaps));
>  	prev->nr_reenq_cap = qa->nr_reenq_cap;
>  	prev->nr_reenq_immed = qa->nr_reenq_immed;
>  	prev->nr_enq_blocked = qa->nr_enq_blocked;
>  	prev->nr_inject_attempts = qa->nr_inject_attempts;
>  	prev->nr_rescue_dsp = qa->nr_rescue_dsp;
> +	prev->nr_child_ecaps = qa->nr_child_ecaps;
>  
>  	/*
>  	 * alloc is the cid-time the partition handed each participant, and used
> diff --git a/tools/sched_ext/scx_qmap.h b/tools/sched_ext/scx_qmap.h
> index 949459d06a18..9e6eefb39735 100644
> --- a/tools/sched_ext/scx_qmap.h
> +++ b/tools/sched_ext/scx_qmap.h
> @@ -195,6 +195,7 @@ struct qmap_arena {
>  	u64 nr_enq_blocked;		/* SCX_ENQ_BLOCKED dispatches */
>  	u64 nr_inject_attempts;		/* fault-injection: dispatches to an unheld cid */
>  	u64 nr_rescue_dsp;		/* SCX_ENQ_RESCUE dispatch attempts */
> +	u64 nr_child_ecaps;		/* ops.sub_child_ecaps_updated() deliveries */
>  	u32 inject_mode;		/* fault-injection mode (QMAP_INJ_*) */
>  };
>  
> -- 
> 2.55.0
>