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;
Date:   Mon, 24 Jan 2022 08:42:51 -0800
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Ammar Faizi <ammarfaizi2@gnuweeb.org>
Cc:     Zqiang <qiang1.zhang@intel.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] rcu: Add per-CPU rcuc task info to RCU CPU stall
 warnings
Message-ID: <20220124164251.GF4285@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220124103637.4001386-1-qiang1.zhang@intel.com>
 <e7d56d70-3750-b83a-8c1c-99878722c805@gnuweeb.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e7d56d70-3750-b83a-8c1c-99878722c805@gnuweeb.org>
Precedence: bulk

On Mon, Jan 24, 2022 at 05:38:21PM +0700, Ammar Faizi wrote:
> 
> [ 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);

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

> 	 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