[PATCH v2] rcu: Add nocb_cb_kthread check to rcu_is_callbacks_kthread()

[PATCH v2] rcu: Add nocb_cb_kthread check to rcu_is_callbacks_kthread()

[Zqiang]

At present, there are two situations which the rcu callback function
be exectued in the kthreads, one is if the use_softirq is set to zero,
the RCU_SOFTIRQ processing is carried out by the per-CPU rcuc kthreads,
for non-offload rdp, the rdp's rcu callback function be exectued in rcuc
kthreads. another one is if the rdp is set to offloaded, the rdp's rcu
callback function be exectued in the rcuop kthreads.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Zqiang <qiang1.zhang@intel.com>
---
 v1->v2:
 fix compilation error when CONFIG_RCU_NOCB_CPU is no define

 kernel/rcu/tree.c        |  4 ++--
 kernel/rcu/tree.h        |  2 +-
 kernel/rcu/tree_plugin.h | 12 +++++++++---
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 5c587e00811c..9dc4c4e82db6 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2610,7 +2610,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
 		trace_rcu_batch_end(rcu_state.name, 0,
 				    !rcu_segcblist_empty(&rdp->cblist),
 				    need_resched(), is_idle_task(current),
-				    rcu_is_callbacks_kthread());
+				    rcu_is_callbacks_kthread(rdp));
 		return;
 	}
 
@@ -2688,7 +2688,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
 	rcu_nocb_lock_irqsave(rdp, flags);
 	rdp->n_cbs_invoked += count;
 	trace_rcu_batch_end(rcu_state.name, count, !!rcl.head, need_resched(),
-			    is_idle_task(current), rcu_is_callbacks_kthread());
+			    is_idle_task(current), rcu_is_callbacks_kthread(rdp));
 
 	/* Update counts and requeue any remaining callbacks. */
 	rcu_segcblist_insert_done_cbs(&rdp->cblist, &rcl);
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index 996387962de3..3cdc18997a38 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -433,7 +433,7 @@ static void rcu_flavor_sched_clock_irq(int user);
 static void dump_blkd_tasks(struct rcu_node *rnp, int ncheck);
 static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags);
 static void rcu_preempt_boost_start_gp(struct rcu_node *rnp);
-static bool rcu_is_callbacks_kthread(void);
+static bool rcu_is_callbacks_kthread(struct rcu_data *rdp);
 static void rcu_cpu_kthread_setup(unsigned int cpu);
 static void rcu_spawn_one_boost_kthread(struct rcu_node *rnp);
 static bool rcu_preempt_has_tasks(struct rcu_node *rnp);
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 971bb6a00ede..120bade40e02 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -1155,9 +1155,15 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
  * Is the current CPU running the RCU-callbacks kthread?
  * Caller must have preemption disabled.
  */
-static bool rcu_is_callbacks_kthread(void)
+static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
 {
-	return __this_cpu_read(rcu_data.rcu_cpu_kthread_task) == current;
+	bool ret;
+#ifdef CONFIG_RCU_NOCB_CPU
+	ret = rdp->rcu_cpu_kthread_task == current || rdp->nocb_cb_kthread == current;
+#else
+	ret = rdp->rcu_cpu_kthread_task == current;
+#endif
+	return ret;
 }
 
 #define RCU_BOOST_DELAY_JIFFIES DIV_ROUND_UP(CONFIG_RCU_BOOST_DELAY * HZ, 1000)
@@ -1242,7 +1248,7 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
 	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 }
 
-static bool rcu_is_callbacks_kthread(void)
+static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
 {
 	return false;
 }
-- 
2.25.1

Re: [PATCH v2] rcu: Add nocb_cb_kthread check to rcu_is_callbacks_kthread()

[Paul E. McKenney]

On Tue, Apr 26, 2022 at 03:00:31PM +0800, Zqiang wrote:
> At present, there are two situations which the rcu callback function
> be exectued in the kthreads, one is if the use_softirq is set to zero,
> the RCU_SOFTIRQ processing is carried out by the per-CPU rcuc kthreads,
> for non-offload rdp, the rdp's rcu callback function be exectued in rcuc
> kthreads. another one is if the rdp is set to offloaded, the rdp's rcu
> callback function be exectued in the rcuop kthreads.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

Good catch!

