rcu.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rcu-tasks: Correct IPI failure handling in trc_wait_for_one_reader
@ 2021-08-27  8:13 Neeraj Upadhyay
  2021-08-27 21:52 ` Paul E. McKenney
  0 siblings, 1 reply; 2+ messages in thread
From: Neeraj Upadhyay @ 2021-08-27  8:13 UTC (permalink / raw)
  To: paulmck, josh, rostedt, mathieu.desnoyers, jiangshanlai, joel
  Cc: rcu, linux-kernel, urezki, frederic, boqun.feng, Neeraj Upadhyay

The trc_wait_for_one_reader() function is called at multiple stages
of trace rcu-tasks GP function, rcu_tasks_wait_gp():

- First, it is called as part of per task function -
  rcu_tasks_trace_pertask(), for all non-idle tasks. As part of per task
  processing, this function add the task in the holdout list and if the
  task is currently running on a CPU, it sends IPI to the task's CPU.
  The IPI handler takes action depending on whether task is in trace
  rcu-tasks read side critical section or not:

  - a. If the task is in trace rcu-tasks read side critical section
       (t->trc_reader_nesting != 0), the IPI handler sets the task's
       ->trc_reader_special.b.need_qs, so that this task notifies exit
       from its outermost read side critical section (by decrementing
       trc_n_readers_need_end) to the GP handling function.
       trc_wait_for_one_reader() also increments trc_n_readers_need_end,
       so that the trace rcu-tasks GP handler function waits for this
       task's read side exit notification. The IPI handler also sets
       t->trc_reader_checked to true, and no further IPIs are sent for
       this task, for this trace rcu-tasks grace period and this
       task can be removed from holdout list.

  - b. If the task is in the process of exiting its trace rcu-tasks
       read side critical section, (t->trc_reader_nesting < 0), defer
       this task's processing to future calls to trc_wait_for_one_reader().

  - c. If task is not in rcu-task read side critical section,
       t->trc_reader_nesting == 0, ->trc_reader_checked is set for this
       task, so that, this task is removed from holdout list.

- Second, trc_wait_for_one_reader() is called as part of post scan, in
  function rcu_tasks_trace_postscan(), for all idle tasks.

- Third, in function check_all_holdout_tasks_trace(), for each task in
  the holdout list, this function is called, if there isn't a pending IPI
  for the task (->trc_ipi_to_cpu == -1). This function removed the task
  from holdout list, if IPI handler has completed the required work,
  to ensure that the current trace rcu-tasks grace period either waits
  for this task, or this task is not in a trace rcu-tasks read side
  critical section.

Now, considering the scenario where smp_call_function_single() fails in
first case, inside rcu_tasks_trace_pertask(). In this case,
->trc_ipi_to_cpu is set to the current CPU for that task. This will
result in trc_wait_for_one_reader() getting skipped in third case,
inside check_all_holdout_tasks_trace(), for this task. This further
results in ->trc_reader_checked never getting set for this task,
and the task not getting removed from holdout list. This can cause
the current trace rcu-tasks grace period to stall.

Fix the above problem, by resetting ->trc_ipi_to_cpu to -1, on
smp_call_function_single() failure, so that future IPI calls can
be send for this task. As all three callers (rcu_tasks_trace_pertask(),
rcu_tasks_trace_postscan(), check_all_holdout_tasks_trace())
of trc_wait_for_one_reader() run with CPU read lock held,
smp_call_function_single() cannot race with CPU hotplug. So,
also add a warning, to report and identify any such failure.

Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
---

Ran rcutorture TRACE01 tree for 10 hrs with below commandline, didn't
observe warning:

Command line: debug_boot_weak_hash panic=-1 selinux=0 initcall_debug debug
console=ttyS0 rcutree.rcu_fanout_leaf=4 nohz_full=1-N rcutorture.onoff_interval=1000
rcutorture.onoff_holdoff=30 rcutorture.n_barrier_cbs=4 rcutorture.stat_interval=15
rcutorture.shutdown_secs=36000 rcutorture.test_no_idle_hz=1 rcutorture.verbose=1
rcutorture.torture_type=tasks-tracing

 kernel/rcu/tasks.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index e6f0def..c110fd0 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1014,9 +1014,11 @@ static void trc_wait_for_one_reader(struct task_struct *t,
 		if (smp_call_function_single(cpu, trc_read_check_handler, t, 0)) {
 			// Just in case there is some other reason for
 			// failure than the target CPU being offline.
+			WARN_ONCE(1, "%s():  smp_call_function_single() failed for CPU: %d\n",
+				  __func__, cpu);
 			rcu_tasks_trace.n_ipis_fails++;
 			per_cpu(trc_ipi_to_cpu, cpu) = false;
-			t->trc_ipi_to_cpu = cpu;
+			t->trc_ipi_to_cpu = -1;
 		}
 	}
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] rcu-tasks: Correct IPI failure handling in trc_wait_for_one_reader
  2021-08-27  8:13 [PATCH] rcu-tasks: Correct IPI failure handling in trc_wait_for_one_reader Neeraj Upadhyay
@ 2021-08-27 21:52 ` Paul E. McKenney
  0 siblings, 0 replies; 2+ messages in thread
