Re: [PATCH 3/3] sched, sched_ext: Move some declarations from kernel/sched/ext.h to sched.h

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

On Sat, Jun 22, 2024 at 03:50:22PM -1000, Tejun Heo wrote:

Hello Tejun,

> While sched_ext was out of tree, everything sched_ext specific which can be
> put in kernel/sched/ext.h was put there to ease forward porting. However,
> kernel/sched/sched.h is the better location for some of them. Relocate.
> 
> - struct sched_enq_and_set_ctx, sched_deq_and_put_task() and
>   sched_enq_and_set_task().
> 
> - scx_enabled() and scx_switched_all().
> 
> - for_active_class_range() and for_each_active_class(). sched_class
>   declarations are moved above the class iterators for this.
> 
> No functional changes intended.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: David Vernet <void@manifault.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---

[...]

Bringing everything above this into sched.h seems reasonable. See below for
some thoughts about sched_deq_and_put_task() and sched_enq_and_set_task().

>  
>  static inline bool sched_stop_runnable(struct rq *rq)
>  {
> @@ -3698,6 +3733,24 @@ static inline void balance_callbacks(struct rq *rq, struct balance_callback *hea
>  
>  #endif
>  
> +#ifdef CONFIG_SCHED_CLASS_EXT
> +/*
> + * Used by SCX in the enable/disable paths to move tasks between sched_classes
> + * and establish invariants.
> + */
> +struct sched_enq_and_set_ctx {
> +	struct task_struct	*p;
> +	int			queue_flags;
> +	bool			queued;
> +	bool			running;
> +};
> +
> +void sched_deq_and_put_task(struct task_struct *p, int queue_flags,
> +			    struct sched_enq_and_set_ctx *ctx);
> +void sched_enq_and_set_task(struct sched_enq_and_set_ctx *ctx);

I'm not opposed to bringing these into sched.h, but they're implementing a
pattern that's used almost verbatim in a few other places in core.c, so it
would be nice if we could figure out some way to just have every caller use the
same API for doing the whole dequeue/save -> update state -> re-enqueue/restore
dance. We had SCHED_CHANGE_BLOCK [0] way back in v2, but IIRC we dropped it
because upstream was moving towards a more generic implementation. It looks
like that hasn't materialized yet, so maybe we should resurrect that?

[0]: https://lore.kernel.org/all/20230128001639.3510083-3-tj@kernel.org/

Anyways, no objection from me in moving this into sched.h (as long as Peter,
Thomas and Ingo are OK with it). But it does feel kind of unfortunate to stick
it there in its current form given that it's only used by ext.c, despite being
95% of the way there for quite a few potential callsites in core.c.

Thanks,
David

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]