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: <e7d56d70-3750-b83a-8c1c-99878722c805@gnuweeb.org>
Date:   Mon, 24 Jan 2022 17:38:21 +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: 7bit
Precedence: bulk


[ I resend and fix my reply, my previous reply seems to have an
   issue with the "Date" ]

Hi Zqiang,

On Mon, 24 Jan 2022 18:36:37 +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