From: Paul E. McKenney @ 2021-08-27 21:52 UTC (permalink / raw)
  To: Neeraj Upadhyay
  Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, joel, rcu,
	linux-kernel, urezki, frederic, boqun.feng

On Fri, Aug 27, 2021 at 01:43:35PM +0530, Neeraj Upadhyay wrote:
> The trc_wait_for_one_reader() function is called at multiple stages
> of trace rcu-tasks GP function, rcu_tasks_wait_gp():
> 
> - First, it is called as part of per task function -
>   rcu_tasks_trace_pertask(), for all non-idle tasks. As part of per task
>   processing, this function add the task in the holdout list and if the
>   task is currently running on a CPU, it sends IPI to the task's CPU.
>   The IPI handler takes action depending on whether task is in trace
>   rcu-tasks read side critical section or not:
> 
>   - a. If the task is in trace rcu-tasks read side critical section
>        (t->trc_reader_nesting != 0), the IPI handler sets the task's
>        ->trc_reader_special.b.need_qs, so that this task notifies exit
>        from its outermost read side critical section (by decrementing
>        trc_n_readers_need_end) to the GP handling function.
>        trc_wait_for_one_reader() also increments trc_n_readers_need_end,
>        so that the trace rcu-tasks GP handler function waits for this
>        task's read side exit notification. The IPI handler also sets
>        t->trc_reader_checked to true, and no further IPIs are sent for
>        this task, for this trace rcu-tasks grace period and this
>        task can be removed from holdout list.
> 
>   - b. If the task is in the process of exiting its trace rcu-tasks
>        read side critical section, (t->trc_reader_nesting < 0), defer
>        this task's processing to future calls to trc_wait_for_one_reader().
> 
>   - c. If task is not in rcu-task read side critical section,
>        t->trc_reader_nesting == 0, ->trc_reader_checked is set for this
>        task, so that, this task is removed from holdout list.
> 
> - Second, trc_wait_for_one_reader() is called as part of post scan, in
>   function rcu_tasks_trace_postscan(), for all idle tasks.
> 
> - Third, in function check_all_holdout_tasks_trace(), for each task in
>   the holdout list, this function is called, if there isn't a pending IPI
>   for the task (->trc_ipi_to_cpu == -1). This function removed the task
>   from holdout list, if IPI handler has completed the required work,
>   to ensure that the current trace rcu-tasks grace period either waits
>   for this task, or this task is not in a trace rcu-tasks read side
>   critical section.
> 
> Now, considering the scenario where smp_call_function_single() fails in
> first case, inside rcu_tasks_trace_pertask(). In this case,
> ->trc_ipi_to_cpu is set to the current CPU for that task. This will
> result in trc_wait_for_one_reader() getting skipped in third case,
> inside check_all_holdout_tasks_trace(), for this task. This further
> results in ->trc_reader_checked never getting set for this task,
> and the task not getting removed from holdout list. This can cause
> the current trace rcu-tasks grace period to stall.
> 
> Fix the above problem, by resetting ->trc_ipi_to_cpu to -1, on
> smp_call_function_single() failure, so that future IPI calls can
> be send for this task. As all three callers (rcu_tasks_trace_pertask(),
> rcu_tasks_trace_postscan(), check_all_holdout_tasks_trace())
> of trc_wait_for_one_reader() run with CPU read lock held,
> smp_call_function_single() cannot race with CPU hotplug. So,
> also add a warning, to report and identify any such failure.
> 
> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> ---
> 
> Ran rcutorture TRACE01 tree for 10 hrs with below commandline, didn't
> observe warning:
> 
> Command line: debug_boot_weak_hash panic=-1 selinux=0 initcall_debug debug
> console=ttyS0 rcutree.rcu_fanout_leaf=4 nohz_full=1-N rcutorture.onoff_interval=1000
> rcutorture.onoff_holdoff=30 rcutorture.n_barrier_cbs=4 rcutorture.stat_interval=15
> rcutorture.shutdown_secs=36000 rcutorture.test_no_idle_hz=1 rcutorture.verbose=1
> rcutorture.torture_type=tasks-tracing

