Re: [PATCH v2] rcu: Add per-CPU rcuc task info to RCU CPU stall warnings

[Ammar Faizi]

[ I resend and fix my reply, my previous reply seems to have an
  issue with the "Date" ]

Hi Zqiang,

On Mon, 24 Jan 2022 18:36:37 +0800, Zqiang <qiang1.zhang@xxxxxxxxx> wrote:> +static void rcuc_kthread_dump(struct rcu_data *rdp)
> +{
> +	int cpu;
> +	unsigned long j;
> +	struct task_struct *rcuc = rdp->rcu_cpu_kthread_task;
> +
> +	if (rcu_is_rcuc_kthread_starving(rdp, &j)) {
> +		cpu = rcuc ? task_cpu(rcuc) : -1;
> +
> +		if (rcuc) {
> +			pr_err("%s kthread starved for %ld jiffies, stack dump:\n",
> +									rcuc->comm, j);
> +			sched_show_task(rcuc);
> +			if (cpu >= 0) {
> +				if (cpu_online(cpu) && !idle_cpu(cpu)) {
> +					pr_err("Dump current CPU stack:\n");
> +					if (!trigger_single_cpu_backtrace(cpu))
> +						dump_cpu_task(cpu);
> +				}
> +			}
> +		}
> +	}
> +}

1) We can reduce the nested if with an early return after
   checking `rcu_is_rcuc_kthread_starving()`.

2) This ternary operator doesn't make sense:

   `cpu = rcuc ? task_cpu(rcuc) : -1;`

   If `rcuc` is NULL, then the "if (rcuc)" block will never
   be executed, and `cpu` variable won't be used, why should
   we perform a conditional with ternary to assign -1 here?

3) We can use an early return as well for the `if (rcuc)` to
   avoid more nested if.

FWIW, this one makes more sense:
```
static void rcuc_kthread_dump(struct rcu_data *rdp)
{
	 int cpu;
	 unsigned long j;
	 struct task_struct *rcuc;

	 if (!rcu_is_rcuc_kthread_starving(rdp, &j))
		 return;

	 rcuc = rdp->rcu_cpu_kthread_task;
	 if (!rcuc)
		 return;

	 pr_err("%s kthread starved for %ld jiffies, stack dump:\n", rcuc->comm, j);
	 sched_show_task(rcuc);
	 cpu = task_cpu(rcuc);
	 if (cpu_online(cpu) && !idle_cpu(cpu)) {
		 pr_err("Dump current CPU stack:\n");
		 if (!trigger_single_cpu_backtrace(cpu))
			 dump_cpu_task(cpu);
	 }
}
```

Thank you!

--
Ammar Faizi