From what I can see, this affects only tracing.  Or did I miss a
use case?

Also, should the two definitions of rcu_is_callbacks_kthread() be merged?
It looks like the current situation dates back to when the only time
rcuc kthreads were created was in kernels built with CONFIG_RCU_BOOST=y.
If so, the definition of this function should move from tree_plugin.h
to tree.c.

							Thanx, Paul

> ---
>  v1->v2:
>  fix compilation error when CONFIG_RCU_NOCB_CPU is no define
> 
>  kernel/rcu/tree.c        |  4 ++--
>  kernel/rcu/tree.h        |  2 +-
>  kernel/rcu/tree_plugin.h | 12 +++++++++---
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 5c587e00811c..9dc4c4e82db6 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2610,7 +2610,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  		trace_rcu_batch_end(rcu_state.name, 0,
>  				    !rcu_segcblist_empty(&rdp->cblist),
>  				    need_resched(), is_idle_task(current),
> -				    rcu_is_callbacks_kthread());
> +				    rcu_is_callbacks_kthread(rdp));
>  		return;
>  	}
>  
> @@ -2688,7 +2688,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  	rcu_nocb_lock_irqsave(rdp, flags);
>  	rdp->n_cbs_invoked += count;
>  	trace_rcu_batch_end(rcu_state.name, count, !!rcl.head, need_resched(),
> -			    is_idle_task(current), rcu_is_callbacks_kthread());
> +			    is_idle_task(current), rcu_is_callbacks_kthread(rdp));
>  
>  	/* Update counts and requeue any remaining callbacks. */
>  	rcu_segcblist_insert_done_cbs(&rdp->cblist, &rcl);
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 996387962de3..3cdc18997a38 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -433,7 +433,7 @@ static void rcu_flavor_sched_clock_irq(int user);
>  static void dump_blkd_tasks(struct rcu_node *rnp, int ncheck);
>  static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags);
>  static void rcu_preempt_boost_start_gp(struct rcu_node *rnp);
> -static bool rcu_is_callbacks_kthread(void);
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp);
>  static void rcu_cpu_kthread_setup(unsigned int cpu);
>  static void rcu_spawn_one_boost_kthread(struct rcu_node *rnp);
>  static bool rcu_preempt_has_tasks(struct rcu_node *rnp);
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 971bb6a00ede..120bade40e02 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1155,9 +1155,15 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
>   * Is the current CPU running the RCU-callbacks kthread?
>   * Caller must have preemption disabled.
>   */
> -static bool rcu_is_callbacks_kthread(void)
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
>  {
> -	return __this_cpu_read(rcu_data.rcu_cpu_kthread_task) == current;
> +	bool ret;
> +#ifdef CONFIG_RCU_NOCB_CPU
> +	ret = rdp->rcu_cpu_kthread_task == current || rdp->nocb_cb_kthread == current;
> +#else
> +	ret = rdp->rcu_cpu_kthread_task == current;
> +#endif

The problem here is that the first part of that condition is duplicated.
This is an accident waiting to happen when someone fixes one side of that
#ifdef without also adjusting the other side.  One approach is to define a
function that tests "nocb_cb_kthread == current" in CONFIG_RCU_NOCB_CPU=y
kernels and just returns "false" otherwise.

Alternatively, you could make ->nocb_cb_kthread defined unconditionally,
but left always NULL in CONFIG_RCU_NOCB_CPU=n kernels.

> +	return ret;
>  }
>  
>  #define RCU_BOOST_DELAY_JIFFIES DIV_ROUND_UP(CONFIG_RCU_BOOST_DELAY * HZ, 1000)
> @@ -1242,7 +1248,7 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
>  }
>  
> -static bool rcu_is_callbacks_kthread(void)
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
>  {
>  	return false;
>  }
> -- 
> 2.25.1
> 

RE: [PATCH v2] rcu: Add nocb_cb_kthread check to rcu_is_callbacks_kthread()

[Zhang, Qiang1]

On Tue, Apr 26, 2022 at 03:00:31PM +0800, Zqiang wrote:
> At present, there are two situations which the rcu callback function 
> be exectued in the kthreads, one is if the use_softirq is set to zero, 
> the RCU_SOFTIRQ processing is carried out by the per-CPU rcuc 
> kthreads, for non-offload rdp, the rdp's rcu callback function be 
> exectued in rcuc kthreads. another one is if the rdp is set to 
> offloaded, the rdp's rcu callback function be exectued in the rcuop kthreads.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

