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

[Tejun Heo <tj@kernel.org>]

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()

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.

> > -#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.

Thanks.

-- 
tejun