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]

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


[ 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@...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