>Good catch!
>
>From what I can see, this affects only tracing.  Or did I miss a use case?

Yes it is affects tracing, it was add make tracing info more accurate.

>
>Also, should the two definitions of rcu_is_callbacks_kthread() be merged?
>It looks like the current situation dates back to when the only time rcuc kthreads were created was in kernels built with CONFIG_RCU_BOOST=y.
>If so, the definition of this function should move from tree_plugin.h to tree.c.

Yes it should not depend on CONFIG_RCU_BOOST option.

>
>							Thanx, Paul

> ---
>  v1->v2:
>  fix compilation error when CONFIG_RCU_NOCB_CPU is no define
> 
>  kernel/rcu/tree.c        |  4 ++--
>  kernel/rcu/tree.h        |  2 +-
>  kernel/rcu/tree_plugin.h | 12 +++++++++---
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 
> 5c587e00811c..9dc4c4e82db6 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2610,7 +2610,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  		trace_rcu_batch_end(rcu_state.name, 0,
>  				    !rcu_segcblist_empty(&rdp->cblist),
>  				    need_resched(), is_idle_task(current),
> -				    rcu_is_callbacks_kthread());
> +				    rcu_is_callbacks_kthread(rdp));
>  		return;
>  	}
>  
> @@ -2688,7 +2688,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  	rcu_nocb_lock_irqsave(rdp, flags);
>  	rdp->n_cbs_invoked += count;
>  	trace_rcu_batch_end(rcu_state.name, count, !!rcl.head, need_resched(),
> -			    is_idle_task(current), rcu_is_callbacks_kthread());
> +			    is_idle_task(current), rcu_is_callbacks_kthread(rdp));
>  
>  	/* Update counts and requeue any remaining callbacks. */
>  	rcu_segcblist_insert_done_cbs(&rdp->cblist, &rcl); diff --git 
> a/kernel/rcu/tree.h b/kernel/rcu/tree.h index 
> 996387962de3..3cdc18997a38 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -433,7 +433,7 @@ static void rcu_flavor_sched_clock_irq(int user);  
> static void dump_blkd_tasks(struct rcu_node *rnp, int ncheck);  static 
> void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags);  
> static void rcu_preempt_boost_start_gp(struct rcu_node *rnp); -static 
> bool rcu_is_callbacks_kthread(void);
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp);
>  static void rcu_cpu_kthread_setup(unsigned int cpu);  static void 
> rcu_spawn_one_boost_kthread(struct rcu_node *rnp);  static bool 
> rcu_preempt_has_tasks(struct rcu_node *rnp); diff --git 
> a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h index 
> 971bb6a00ede..120bade40e02 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1155,9 +1155,15 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
>   * Is the current CPU running the RCU-callbacks kthread?
>   * Caller must have preemption disabled.
>   */
> -static bool rcu_is_callbacks_kthread(void)
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
>  {
> -	return __this_cpu_read(rcu_data.rcu_cpu_kthread_task) == current;
> +	bool ret;
> +#ifdef CONFIG_RCU_NOCB_CPU
> +	ret = rdp->rcu_cpu_kthread_task == current || rdp->nocb_cb_kthread 
> +== current; #else
> +	ret = rdp->rcu_cpu_kthread_task == current; #endif
>
>The problem here is that the first part of that condition is duplicated.
>This is an accident waiting to happen when someone fixes one side of that #ifdef without also adjusting the other side.  One approach is to define a function that tests "nocb_cb_kthread == current" in CONFIG_RCU_NOCB_CPU=y kernels and just returns "false" otherwise.

Thanks for your suggestion, I will resend.

>
>Alternatively, you could make ->nocb_cb_kthread defined unconditionally, but left always NULL in CONFIG_RCU_NOCB_CPU=n kernels.

> +	return ret;
>  }
>  
>  #define RCU_BOOST_DELAY_JIFFIES DIV_ROUND_UP(CONFIG_RCU_BOOST_DELAY * 
> HZ, 1000) @@ -1242,7 +1248,7 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);  }
>  
> -static bool rcu_is_callbacks_kthread(void)
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
>  {
>  	return false;
>  }
> --
> 2.25.1
>