Re: [PATCH 4/5] sched_ext: Drop "ops" from scx_ops_exit(), scx_ops_error() and friends

[Andrea Righi]

On Fri, Apr 04, 2025 at 09:10:58AM -1000, Tejun Heo wrote:
> Hello, Andrea.
> 
> On Fri, Apr 04, 2025 at 09:30:23AM +0200, Andrea Righi wrote:
> > > @@ -1043,18 +1043,17 @@ static struct kobject *scx_root_kobj;
> > >  
> > >  static void process_ddsp_deferred_locals(struct rq *rq);
> > >  static void scx_bpf_kick_cpu(s32 cpu, u64 flags);
> > > -static __printf(3, 4) void scx_ops_exit_kind(enum scx_exit_kind kind,
> > > -					     s64 exit_code,
> > > -					     const char *fmt, ...);
> > > +static __printf(3, 4) void __scx_exit(enum scx_exit_kind kind, s64 exit_code,
> > > +				      const char *fmt, ...);
> > >  
> > > -#define scx_ops_error_kind(err, fmt, args...)					\
> > > -	scx_ops_exit_kind((err), 0, fmt, ##args)
> > > +#define __scx_error(err, fmt, args...)						\
> > > +	__scx_exit((err), 0, fmt, ##args)
> > >  
> > 
> > Can we move scx_error() here, right after __scx_error(), for better
> > readability?
> 
> The current order is:
> 
>   __scx_exit()
>   __scx_error()
>   scx_exit()
>   scx_error()

I see, that's because there's __scx_exit() function definition before the
macros. It's a bit difficult to read, but not the end of the world.

> 
> If relocated as you suggested:
> 
>   __scx_exit()
>   __scx_error()
>   scx_error()
>   scx_exit()
> 
> which probably isn't optimal. We can do:
> 
>   __scx_exit()
>   scx_exit()
>   __scx_error()
>   scx_error()
> 
> Maybe that's a bit better. I don't know.

This looks better, but I'm just nitpicking here, so feel free to ignore
this.

> 
> > > -#define scx_ops_exit(code, fmt, args...)					\
> > > -	scx_ops_exit_kind(SCX_EXIT_UNREG_KERN, (code), fmt, ##args)
> > > +#define scx_exit(code, fmt, args...)						\
> > > +	__scx_exit(SCX_EXIT_UNREG_KERN, (code), fmt, ##args)
> > >  
> > > -#define scx_ops_error(fmt, args...)						\
> > > -	scx_ops_error_kind(SCX_EXIT_ERROR, fmt, ##args)
> > > +#define scx_error(fmt, args...)							\
> > > +	__scx_error(SCX_EXIT_ERROR, fmt, ##args)
> > 
> > I've always found scx_exit_kind / exit_code a bit confusing, scx_exit_kind
> > represents the reason of the exit, while exit_code is an additional code to
> > describe the error.
> > 
> > Not necessarily for this patch set, but what do you think about renaming
> > scx_exit_kind to scx_exit_reason and scx_exit_reason() to
> > scx_exit_reason_str()?
> 
> Even if those are better names, the enum is exposed and used from the
> schedulers and renaming it would need to go through compatibility process.
> I'm not sure the gain here would be justified.
> 
> We basically have two levels of exit descriptors. We call the higher level
> kind and lower level code. I think having two exit descriptors is going to
> be a bit confusing no matter what we do and maybe we should have stuck to
> one value. Anyways, here, I'm not sure the return would justify the cost.

Yeah, I wasn't considering that it's part of the BPF API, definitely not
worth changing it. Let's keep it as it is, maybe we can add a comment later
to scx_exit_kind to better explain the difference with the exit code.

Thanks,
-Andrea