All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Frederic Weisbecker <frederic@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>, rcu@vger.kernel.org
Subject: Re: [PATCH 3/3] rcu: Assume rcu_report_dead() always deals with local CPU
Date: Wed, 19 May 2021 11:51:07 -0700	[thread overview]
Message-ID: <20210519185107.GB4441@paulmck-ThinkPad-P17-Gen-1> (raw)
In-Reply-To: <20210519000930.15702-4-frederic@kernel.org>

On Wed, May 19, 2021 at 02:09:30AM +0200, Frederic Weisbecker wrote:
> rcu_report_dead() is always called locally from the idle path. Passing
> a CPU number to it suggests otherwise and is rather error-prone as the
> code inside relies on locality.
> 
> Robustify the function prototype and refine the name along the way.
> 
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>

Makes a lot of sense, thank you!

On the function name, here is the list:

int rcutree_prepare_cpu(unsigned int cpu) -- notifier from any CPU.
void rcu_cpu_starting(unsigned int cpu) -- direct call on incoming CPU.
int rcutree_online_cpu(unsigned int cpu) -- notifier from any CPU.

int rcutree_offline_cpu(unsigned int cpu) -- notifier from any CPU.
void rcu_report_dead(unsigned int cpu) -- direct call on outgoing CPU.
void rcutree_migrate_callbacks(int cpu) -- direct call from surviving CPU.
int rcutree_dead_cpu(unsigned int cpu) -- notifier from any CPU.

Note that rcu_report_dead() can also be invoked from cpu_die_early() on
other CPU when onlining a CPU fails.  This happens on arm64.  Which might
be an arm64 bug, but unless I am missing something it is a case where
rcu_report_dead() is called non-locally.

Which means that I have to hold off on this patch for the moment.

And the naming is currently a bit random, isn't it?  :-/

Maybe rcutree_*_cpu() if there is a CPU parameter and rcutree_*_self()
if all calls run on the CPU in question?  I cannot immediately think of
a reason to make names reflect whether the corresponding functions are
directly called or are called via notifier.  Thoughts?

							Thanx, Paul

