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

[Ammar Faizi <ammarfaizi2@gnuweeb.org>]

Hi Zqiang,

On Sat, 22 Jan 2022 02:30:00 +0800, Zqiang <qiang1.zhang@intel.com> 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