diff mbox series

[05/31] sched: Add sched_class->reweight_task()

Message ID 20221130082313.3241517-6-tj@kernel.org (mailing list archive)
State RFC
Delegated to: BPF
Headers show
Series [01/31] rhashtable: Allow rhashtable to be used from irq-safe contexts | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR fail merge-conflict

Commit Message

Tejun Heo Nov. 30, 2022, 8:22 a.m. UTC 
Currently, during a task weight change, sched core directly calls
reweight_task() defined in fair.c if @p is on CFS. Let's make it a proper
sched_class operation instead. CFS's reweight_task() is renamed to
reweight_task_fair() and now called through sched_class.

While it turns a direct call into an indirect one, set_load_weight() isn't
called from a hot path and this change shouldn't cause any noticeable
difference. This will be used to implement reweight_task for other scheduler
classes.

This will be used by a new sched_class to track weight changes.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reviewed-by: David Vernet <dvernet@meta.com>
Acked-by: Josh Don <joshdon@google.com>
Acked-by: Hao Luo <haoluo@google.com>
Acked-by: Barret Rhoden <brho@google.com>
---
 kernel/sched/core.c  | 4 ++--
 kernel/sched/fair.c  | 3 ++-
 kernel/sched/sched.h | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

Comments

Peter Zijlstra Dec. 12, 2022, 11:22 a.m. UTC | #1 
On Tue, Nov 29, 2022 at 10:22:47PM -1000, Tejun Heo wrote:
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index a4a20046e586..08799b2a566e 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2193,6 +2193,8 @@ struct sched_class {
>  	 */
>  	void (*switched_from)(struct rq *this_rq, struct task_struct *task);
>  	void (*switched_to)  (struct rq *this_rq, struct task_struct *task);
> +	void (*reweight_task)(struct rq *this_rq, struct task_struct *task,
> +			      int newprio);
>  	void (*prio_changed) (struct rq *this_rq, struct task_struct *task,
>  			      int oldprio);

Hurmph.. this further propagate the existing problem of thinking that
'prio' is a useful concept in general (it isn't).

Yeah, you're just following precedent here, but still :/
Tejun Heo Dec. 12, 2022, 5:34 p.m. UTC | #2 
Hello,

On Mon, Dec 12, 2022 at 12:22:43PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 29, 2022 at 10:22:47PM -1000, Tejun Heo wrote:
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index a4a20046e586..08799b2a566e 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2193,6 +2193,8 @@ struct sched_class {
> >  	 */
> >  	void (*switched_from)(struct rq *this_rq, struct task_struct *task);
> >  	void (*switched_to)  (struct rq *this_rq, struct task_struct *task);
> > +	void (*reweight_task)(struct rq *this_rq, struct task_struct *task,
> > +			      int newprio);
> >  	void (*prio_changed) (struct rq *this_rq, struct task_struct *task,
> >  			      int oldprio);
> 
> Hurmph.. this further propagate the existing problem of thinking that
> 'prio' is a useful concept in general (it isn't).

I'm not quite following. Can you please expand on why prio isn't a generally
useful concept?

Thanks.
Peter Zijlstra Dec. 12, 2022, 8:11 p.m. UTC | #3 
On Mon, Dec 12, 2022 at 07:34:27AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Mon, Dec 12, 2022 at 12:22:43PM +0100, Peter Zijlstra wrote:
> > On Tue, Nov 29, 2022 at 10:22:47PM -1000, Tejun Heo wrote:
> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > index a4a20046e586..08799b2a566e 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -2193,6 +2193,8 @@ struct sched_class {
> > >  	 */
> > >  	void (*switched_from)(struct rq *this_rq, struct task_struct *task);
> > >  	void (*switched_to)  (struct rq *this_rq, struct task_struct *task);
> > > +	void (*reweight_task)(struct rq *this_rq, struct task_struct *task,
> > > +			      int newprio);
> > >  	void (*prio_changed) (struct rq *this_rq, struct task_struct *task,
> > >  			      int oldprio);
> > 
> > Hurmph.. this further propagate the existing problem of thinking that
> > 'prio' is a useful concept in general (it isn't).
> 
> I'm not quite following. Can you please expand on why prio isn't a generally
> useful concept?

The whole fixed vs dynamic priority scheduling thing. Specifically
SCHED_DEADLINE implements a dynamic priority scheme using the sporadic
task model which just doesn't map well to this single prio value
(notably every SCHED_DEADLINE task has prio -1, making it impossible to
order SCHED_DEADLINE tasks based on this).
Tejun Heo Dec. 12, 2022, 8:15 p.m. UTC | #4 
On Mon, Dec 12, 2022 at 09:11:36PM +0100, Peter Zijlstra wrote:
> > > Hurmph.. this further propagate the existing problem of thinking that
> > > 'prio' is a useful concept in general (it isn't).
> > 
> > I'm not quite following. Can you please expand on why prio isn't a generally
> > useful concept?
> 
> The whole fixed vs dynamic priority scheduling thing. Specifically
> SCHED_DEADLINE implements a dynamic priority scheme using the sporadic
> task model which just doesn't map well to this single prio value
> (notably every SCHED_DEADLINE task has prio -1, making it impossible to
> order SCHED_DEADLINE tasks based on this).

I see, so the expressive power isn't sufficient to cover some use cases.
Yeah, I mean, it's limited and we can add whatever that's necessary but
given that there are many use cases that make use of priorities, I don't
think SCX can skip supporting it.

Thanks.
diff mbox series

Patch

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 85eb82ad2ffd..70ec74dbb45a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1275,8 +1275,8 @@  static void set_load_weight(struct task_struct *p, bool update_load)
 	 * SCHED_OTHER tasks have to update their load when changing their
 	 * weight
 	 */
-	if (update_load && p->sched_class == &fair_sched_class) {
-		reweight_task(p, prio);
+	if (update_load && p->sched_class->reweight_task) {
+		p->sched_class->reweight_task(task_rq(p), p, prio);
 	} else {
 		load->weight = scale_load(sched_prio_to_weight[prio]);
 		load->inv_weight = sched_prio_to_wmult[prio];
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index e4a0b8bd941c..78263cef1ea8 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3330,7 +3330,7 @@  static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
 
 }
 
-void reweight_task(struct task_struct *p, int prio)
+static void reweight_task_fair(struct rq *rq, struct task_struct *p, int prio)
 {
 	struct sched_entity *se = &p->se;
 	struct cfs_rq *cfs_rq = cfs_rq_of(se);
@@ -12176,6 +12176,7 @@  DEFINE_SCHED_CLASS(fair) = {
 	.task_tick		= task_tick_fair,
 	.task_fork		= task_fork_fair,
 
+	.reweight_task		= reweight_task_fair,
 	.prio_changed		= prio_changed_fair,
 	.switched_from		= switched_from_fair,
 	.switched_to		= switched_to_fair,
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index a4a20046e586..08799b2a566e 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2193,6 +2193,8 @@  struct sched_class {
 	 */
 	void (*switched_from)(struct rq *this_rq, struct task_struct *task);
 	void (*switched_to)  (struct rq *this_rq, struct task_struct *task);
+	void (*reweight_task)(struct rq *this_rq, struct task_struct *task,
+			      int newprio);
 	void (*prio_changed) (struct rq *this_rq, struct task_struct *task,
 			      int oldprio);
 
@@ -2345,8 +2347,6 @@  extern void init_sched_dl_class(void);
 extern void init_sched_rt_class(void);
 extern void init_sched_fair_class(void);
 
-extern void reweight_task(struct task_struct *p, int prio);
-
 extern void resched_curr(struct rq *rq);
 extern void resched_cpu(int cpu);