Re: [PATCH 2/2] sched_ext: Dump the stall CPU first in watchdog exit

[Changwoo Min <changwoo@igalia.com>]

Hi Andrea,


On 4/9/26 2:44 PM, Andrea Righi wrote:
> Hi Changwoo,
> 
> On Wed, Apr 08, 2026 at 12:11:13PM +0900, Changwoo Min wrote:
>> When a watchdog timeout fires, the CPU where the stalled task was
>> running is the most relevant piece of information for diagnosing the
>> hang. However, if there are many CPUs, the dump can get truncated and
>> the stall CPU's information may not appear in the output.
>>
>> Add a stall_cpu field to scx_exit_info, thread it through scx_vexit()
>> and __scx_exit(), and populate it from cpu_of(rq) in
>> check_rq_for_timeouts(). In scx_dump_state(), dump the stall CPU
>> before iterating the rest so it always appears at the top of the output.
>>
>> Introduce a scx_exit() macro that wraps __scx_exit() with stall_cpu=0
>> for all non-stall exit paths, keeping call sites unchanged.
> 
> Should we use stall_cpu = -1 as a sentinel to represent "no stall"?

I think initializing stall_cpu to 0 has advantages: since CPU 0 cannot
be turned off and is always valid, even if a task stall didn’t happen,
the first scx_dump_cpu() call is always valid.

