Re: [PATCH sched_ext/for-6.12] sched_ext: Improve logging around enable/disable

From: David Vernet
Date: Thu Aug 08 2024 - 13:00:48 EST


On Wed, Aug 07, 2024 at 02:52:50PM -1000, Tejun Heo wrote:

Hi Tejun,

> sched_ext currently doesn't generate messages when the BPF scheduler is
> enabled and disabled unless there are errors. It is useful to have paper
> trail. Improve logging around enable/disable:
>
> - Generate info messages on enable and non-error disable.
>
> - Update error exit message formatting so that it's consistent with
> non-error message. Also, prefix ei->msg with the BPF scheduler's name to
> make it clear where the message is coming from.
>
> - Shorten scx_exit_reason() strings for SCX_EXIT_UNREG* for brevity and
> consistency.
>
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Suggested-by: Phil Auld <pauld@xxxxxxxxxx>
> ---
> kernel/sched/ext.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> index 60a7eb7d8a9e..eea2fda8e099 100644
> --- a/kernel/sched/ext.c
> +++ b/kernel/sched/ext.c
> @@ -4004,11 +4004,11 @@ static const char *scx_exit_reason(enum scx_exit_kind kind)
> {
> switch (kind) {
> case SCX_EXIT_UNREG:
> - return "Scheduler unregistered from user space";
> + return "unregistered from user space";
> case SCX_EXIT_UNREG_BPF:
> - return "Scheduler unregistered from BPF";
> + return "unregistered from BPF";
> case SCX_EXIT_UNREG_KERN:
> - return "Scheduler unregistered from the main kernel";
> + return "unregistered from the main kernel";
> case SCX_EXIT_SYSRQ:
> return "disabled by sysrq-S";
> case SCX_EXIT_ERROR:
> @@ -4126,14 +4126,17 @@ static void scx_ops_disable_workfn(struct kthread_work *work)
> percpu_up_write(&scx_fork_rwsem);
>
> if (ei->kind >= SCX_EXIT_ERROR) {
> - printk(KERN_ERR "sched_ext: BPF scheduler \"%s\" errored, disabling\n", scx_ops.name);
> + pr_err("sched_ext: BPF scheduler \"%s\" disabled (%s)\n",
> + scx_ops.name, ei->reason);
>
> - if (ei->msg[0] == '\0')
> - printk(KERN_ERR "sched_ext: %s\n", ei->reason);
> - else
> - printk(KERN_ERR "sched_ext: %s (%s)\n", ei->reason, ei->msg);
> + if (ei->msg[0] != '\0')
> + printk(KERN_ERR "sched_ext: %s: %s\n",
> + scx_ops.name, ei->msg);

Was it intentional to use pr_err() above, and printk() here? If not,
let's make them consistent?

Looks good otherwise.

Acked-by: David Vernet <void@xxxxxxxxxxxxx>

Thanks,
David

Attachment: signature.asc
Description: PGP signature