sched_ext: Fix missing post-enqueue handling in move_local_task_to_local_dsq()
Subsystems (3)
Tejun Heo
reviewerPATCH 1/1 sched_ext: Fix missing post-enqueue handling in move_local_task_to_local_dsq()
Hello,
move_local_task_to_local_dsq() was missing post-enqueue handling which matters now that scx_bpf_dsq_move() can be called while the CPU is busy.
0001-sched_ext-Factor-out-local_dsq_post_enq-from-dispatc.patch 0002-sched_ext-Fix-missing-post-enqueue-handling-in-move_.patch
diffstat:
kernel/sched/ext.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-)
--
tejun
Tejun Heo
reviewerPATCH 1/2 sched_ext: Factor out local_dsq_post_enq() from dispatch_enqueue()
Factor out local_dsq_post_enq() which performs post-enqueue handling for local DSQs - triggering preemption or resched_curr() if the enqueued task has higher priority than the currently running task. No functional change.
This will be used by the next patch to fix move_local_task_to_local_dsq().
--- kernel/sched/ext.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
kernel/sched/ext.c +19 -15
@@ -975,6 +975,22 @@ static void refill_task_slice_dfl(struct__scx_add_event (sch,SCX_EV_REFILL_SLICE_DFL ,1 ); } +static void local_dsq_post_enq (struct scx_dispatch_q *dsq,struct task_struct *p, +u64 enq_flags) +{ + struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); +bool preempt = false; + +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && + rq->curr->sched_class == &ext_sched_class) { + rq->curr->scx.slice =0 ; + preempt = true; + } + +if (preempt ||sched_class_above (&ext_sched_class, rq->curr->sched_class)) +resched_curr (rq); +} +static void dispatch_enqueue (struct scx_sched *sch,struct scx_dispatch_q *dsq,struct task_struct *p,u64 enq_flags) {
@@ -1086,22 +1102,10 @@ static void dispatch_enqueue(struct scx_if (enq_flags &SCX_ENQ_CLEAR_OPSS )atomic_long_set_release (&p->scx.ops_state,SCX_OPSS_NONE ); -if (is_local) { - struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); -bool preempt = false; - -if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && - rq->curr->sched_class == &ext_sched_class) { - rq->curr->scx.slice =0 ; - preempt = true; - } - -if (preempt ||sched_class_above (&ext_sched_class, - rq->curr->sched_class)) -resched_curr (rq); - }else { +if (is_local) +local_dsq_post_enq (dsq, p, enq_flags); +else raw_spin_unlock (&dsq->lock); - } } static void task_unlink_from_dsq(struct task_struct *p,
Andrea Righi
reviewerRe: [PATCH 1/2] sched_ext: Factor out local_dsq_post_enq() from dispatch_enqueue()
Quoted 3 lines · Tejun Heo
Factor out
local_dsq_post_enq()which performs post-enqueue handling for local DSQs - triggering preemption orresched_curr()if the enqueued task has higher priority than the currently running task. No functional change.
Maybe "higher priority" is a bit misleading, how about: "triggering resched_curr() if SCX_ENQ_PREEMPT is specified or if the current CPU is idle".
Thanks,
-Andrea
Quoted 59 lines
This will be used by the next patch to fix
move_local_task_to_local_dsq().Signed-off-by Tejun Heo <[email protected]>--- kernel/sched/ext.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)kernel/sched/ext.c +19
-15@@ -975,6 +975,22 @@ static void refill_task_slice_dfl(struct__scx_add_event (sch,SCX_EV_REFILL_SLICE_DFL ,1 ); } +static void local_dsq_post_enq (struct scx_dispatch_q *dsq,struct task_struct *p, +u64 enq_flags) +{ + struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); +bool preempt = false; + +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && + rq->curr->sched_class == &ext_sched_class) { + rq->curr->scx.slice =0 ; + preempt = true; + } + +if (preempt ||sched_class_above (&ext_sched_class, rq->curr->sched_class)) +resched_curr (rq); +} +static void dispatch_enqueue (struct scx_sched *sch,struct scx_dispatch_q *dsq,struct task_struct *p,u64 enq_flags) {@@ -1086,22 +1102,10 @@ static void dispatch_enqueue(struct scx_if (enq_flags &SCX_ENQ_CLEAR_OPSS )atomic_long_set_release (&p->scx.ops_state,SCX_OPSS_NONE ); -if (is_local) { - struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); -bool preempt = false; - -if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && - rq->curr->sched_class == &ext_sched_class) { - rq->curr->scx.slice =0 ; - preempt = true; - } - -if (preempt ||sched_class_above (&ext_sched_class, - rq->curr->sched_class)) -resched_curr (rq); - }else { +if (is_local) +local_dsq_post_enq (dsq, p, enq_flags); +else raw_spin_unlock (&dsq->lock); - } } static void task_unlink_from_dsq(struct task_struct *p,
Tejun Heo
reviewerPATCH 2/2 sched_ext: Fix missing post-enqueue handling in move_local_task_to_local_dsq()
In reply to Tejun Heomove_local_task_to_local_dsq() is used when moving a task from a non-local DSQ to a local DSQ on the same CPU. It directly manipulates the local DSQ without going through dispatch_enqueue() and was missing the post-enqueue handling that triggers preemption when SCX_ENQ_PREEMPT is set or the enqueued task should preempt a lower priority task.
This was fine when the function was only used from consume_dispatch_q() where the CPU is already vacant and looking for the next task. However, the function is now also used by move_task_between_dsqs() which backs scx_bpf_dsq_move() which may be called while the CPU is busy.
Add local_dsq_post_enq() call to move_local_task_to_local_dsq(). As the dispatch path doesn't need post-enqueue handling, add SCX_RQ_IN_BALANCE early exit to keep consume_dispatch_q() behavior unchanged and avoid triggering unnecessary resched when scx_bpf_dsq_move() is used from the dispatch path.
--- kernel/sched/ext.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
kernel/sched/ext.c +10 -0
@@ -981,6 +981,14 @@ static void local_dsq_post_enq(struct sc struct rq *rq = container_of(dsq, struct rq, scx.local_dsq);bool preempt = false; +/* +* If @rq is in balance, the CPU is already vacant and looking for the +* next task to run. No need to preempt or trigger resched after moving +* @p into its local DSQ. +*/ +if (rq->scx.flags &SCX_RQ_IN_BALANCE ) +return ; +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && rq->curr->sched_class == &ext_sched_class) { rq->curr->scx.slice =0 ;
@@ -1629,6 +1637,8 @@ static void move_local_task_to_local_dsqdsq_mod_nr (dst_dsq,1 ); p->scx.dsq = dst_dsq; + +local_dsq_post_enq (dst_dsq, p, enq_flags); }/**
Andrea Righi
reviewerQuoted 5 lines · Tejun Heo
move_local_task_to_local_dsq()is used when moving a task from a non-local DSQ to a local DSQ on the same CPU. It directly manipulates the local DSQ without going throughdispatch_enqueue()and was missing the post-enqueue handling that triggers preemption whenSCX_ENQ_PREEMPTis set or the enqueued task should preempt a lower priority task.
Ditto, lower sched class / idle task maybe?
Thanks,
-Andrea
Quoted 43 lines
This was fine when the function was only used from
consume_dispatch_q()where the CPU is already vacant and looking for the next task. However, the function is now also used bymove_task_between_dsqs()which backsscx_bpf_dsq_move()which may be called while the CPU is busy.Add
local_dsq_post_enq()call tomove_local_task_to_local_dsq(). As the dispatch path doesn't need post-enqueue handling, addSCX_RQ_IN_BALANCEearly exit to keepconsume_dispatch_q()behavior unchanged and avoid triggering unnecessary resched whenscx_bpf_dsq_move()is used from the dispatch path.Signed-off-by Tejun Heo <[email protected]>--- kernel/sched/ext.c | 10 ++++++++++ 1 file changed, 10 insertions(+)kernel/sched/ext.c +10
-0@@ -981,6 +981,14 @@ static void local_dsq_post_enq(struct sc struct rq *rq = container_of(dsq, struct rq, scx.local_dsq);bool preempt = false; +/* +* If @rq is in balance, the CPU is already vacant and looking for the +* next task to run. No need to preempt or trigger resched after moving +* @p into its local DSQ. +*/ +if (rq->scx.flags &SCX_RQ_IN_BALANCE ) +return ; +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && rq->curr->sched_class == &ext_sched_class) { rq->curr->scx.slice =0 ;@@ -1629,6 +1637,8 @@ static void move_local_task_to_local_dsqdsq_mod_nr (dst_dsq,1 ); p->scx.dsq = dst_dsq; + +local_dsq_post_enq (dst_dsq, p, enq_flags); }/**
Quoted 10 lines · Tejun Heo
move_local_task_to_local_dsq()is used when moving a task from a non-local DSQ to a local DSQ on the same CPU. It directly manipulates the local DSQ without going throughdispatch_enqueue()and was missing the post-enqueue handling that triggers preemption whenSCX_ENQ_PREEMPTis set or the enqueued task should preempt a lower priority task.This was fine when the function was only used from
consume_dispatch_q()where the CPU is already vacant and looking for the next task. However, the function is now also used bymove_task_between_dsqs()which backsscx_bpf_dsq_move()which may be called while the CPU is busy.
When was this changed? I couldn't find the commit when looking briefly :/
Quoted 39 lines
Add
local_dsq_post_enq()call tomove_local_task_to_local_dsq(). As the dispatch path doesn't need post-enqueue handling, addSCX_RQ_IN_BALANCEearly exit to keepconsume_dispatch_q()behavior unchanged and avoid triggering unnecessary resched whenscx_bpf_dsq_move()is used from the dispatch path.Signed-off-by Tejun Heo <[email protected]>--- kernel/sched/ext.c | 10 ++++++++++ 1 file changed, 10 insertions(+)kernel/sched/ext.c +10
-0@@ -981,6 +981,14 @@ static void local_dsq_post_enq(struct sc struct rq *rq = container_of(dsq, struct rq, scx.local_dsq);bool preempt = false; +/* +* If @rq is in balance, the CPU is already vacant and looking for the +* next task to run. No need to preempt or trigger resched after moving +* @p into its local DSQ. +*/ +if (rq->scx.flags &SCX_RQ_IN_BALANCE ) +return ; +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && rq->curr->sched_class == &ext_sched_class) { rq->curr->scx.slice =0 ;@@ -1629,6 +1637,8 @@ static void move_local_task_to_local_dsqdsq_mod_nr (dst_dsq,1 ); p->scx.dsq = dst_dsq; + +local_dsq_post_enq (dst_dsq, p, enq_flags); }/**
Tejun Heo
reviewerQuoted 7 lines · Christian Loehle
Quoted 6 lines · Tejun Heo
move_local_task_to_local_dsq()is used when moving a task from a non-local DSQ to a local DSQ on the same CPU. It directly manipulates the local DSQ without going throughdispatch_enqueue()and was missing the post-enqueue handling that triggers preemption whenSCX_ENQ_PREEMPTis set or the enqueued task should preempt a lower priority task.
Quoted 6 lines
Quoted 4 lines
This was fine when the function was only used from
consume_dispatch_q()where the CPU is already vacant and looking for the next task. However, the function is now also used bymove_task_between_dsqs()which backsscx_bpf_dsq_move()which may be called while the CPU is busy.When was this changed? I couldn't find the commit when looking briefly :/
4c30f5ce4f7a ("sched_ext: Implement scx_bpf_dispatch[_vtime]_from_dsq()") which was v6.12, so upstream history-wise, the above paragraph isn't accurate. It was broken from get-go. Will update the commit message. I'm split on whether I should tag stable for it given that the usage pattern isn't very common and there's some risk in the behavior change. Hmm... might as well.
Thanks.
Signature · 1 line
tejun
Hi Tejun,
Quoted 7 lines · Tejun Heo
Hello,
move_local_task_to_local_dsq()was missing post-enqueue handling which matters now thatscx_bpf_dsq_move()can be called while the CPU is busy.0001-sched_ext-Factor-out-local_dsq_post_enq-from-dispatc.patch 0002-sched_ext-Fix-missing-post-enqueue-handling-in-move_.patch
A couple of comments about the commit description, but overall looks good to me.
Thanks,
-Andrea
Quoted 7 lines
diffstat:
kernel/sched/ext.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-)--
tejun
Tejun Heo
reviewerPATCH v2 1/1 sched_ext: Fix missing post-enqueue handling in move_local_task_to_local_dsq()
In reply to Tejun HeoHello,
move_local_task_to_local_dsq() was missing post-enqueue handling which matters now that scx_bpf_dsq_move() can be called while the CPU is busy.
v2: Updated commit messages and added Cc stable.
v1: http://lkml.kernel.org/r/[email protected]
0001-sched_ext-Factor-out-local_dsq_post_enq-from-dispatc.patch 0002-sched_ext-Fix-missing-post-enqueue-handling-in-move_.patch
Based on sched_ext/for-6.19-fixes (9f769637a93f).
diffstat:
kernel/sched/ext.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-)
--
tejun
Tejun Heo
reviewerPATCH 1/2 sched_ext: Factor out local_dsq_post_enq() from dispatch_enqueue()
Factor out local_dsq_post_enq() which performs post-enqueue handling for local DSQs - triggering resched_curr() if SCX_ENQ_PREEMPT is specified or if the current CPU is idle. No functional change.
This will be used by the next patch to fix move_local_task_to_local_dsq().
--- kernel/sched/ext.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
kernel/sched/ext.c +19 -15
@@ -982,6 +982,22 @@ static void refill_task_slice_dfl(struct scx_sched *sch, struct task_struct *p)__scx_add_event (sch,SCX_EV_REFILL_SLICE_DFL ,1 ); } +static void local_dsq_post_enq (struct scx_dispatch_q *dsq,struct task_struct *p, +u64 enq_flags) +{ + struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); +bool preempt = false; + +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && + rq->curr->sched_class == &ext_sched_class) { + rq->curr->scx.slice =0 ; + preempt = true; + } + +if (preempt ||sched_class_above (&ext_sched_class, rq->curr->sched_class)) +resched_curr (rq); +} +static void dispatch_enqueue (struct scx_sched *sch,struct scx_dispatch_q *dsq,struct task_struct *p,u64 enq_flags) {
@@ -1093,22 +1109,10 @@ static void dispatch_enqueue(struct scx_sched *sch, struct scx_dispatch_q *dsq,if (enq_flags &SCX_ENQ_CLEAR_OPSS )atomic_long_set_release (&p->scx.ops_state,SCX_OPSS_NONE ); -if (is_local) { - struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); -bool preempt = false; - -if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && - rq->curr->sched_class == &ext_sched_class) { - rq->curr->scx.slice =0 ; - preempt = true; - } - -if (preempt ||sched_class_above (&ext_sched_class, - rq->curr->sched_class)) -resched_curr (rq); - }else { +if (is_local) +local_dsq_post_enq (dsq, p, enq_flags); +else raw_spin_unlock (&dsq->lock); - } } static void task_unlink_from_dsq(struct task_struct *p,
Signature · 1 line
2.52.0
Re: [PATCH 1/2] sched_ext: Factor out local_dsq_post_enq() from dispatch_enqueue()
Quoted 10 lines · Tejun Heo
Factor out
local_dsq_post_enq()which performs post-enqueue handling for local DSQs - triggeringresched_curr()ifSCX_ENQ_PREEMPTis specified or if the current CPU is idle. No functional change.This will be used by the next patch to fix
move_local_task_to_local_dsq().Cc [email protected] # v6.12+Reviewed-by Andrea Righi <[email protected]>Signed-off-by Tejun Heo <[email protected]>---
Quoted 56 lines
kernel/sched/ext.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)kernel/sched/ext.c +19
-15@@ -982,6 +982,22 @@ static void refill_task_slice_dfl(struct scx_sched *sch, struct task_struct *p)__scx_add_event (sch,SCX_EV_REFILL_SLICE_DFL ,1 ); } +static void local_dsq_post_enq (struct scx_dispatch_q *dsq,struct task_struct *p, +u64 enq_flags) +{ + struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); +bool preempt = false; + +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && + rq->curr->sched_class == &ext_sched_class) { + rq->curr->scx.slice =0 ; + preempt = true; + } + +if (preempt ||sched_class_above (&ext_sched_class, rq->curr->sched_class)) +resched_curr (rq); +} +static void dispatch_enqueue (struct scx_sched *sch,struct scx_dispatch_q *dsq,struct task_struct *p,u64 enq_flags) {@@ -1093,22 +1109,10 @@ static void dispatch_enqueue(struct scx_sched *sch, struct scx_dispatch_q *dsq,if (enq_flags &SCX_ENQ_CLEAR_OPSS )atomic_long_set_release (&p->scx.ops_state,SCX_OPSS_NONE ); -if (is_local) { - struct rq *rq = container_of(dsq, struct rq, scx.local_dsq); -bool preempt = false; - -if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && - rq->curr->sched_class == &ext_sched_class) { - rq->curr->scx.slice =0 ; - preempt = true; - } - -if (preempt ||sched_class_above (&ext_sched_class, - rq->curr->sched_class)) -resched_curr (rq); - }else { +if (is_local) +local_dsq_post_enq (dsq, p, enq_flags); +else raw_spin_unlock (&dsq->lock); - } } static void task_unlink_from_dsq(struct task_struct *p,
Tejun Heo
reviewerPATCH 2/2 sched_ext: Fix missing post-enqueue handling in move_local_task_to_local_dsq()
In reply to Tejun Heomove_local_task_to_local_dsq() is used when moving a task from a non-local DSQ to a local DSQ on the same CPU. It directly manipulates the local DSQ without going through dispatch_enqueue() and was missing the post-enqueue handling that triggers preemption when SCX_ENQ_PREEMPT is set or the idle task is running.
The function is used by move_task_between_dsqs() which backs scx_bpf_dsq_move() and may be called while the CPU is busy.
Add local_dsq_post_enq() call to move_local_task_to_local_dsq(). As the dispatch path doesn't need post-enqueue handling, add SCX_RQ_IN_BALANCE early exit to keep consume_dispatch_q() behavior unchanged and avoid triggering unnecessary resched when scx_bpf_dsq_move() is used from the dispatch path.
--- kernel/sched/ext.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
kernel/sched/ext.c +10 -0
@@ -988,6 +988,14 @@ static void local_dsq_post_enq(struct scx_dispatch_q *dsq, struct task_struct *p struct rq *rq = container_of(dsq, struct rq, scx.local_dsq);bool preempt = false; +/* +* If @rq is in balance, the CPU is already vacant and looking for the +* next task to run. No need to preempt or trigger resched after moving +* @p into its local DSQ. +*/ +if (rq->scx.flags &SCX_RQ_IN_BALANCE ) +return ; +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && rq->curr->sched_class == &ext_sched_class) { rq->curr->scx.slice =0 ;
@@ -1636,6 +1644,8 @@ static void move_local_task_to_local_dsq(struct task_struct *p, u64 enq_flags,dsq_mod_nr (dst_dsq,1 ); p->scx.dsq = dst_dsq; + +local_dsq_post_enq (dst_dsq, p, enq_flags); }/**
Signature · 1 line
2.52.0
Quoted 15 lines · Tejun Heo
move_local_task_to_local_dsq()is used when moving a task from a non-local DSQ to a local DSQ on the same CPU. It directly manipulates the local DSQ without going throughdispatch_enqueue()and was missing the post-enqueue handling that triggers preemption whenSCX_ENQ_PREEMPTis set or the idle task is running.The function is used by
move_task_between_dsqs()which backsscx_bpf_dsq_move()and may be called while the CPU is busy.Add
local_dsq_post_enq()call tomove_local_task_to_local_dsq(). As the dispatch path doesn't need post-enqueue handling, addSCX_RQ_IN_BALANCEearly exit to keepconsume_dispatch_q()behavior unchanged and avoid triggering unnecessary resched whenscx_bpf_dsq_move()is used from the dispatch path.
Quoted 4 lines
Fixes 4c30f5ce4f7a ("sched_ext: Implement scx_bpf_dispatch[_vtime]_from_dsq()")Cc [email protected] # v6.12+Reviewed-by Andrea Righi <[email protected]>Signed-off-by Tejun Heo <[email protected]>
Quoted 32 lines
--- kernel/sched/ext.c | 10 ++++++++++ 1 file changed, 10 insertions(+)kernel/sched/ext.c +10
-0@@ -988,6 +988,14 @@ static void local_dsq_post_enq(struct scx_dispatch_q *dsq, struct task_struct *p struct rq *rq = container_of(dsq, struct rq, scx.local_dsq);bool preempt = false; +/* +* If @rq is in balance, the CPU is already vacant and looking for the +* next task to run. No need to preempt or trigger resched after moving +* @p into its local DSQ. +*/ +if (rq->scx.flags &SCX_RQ_IN_BALANCE ) +return ; +if ((enq_flags &SCX_ENQ_PREEMPT ) && p != rq->curr && rq->curr->sched_class == &ext_sched_class) { rq->curr->scx.slice =0 ;@@ -1636,6 +1644,8 @@ static void move_local_task_to_local_dsq(struct task_struct *p, u64 enq_flags,dsq_mod_nr (dst_dsq,1 ); p->scx.dsq = dst_dsq; + +local_dsq_post_enq (dst_dsq, p, enq_flags); }/**
Quoted 3 lines
Tejun Heo (2):
sched_ext: Factor outlocal_dsq_post_enq()fromdispatch_enqueue()
sched_ext: Fix missing post-enqueue handling inmove_local_task_to_local_dsq()
Applied to sched_ext/for-6.19-fixes.
Thanks.
--
tejun