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

[Ammar Faizi]

Hi Zqiang,

On Sat, 22 Jan 2022 02:30:00 +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