Re: [PATCH] rcu: Print CPU that preempted stalled reader wants to run on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] rcu: unused variable th in check_slow_task() causes W=1 build
  warning
- [Low] sched: cpu_curr_snapshot_rcu() declared outside _LINUX_SCHED_H
  guard
--

commit e43a0801750c8342c9f97761eb87e99e324c35d8
Author: Paul E. McKenney <paulmck@kernel.org>

rcu: Print CPU that preempted stalled reader wants to run on

This commit adds cpu_curr_snapshot_rcu() and uses it in RCU CPU stall
warnings to report the CPU and currently running task for each stalled
RCU reader task, while dropping the get_task_struct() and
put_task_struct() calls in rcu_print_task_stall().

Link: https://lore.kernel.org/all/a7a21bf3-a7d5-45bb-be4f-0119fe53fa20@paulmck-laptop/
Link: https://lore.kernel.org/all/e8b6b372-d16b-4939-aa7a-755b7149e0fb@paulmck-laptop/
Link: https://lore.kernel.org/all/22d0d56c-6cef-47d7-a923-1d888e224b58@paulmck-laptop/

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index e4f6323d78555..09e05e2b6c9aa 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2529,3 +2529,5 @@ extern void migrate_enable(void);
>  DEFINE_LOCK_GUARD_0(migrate, migrate_disable(), migrate_enable())
>  
>  #endif
> +
> +struct task_struct *cpu_curr_snapshot_rcu(int cpu);

[Severity: Low]
As reported by K Prateek Nayak on v1 [1], should cpu_curr_snapshot_rcu()
be declared inside the _LINUX_SCHED_H header guard next to
cpu_curr_snapshot()?  Placing it after the closing #endif causes the
prototype to be re-parsed on every inclusion of linux/sched.h and
disables the preprocessor's multiple-include optimization for this
header.

> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
> index 803a56af3259f..08699652f51c2 100644
> --- a/kernel/rcu/tree_stall.h
> +++ b/kernel/rcu/tree_stall.h
[ ... ]
> @@ -317,8 +320,10 @@ struct rcu_stall_chk_rdr {
>   */
>  static int check_slow_task(struct task_struct *t, void *arg)
>  {
> +	int cpu;
>  	struct rcu_node *rnp;
>  	struct rcu_stall_chk_rdr *rscrp = arg;
> +	struct task_struct *th;
>  
>  	if (task_curr(t))
>  		return -EBUSY; // It is running, so decline to inspect it.
> @@ -327,6 +332,13 @@ static int check_slow_task(struct task_struct *t, void *arg)
>  	rscrp->on_blkd_list = !list_empty(&t->rcu_node_entry);
>  	rscrp->rcu_rdr_running = task_curr(t);
>  	rscrp->rcu_rdr_boosted = 0;
> +	rscrp->rcu_rdr_cpu = cpu = task_cpu(t);
> +	rscrp->rcu_rdr_cpu_offline = cpu_is_offline(cpu);
> +	lockdep_assert_irqs_disabled();
> +	rscrp->rcu_rdr_cpu_task = th = cpu_curr_snapshot_rcu(cpu);

[Severity: Low]
Can the local variable th be dropped here?  It is never read after this
assignment, which triggers a -Wunused-but-set-variable warning under W=1.

[1] https://lore.kernel.org/r/0d0974b0-5235-4764-abad-0a665c27a1d0@amd.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ba4a68a7-3656-434c-a51d-5c312c0924c0@paulmck-laptop?part=1