> 
>>
>> Signed-off-by: Changwoo Min <changwoo@igalia.com>
>> ---
>>   kernel/sched/ext.c          | 31 ++++++++++++++++++++-----------
>>   kernel/sched/ext_internal.h |  3 +++
>>   2 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
>> index 8f7d5c1556be..671a1713aedb 100644
>> --- a/kernel/sched/ext.c
>> +++ b/kernel/sched/ext.c
>> @@ -200,24 +200,28 @@ static bool task_dead_and_done(struct task_struct *p);
>>   static void scx_kick_cpu(struct scx_sched *sch, s32 cpu, u64 flags);
>>   static void scx_disable(struct scx_sched *sch, enum scx_exit_kind kind);
>>   static bool scx_vexit(struct scx_sched *sch, enum scx_exit_kind kind,
>> -		      s64 exit_code, const char *fmt, va_list args);
>> +		      s64 exit_code, int stall_cpu, const char *fmt,
>> +		      va_list args);
>>   
>> -static __printf(4, 5) bool scx_exit(struct scx_sched *sch,
>> -				    enum scx_exit_kind kind, s64 exit_code,
>> -				    const char *fmt, ...)
>> +static __printf(5, 6) bool __scx_exit(struct scx_sched *sch,
>> +				      enum scx_exit_kind kind, s64 exit_code,
>> +				      int stall_cpu, const char *fmt, ...)
>>   {
>>   	va_list args;
>>   	bool ret;
>>   
>>   	va_start(args, fmt);
>> -	ret = scx_vexit(sch, kind, exit_code, fmt, args);
>> +	ret = scx_vexit(sch, kind, exit_code, stall_cpu, fmt, args);
>>   	va_end(args);
>>   
>>   	return ret;
>>   }
>>   
>> +#define scx_exit(sch, kind, exit_code, fmt, args...)				\
>> +					__scx_exit(sch, kind, exit_code, 0, fmt, ##args)
>> +
>>   #define scx_error(sch, fmt, args...)	scx_exit((sch), SCX_EXIT_ERROR, 0, fmt, ##args)
>> -#define scx_verror(sch, fmt, args)	scx_vexit((sch), SCX_EXIT_ERROR, 0, fmt, args)
>> +#define scx_verror(sch, fmt, args)	scx_vexit((sch), SCX_EXIT_ERROR, 0, 0, fmt, args)
>>   
>>   #define SCX_HAS_OP(sch, op)	test_bit(SCX_OP_IDX(op), (sch)->has_op)
>>   
>> @@ -3433,9 +3437,10 @@ static bool check_rq_for_timeouts(struct rq *rq)
>>   					last_runnable + READ_ONCE(sch->watchdog_timeout)))) {
>>   			u32 dur_ms = jiffies_to_msecs(jiffies - last_runnable);
>>   
>> -			scx_exit(sch, SCX_EXIT_ERROR_STALL, 0,
>> -				 "%s[%d] failed to run for %u.%03us",
>> -				 p->comm, p->pid, dur_ms / 1000, dur_ms % 1000);
>> +			__scx_exit(sch, SCX_EXIT_ERROR_STALL, 0, cpu_of(rq),
>> +				   "%s[%d] failed to run for %u.%03us",
>> +				   p->comm, p->pid, dur_ms / 1000,
>> +				   dur_ms % 1000);
>>   			timed_out = true;
>>   			break;
>>   		}
>> @@ -6337,8 +6342,11 @@ static void scx_dump_state(struct scx_sched *sch, struct scx_exit_info *ei,
>>   	dump_line(&s, "CPU states");
>>   	dump_line(&s, "----------");
>>   
>> +	/* Dump the stall CPU first, then dump the rest in order. */
>> +	scx_dump_cpu(sch, &s, &dctx, ei->stall_cpu, dump_all_tasks);
> 
> And here we can skip this if ei->stall_cpu < 0.

As mentioned earlier, even if stall_cpu is not set, scx_dump_cpu() for
CPU 0 is always valid, so the additional if statement is not necessary.
I can add comments here to avoid potential confusion in the future,
instead of initializing stall_cpu to -1. What do you think?

> 
>>   	for_each_possible_cpu(cpu) {
>> -		scx_dump_cpu(sch, &s, &dctx, cpu, dump_all_tasks);
>> +		if (cpu != ei->stall_cpu)
>> +			scx_dump_cpu(sch, &s, &dctx, cpu, dump_all_tasks);
>>   	}
>>   
>>   	dump_newline(&s);
>> @@ -6377,7 +6385,7 @@ static void scx_disable_irq_workfn(struct irq_work *irq_work)
>>   }
>>   
>>   static bool scx_vexit(struct scx_sched *sch,
>> -		      enum scx_exit_kind kind, s64 exit_code,
>> +		      enum scx_exit_kind kind, s64 exit_code, int stall_cpu,
>>   		      const char *fmt, va_list args)
>>   {
>>   	struct scx_exit_info *ei = sch->exit_info;
>> @@ -6400,6 +6408,7 @@ static bool scx_vexit(struct scx_sched *sch,
>>   	 */
>>   	ei->kind = kind;
>>   	ei->reason = scx_exit_reason(ei->kind);
>> +	ei->stall_cpu = stall_cpu;
>>   
>>   	irq_work_queue(&sch->disable_irq_work);
>>   	return true;
>> diff --git a/kernel/sched/ext_internal.h b/kernel/sched/ext_internal.h
>> index b4f36d8b9c1d..a0a09e8f2ac2 100644
>> --- a/kernel/sched/ext_internal.h
>> +++ b/kernel/sched/ext_internal.h
>> @@ -93,6 +93,9 @@ struct scx_exit_info {
>>   	/* %SCX_EXIT_* - broad category of the exit reason */
>>   	enum scx_exit_kind	kind;
>>   
>> +	/* CPU where a task stall happened. */
>> +	int			stall_cpu;
>> +
> 
> With CO-RE we shouldn't have any compatibility issue, but would it make sense to
> move this at the end of the struct anyway?

I wanted to fill the 4-byte hole between kind and exit_code. If there is
no compatibility issue, I would prefer to fill the hole and colocate the
high-level information, like kind and exit_code, together rather than
placing it after the messages. However, this is a matter of taste, so I
don’t have a strong opinion here.

Regards,
Changwoo Min
> 
>>   	/* exit code if gracefully exiting */
>>   	s64			exit_code;
>>   
>> -- 
>> 2.53.0
>>
> 
> Thanks,
> -Andrea
>