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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Jan 2022 17:03:58 +0700
From:   Ammar Faizi <ammarfaizi2@...weeb.org>
To:     Zqiang <qiang1.zhang@...el.com>, paulmck@...nel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rcu: Add per-CPU rcuc task info to RCU CPU stall
 warnings

Hi Zqiang,

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