Re: [PATCH] rcu: Remove the unnecessary separate function, rcu_preempt_do_callback()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, Feb 27, 2018 at 08:40:47AM +0900, Byungchul Park wrote:
> On 2/27/2018 8:35 AM, Byungchul Park wrote:
> >On 2/27/2018 3:22 AM, Paul E. McKenney wrote:
> >>On Mon, Feb 26, 2018 at 12:15:14PM -0500, Steven Rostedt wrote:
> >>>On Mon, 26 Feb 2018 14:11:36 +0900
> >>>Byungchul Park <byungchul.park@lge.com> wrote:
> >>>
> >>>>rcu_preemptp_do_callback() was introduced in commit 09223371dea(rcu:
> >>>>Use softirq to address performance regression), where it had to be
> >>>>distinguished between in the case CONFIG_TREE_PREEMPT_RCU is set and
> >>>>it's not.
> >>>>
> >>>>Now that the code was cleaned up so that rcu_preemt_do_callback() is
> >>>>only called in rcu_kthread_do_work() in the same file, tree_plugin.h,
> >>>>we don't have to keep the separate function anymore. Remove it for a
> >>>>better readability.
> >>>
> >>>Looks good to me (looks like commit f8b7fc6b51 "rcu: use softirq
> >>>instead of kthreads except when RCU_BOOST=y" cleaned up the ifdefs and
> >>>removed the requirement).
> >>>
> >>>Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >>
> >>Thank you both!  I have queued a slightly modified patch for testing
> >>and further review.  Please see below and let me know if I messed
> >>something up.
> >>
> >>                            Thanx, Paul
> >>
> >>------------------------------------------------------------------------
> >>
> >>commit b8a3012ddba397d4a18d9fd4a00432f8c2626bd6
> >>Author: Byungchul Park <byungchul.park@lge.com>
> >>Date:   Mon Feb 26 14:11:36 2018 +0900
> >>
> >>     rcu: Inline rcu_preempt_do_callback() into its sole caller
> >>     The rcu_preempt_do_callbacks() function was introduced in commit
> >>     09223371dea(rcu: Use softirq to address performance
> >>regression), where it
> >>     was necessary to handle kernel builds both containing and
> >>not containing
> >>     RCU-preempt.  Since then, various changes (most notably f8b7fc6b51
> >>     ("rcu: use softirq instead of kthreads except when
> >>RCU_BOOST=y")) have
> >>     resulted in this function being invoked only from
> >>rcu_kthread_do_work(),
> >>     which is present only in kernels containing RCU-preempt,
> >>which in turn
> >>     means that the rcu_preempt_do_callbacks() function is no
> >>longer needed.
> >>     This commit therefore inlines rcu_preempt_do_callbacks() into its
> >>     sole remaining caller and also removes the rcu_state_p and
> >>rcu_data_p
> >>     indirection for added clarity.
> >>     Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> >>     Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> >>     [ paulmck: Remove the rcu_state_p and rcu_data_p indirection. ]
> >>
> >>diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> >>index dc6f2319fc21..9dd0ea77faed 100644
> >>--- a/kernel/rcu/tree.h
> >>+++ b/kernel/rcu/tree.h
> >>@@ -449,7 +449,6 @@ static void
> >>rcu_preempt_boost_start_gp(struct rcu_node *rnp);
> >>  static void invoke_rcu_callbacks_kthread(void);
> >>  static bool rcu_is_callbacks_kthread(void);
> >>  #ifdef CONFIG_RCU_BOOST
> >>-static void rcu_preempt_do_callbacks(void);
> >>  static int rcu_spawn_one_boost_kthread(struct rcu_state *rsp,
> >>                           struct rcu_node *rnp);
> >>  #endif /* #ifdef CONFIG_RCU_BOOST */
> >>diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> >>index 26d7a31e81cb..b0d7f9ba6bf2 100644
> >>--- a/kernel/rcu/tree_plugin.h
> >>+++ b/kernel/rcu/tree_plugin.h
> >>@@ -686,15 +686,6 @@ static void rcu_preempt_check_callbacks(void)
> >>          t->rcu_read_unlock_special.b.need_qs = true;
> >>  }
> >>-#ifdef CONFIG_RCU_BOOST
> >>-
> >>-static void rcu_preempt_do_callbacks(void)
> >>-{
> >>-    rcu_do_batch(rcu_state_p, this_cpu_ptr(rcu_data_p));
> >>-}
> >>-
> >>-#endif /* #ifdef CONFIG_RCU_BOOST */
> >>-
> >>  /**
> >>   * call_rcu() - Queue an RCU callback for invocation after a
> >>grace period.
> >>   * @head: structure to be used for queueing the RCU updates.
> >>@@ -1170,7 +1161,7 @@ static void rcu_kthread_do_work(void)
> >>  {
> >>      rcu_do_batch(&rcu_sched_state, this_cpu_ptr(&rcu_sched_data));
> >>      rcu_do_batch(&rcu_bh_state, this_cpu_ptr(&rcu_bh_data));
> >>-    rcu_preempt_do_callbacks();
> >>+    rcu_do_batch(&rcu_preempt_state, this_cpu_ptr(&rcu_preempt_data));
> >
> >OMG. Sorry for the mistake and thank you very much for fixing it.
> >
> >I will be more careful.
> 
> Ah. Logically no difference between mine and your fixed one.

Yes, your patch was perfectly fine.  It just kept an additional pair of
memory fetches that are unnecessary.  So if you considered your patch
to have a problem, then the original code had that same problem.

> But anyway yours looks much better! Thank you~ :)

And you!

							Thanx, Paul