> ---
>  .../Expedited-Grace-Periods/Expedited-Grace-Periods.rst     | 2 +-
>  Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp-fqs.svg | 2 +-
>  Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp.svg     | 2 +-
>  .../RCU/Design/Memory-Ordering/TreeRCU-hotplug.svg          | 2 +-
>  Documentation/RCU/Design/Requirements/Requirements.rst      | 2 +-
>  arch/arm64/kernel/smp.c                                     | 2 +-
>  include/linux/rcupdate.h                                    | 2 +-
>  kernel/cpu.c                                                | 4 ++--
>  kernel/rcu/tree.c                                           | 6 +++---
>  9 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/RCU/Design/Expedited-Grace-Periods/Expedited-Grace-Periods.rst b/Documentation/RCU/Design/Expedited-Grace-Periods/Expedited-Grace-Periods.rst
> index 6f89cf1e567d..eddf5eea94f1 100644
> --- a/Documentation/RCU/Design/Expedited-Grace-Periods/Expedited-Grace-Periods.rst
> +++ b/Documentation/RCU/Design/Expedited-Grace-Periods/Expedited-Grace-Periods.rst
> @@ -181,7 +181,7 @@ operations is carried out at several levels:
>     of this wait (or series of waits, as the case may be) is to permit a
>     concurrent CPU-hotplug operation to complete.
>  #. In the case of RCU-sched, one of the last acts of an outgoing CPU is
> -   to invoke ``rcu_report_dead()``, which reports a quiescent state for
> +   to invoke ``rcu_report_dead_cpu()``, which reports a quiescent state for
>     that CPU. However, this is likely paranoia-induced redundancy.
>  
>  +-----------------------------------------------------------------------+
> diff --git a/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp-fqs.svg b/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp-fqs.svg
> index 7ddc094d7f28..6e7b05ff00d3 100644
> --- a/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp-fqs.svg
> +++ b/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp-fqs.svg
> @@ -1135,7 +1135,7 @@
>         font-weight="bold"
>         font-size="192"
>         id="text202-7-5-3-27-6-5"
> -       style="font-size:192px;font-style:normal;font-weight:bold;text-anchor:start;fill:#000000;stroke-width:0.025in;font-family:Courier">rcu_report_dead()</text>
> +       style="font-size:192px;font-style:normal;font-weight:bold;text-anchor:start;fill:#000000;stroke-width:0.025in;font-family:Courier">rcu_report_dead_cpu()</text>
>      <text
>         xml:space="preserve"
>         x="3745.7725"
> diff --git a/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp.svg b/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp.svg
> index 069f6f8371c2..ede6ae66d286 100644
> --- a/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp.svg
> +++ b/Documentation/RCU/Design/Memory-Ordering/TreeRCU-gp.svg
> @@ -3274,7 +3274,7 @@
>           font-weight="bold"
>           font-size="192"
>           id="text202-7-5-3-27-6-5"
> -         style="font-size:192px;font-style:normal;font-weight:bold;text-anchor:start;fill:#000000;stroke-width:0.025in;font-family:Courier">rcu_report_dead()</text>
> +         style="font-size:192px;font-style:normal;font-weight:bold;text-anchor:start;fill:#000000;stroke-width:0.025in;font-family:Courier">rcu_report_dead_cpu()</text>
>        <text
>           xml:space="preserve"
>           x="3745.7725"
> diff --git a/Documentation/RCU/Design/Memory-Ordering/TreeRCU-hotplug.svg b/Documentation/RCU/Design/Memory-Ordering/TreeRCU-hotplug.svg
> index 2c9310ba29ba..e0ab8cd8db2d 100644
> --- a/Documentation/RCU/Design/Memory-Ordering/TreeRCU-hotplug.svg
> +++ b/Documentation/RCU/Design/Memory-Ordering/TreeRCU-hotplug.svg
> @@ -607,7 +607,7 @@
>         font-weight="bold"
>         font-size="192"
>         id="text202-7-5-3-27-6"
> -       style="font-size:192px;font-style:normal;font-weight:bold;text-anchor:start;fill:#000000;stroke-width:0.025in;font-family:Courier">rcu_report_dead()</text>
> +       style="font-size:192px;font-style:normal;font-weight:bold;text-anchor:start;fill:#000000;stroke-width:0.025in;font-family:Courier">rcu_report_dead_cpu()</text>
>      <text
>         xml:space="preserve"
>         x="3745.7725"
> diff --git a/Documentation/RCU/Design/Requirements/Requirements.rst b/Documentation/RCU/Design/Requirements/Requirements.rst
> index 38a39476fc24..ede6a2bbf69d 100644
> --- a/Documentation/RCU/Design/Requirements/Requirements.rst
> +++ b/Documentation/RCU/Design/Requirements/Requirements.rst
> @@ -1953,7 +1953,7 @@ if offline CPUs block an RCU grace period for too long.
>  
>  An offline CPU's quiescent state will be reported either:
>  
> -1.  As the CPU goes offline using RCU's hotplug notifier (rcu_report_dead()).
> +1.  As the CPU goes offline using RCU's hotplug notifier (rcu_report_dead_cpu()).
>  2.  When grace period initialization (rcu_gp_init()) detects a
>      race either with CPU offlining or with a task unblocking on a leaf
>      ``rcu_node`` structure whose CPUs are all offline.
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index dcd7041b2b07..74c0ae3d6c93 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -414,7 +414,7 @@ void cpu_die_early(void)
>  
>  	/* Mark this CPU absent */
>  	set_cpu_present(cpu, 0);
> -	rcu_report_dead(cpu);
> +	rcu_report_dead_cpu();
>  
>  	if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
>  		update_cpu_boot_status(CPU_KILL_ME);
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index d9680b798b21..fda1dc9b4e03 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -85,7 +85,7 @@ static inline int rcu_preempt_depth(void)
>  void rcu_init(void);
>  extern int rcu_scheduler_active __read_mostly;
>  void rcu_sched_clock_irq(int user);
> -void rcu_report_dead(unsigned int cpu);
> +void rcu_report_dead_cpu(void);
>  void rcutree_migrate_callbacks(int cpu);
>  
>  #ifdef CONFIG_TASKS_RCU_GENERIC
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index e538518556f4..852fcbda1fd8 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1016,10 +1016,10 @@ void cpuhp_report_idle_dead(void)
>  	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
>  
>  	BUG_ON(st->state != CPUHP_AP_OFFLINE);
> -	rcu_report_dead(smp_processor_id());
> +	rcu_report_dead_cpu();
>  	st->state = CPUHP_AP_IDLE_DEAD;
>  	/*
> -	 * We cannot call complete after rcu_report_dead() so we delegate it
> +	 * We cannot call complete after rcu_report_dead_cpu() so we delegate it
>  	 * to an online cpu.
>  	 */
>  	smp_call_function_single(cpumask_first(cpu_online_mask),
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index a6b448e6e059..824a3b367797 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4322,11 +4322,11 @@ void rcu_cpu_starting(unsigned int cpu)
>   * from the outgoing CPU rather than from the cpuhp_step mechanism.
>   * This is because this function must be invoked at a precise location.
>   */
> -void rcu_report_dead(unsigned int cpu)
> +void rcu_report_dead_cpu(void)
>  {
>  	unsigned long flags;
>  	unsigned long mask;
> -	struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> +	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
>  	struct rcu_node *rnp = rdp->mynode;  /* Outgoing CPU's rdp & rnp. */
>  
>  	// Do any dangling deferred wakeups.
> @@ -4334,7 +4334,7 @@ void rcu_report_dead(unsigned int cpu)
>  
>  	/* QS for any half-done expedited grace period. */
>  	preempt_disable();
> -	rcu_report_exp_rdp(this_cpu_ptr(&rcu_data));
> +	rcu_report_exp_rdp(rdp);
>  	preempt_enable();
>  	rcu_preempt_deferred_qs(current);
>  
> -- 
> 2.25.1
> 

  reply	other threads:[~2021-05-19 18:51 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-19  0:09 [PATCH 0/3] rcu/nocb cleanups Frederic Weisbecker
2021-05-19  0:09 ` [PATCH 1/3] rcu/nocb: Start moving nocb code to its own plugin file Frederic Weisbecker
2021-05-19 15:55   ` Paul E. McKenney
2021-05-20  1:02     ` Frederic Weisbecker
2021-05-20  4:54       ` Paul E. McKenney
2021-05-19  0:09 ` [PATCH 2/3] rcu/nocb: Remove NOCB deferred wakeup from rcutree_dead_cpu() Frederic Weisbecker
2021-05-19 15:59   ` Paul E. McKenney
2021-05-20  0:25     ` Frederic Weisbecker
2021-05-20  0:34       ` Paul E. McKenney
2021-05-19  0:09 ` [PATCH 3/3] rcu: Assume rcu_report_dead() always deals with local CPU Frederic Weisbecker
2021-05-19 18:51   ` Paul E. McKenney [this message]
2021-05-20  0:54     ` Frederic Weisbecker
2021-05-20  4:54       ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210519185107.GB4441@paulmck-ThinkPad-P17-Gen-1 \
    --to=paulmck@kernel.org \
    --cc=frederic@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rcu@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.