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: <3f08b8cf-a1dc-4d83-0de2-94203dff9a4c@gnuweeb.org>
Date:   Tue, 25 Jan 2022 05:08:29 +0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2] rcu: Add per-CPU rcuc task info to RCU CPU stall
 warnings
Content-Language: en-US
To:     paulmck@kernel.org
Cc:     Zqiang <qiang1.zhang@intel.com>, linux-kernel@vger.kernel.org
References: <20220124103637.4001386-1-qiang1.zhang@intel.com>
 <e7d56d70-3750-b83a-8c1c-99878722c805@gnuweeb.org>
 <20220124164251.GF4285@paulmck-ThinkPad-P17-Gen-1>
From:   Ammar Faizi <ammarfaizi2@gnuweeb.org>
In-Reply-To: <20220124164251.GF4285@paulmck-ThinkPad-P17-Gen-1>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/24/22 11:42 PM, Paul E. McKenney wrote:
> On Mon, Jan 24, 2022 at 05:38:21PM +0700, Ammar Faizi wrote:
>> [snip...]
>> 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);
> 
> Thank you for looking this over and for the great feedback, Ammar!
> 
> I am also wondering why the above message should be printed when the
> corresponding CPU is offline or idle.  Why not move the above pr_err()
> line down to replace the pr_err() line below?
> 
> 							Thanx, Paul

Hi Paul, Thank you for the review. Agree with that.
Hopefully this one looks better (untested):
```
static void rcuc_kthread_dump(struct rcu_data *rdp)
{
	int cpu;
	unsigned long j;
	struct task_struct *rcuc;

	rcuc = rdp->rcu_cpu_kthread_task;
	if (!rcuc)
		return;

	cpu = task_cpu(rcuc);
	if (cpu_is_offline(cpu) || idle_cpu(cpu))
		return;

	if (!rcu_is_rcuc_kthread_starving(rdp, &j))
		return;

	pr_err("%s kthread starved for %ld jiffies\n", rcuc->comm, j);
	sched_show_task(rcuc);
	if (!trigger_single_cpu_backtrace(cpu))
		dump_cpu_task(cpu);
}
```

Recall that dump_cpu_task looks like this:
```
void dump_cpu_task(int cpu)
{
	pr_info("Task dump for CPU %d:\n", cpu);
	sched_show_task(cpu_curr(cpu));
}
```
which already tells us it's a dump, so "stack dump" in the pr_err()
can be omitted. Any comment, Zqiang?

-- 
Ammar Faizi