Messages in this thread | | | Date | Mon, 24 Jan 2022 17:03:58 +0700 | Subject | Re: [PATCH v2] rcu: Add per-CPU rcuc task info to RCU CPU stall warnings | From | Ammar Faizi <> |
| |
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
| |