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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240623074842.GD6519@maniforge>
Date: Sun, 23 Jun 2024 02:48:42 -0500
From: David Vernet <void@...ifault.com>
To: Tejun Heo <tj@...nel.org>
Cc: torvalds@...ux-foundation.org, mingo@...hat.com, peterz@...radead.org,
	tglx@...utronix.de, linux-kernel@...r.kernel.org,
	kernel-team@...a.com
Subject: Re: [PATCH 3/3] sched, sched_ext: Move some declarations from
 kernel/sched/ext.h to sched.h

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@...nel.org>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: David Vernet <void@...ifault.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Thomas Gleixner <tglx@...utronix.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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)