Very good, thank you!

I queued this for v5.16 (not the upcoming merge window, but the one after
that) and for review and further testing.  I did a little wordsmithing,
so could you please check to make sure that I didn't mess something up?

							Thanx, Paul

------------------------------------------------------------------------

commit 2717b14fb5fc57e5f131cc0bac26c85fc03a0cd2
Author: Neeraj Upadhyay <neeraju@codeaurora.org>
Date:   Fri Aug 27 13:43:35 2021 +0530

    rcu-tasks: Fix IPI failure handling in trc_wait_for_one_reader
    
    The trc_wait_for_one_reader() function is called at multiple stages
    of trace rcu-tasks GP function, rcu_tasks_wait_gp():
    
    - First, it is called as part of per task function -
      rcu_tasks_trace_pertask(), for all non-idle tasks. As part of per task
      processing, this function add the task in the holdout list and if the
      task is currently running on a CPU, it sends IPI to the task's CPU.
      The IPI handler takes action depending on whether task is in trace
      rcu-tasks read side critical section or not:
    
      - a. If the task is in trace rcu-tasks read side critical section
           (t->trc_reader_nesting != 0), the IPI handler sets the task's
           ->trc_reader_special.b.need_qs, so that this task notifies exit
           from its outermost read side critical section (by decrementing
           trc_n_readers_need_end) to the GP handling function.
           trc_wait_for_one_reader() also increments trc_n_readers_need_end,
           so that the trace rcu-tasks GP handler function waits for this
           task's read side exit notification. The IPI handler also sets
           t->trc_reader_checked to true, and no further IPIs are sent for
           this task, for this trace rcu-tasks grace period and this
           task can be removed from holdout list.
    
      - b. If the task is in the process of exiting its trace rcu-tasks
           read side critical section, (t->trc_reader_nesting < 0), defer
           this task's processing to future calls to trc_wait_for_one_reader().
    
      - c. If task is not in rcu-task read side critical section,
           t->trc_reader_nesting == 0, ->trc_reader_checked is set for this
           task, so that this task is removed from holdout list.
    
    - Second, trc_wait_for_one_reader() is called as part of post scan, in
      function rcu_tasks_trace_postscan(), for all idle tasks.
    
    - Third, in function check_all_holdout_tasks_trace(), this function is
      called for each task in the holdout list, but only if there isn't
      a pending IPI for the task (->trc_ipi_to_cpu == -1). This function
      removed the task from holdout list, if IPI handler has completed the
      required work, to ensure that the current trace rcu-tasks grace period
      either waits for this task, or this task is not in a trace rcu-tasks
      read side critical section.
    
    Now, considering the scenario where smp_call_function_single() fails in
    first case, inside rcu_tasks_trace_pertask(). In this case,
    ->trc_ipi_to_cpu is set to the current CPU for that task. This will
    result in trc_wait_for_one_reader() getting skipped in third case,
    inside check_all_holdout_tasks_trace(), for this task. This further
    results in ->trc_reader_checked never getting set for this task,
    and the task not getting removed from holdout list. This can cause
    the current trace rcu-tasks grace period to stall.
    
    Fix the above problem, by resetting ->trc_ipi_to_cpu to -1, on
    smp_call_function_single() failure, so that future IPI calls can
    be send for this task.
    
    Note that all three of the trc_wait_for_one_reader() function's
    callers (rcu_tasks_trace_pertask(), rcu_tasks_trace_postscan(),
    check_all_holdout_tasks_trace()) hold cpu_read_lock().  This means
    that smp_call_function_single() cannot race with CPU hotplug, and thus
    should never fail.  Therefore, also add a warning in order to report
    any such failure in case smp_call_function_single() grows some other
    reason for failure.
    
    Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index e6f0def1cb60..c110fd080c5b 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1014,9 +1014,11 @@ static void trc_wait_for_one_reader(struct task_struct *t,
 		if (smp_call_function_single(cpu, trc_read_check_handler, t, 0)) {
 			// Just in case there is some other reason for
 			// failure than the target CPU being offline.
+			WARN_ONCE(1, "%s():  smp_call_function_single() failed for CPU: %d\n",
+				  __func__, cpu);
 			rcu_tasks_trace.n_ipis_fails++;
 			per_cpu(trc_ipi_to_cpu, cpu) = false;
-			t->trc_ipi_to_cpu = cpu;
+			t->trc_ipi_to_cpu = -1;
 		}
 	}
 }

^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-08-27 21:52 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-27  8:13 [PATCH] rcu-tasks: Correct IPI failure handling in trc_wait_for_one_reader Neeraj Upadhyay
2021-08-27 21:52 ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).