Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <ec15ce1f-4766-6ccc-97d2-980c8ea183a7@gnuweeb.org>
Date:   Mon, 24 Jan 2022 17:03:58 +0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v2] rcu: Add per-CPU rcuc task info to RCU CPU stall
 warnings
Content-Language: en-US
To:     Zqiang <qiang1.zhang@intel.com>, paulmck@kernel.org
Cc:     linux-kernel@vger.kernel.org
References: <20220124103637.4001386-1-qiang1.zhang@intel.com>
From:   Ammar Faizi <ammarfaizi2@gnuweeb.org>
In-Reply-To: <20220124103637.4001386-1-qiang1.zhang@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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