Re: [PATCH v5 tip/core/rcu 4/9] completion: Replace spin_unlock_wait() with lock/unlock pair

From: Paul E. McKenney
Date: Thu Aug 17 2017 - 08:30:59 EST

Next message: Aviad Krawczyk: "[PATCH V6 12/21] net-next/hinic: Add qp resources"
Previous message: Aviad Krawczyk: "[PATCH V6 13/21] net-next/hinic: Set qp context"
In reply to: Ingo Molnar: "Re: [PATCH v5 tip/core/rcu 4/9] completion: Replace spin_unlock_wait() with lock/unlock pair"
Next in thread: Ingo Molnar: "Re: [PATCH v5 tip/core/rcu 4/9] completion: Replace spin_unlock_wait() with lock/unlock pair"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Aug 17, 2017 at 10:26:16AM +0200, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
>
> > There is no agreed-upon definition of spin_unlock_wait()'s semantics,
> > and it appears that all callers could do just as well with a lock/unlock
> > pair. This commit therefore replaces the spin_unlock_wait() call in
> > completion_done() with spin_lock() followed immediately by spin_unlock().
> > This should be safe from a performance perspective because the lock
> > will be held only the wakeup happens really quickly.
> >
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Will Deacon <will.deacon@xxxxxxx>
> > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Cc: Andrea Parri <parri.andrea@xxxxxxxxx>
> > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > [ paulmck: Updated to use irqsave based on 0day Test Robot feedback. ]
> >
> > diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
> > index 13fc5ae9bf2f..c9524d2d9316 100644
> > --- a/kernel/sched/completion.c
> > +++ b/kernel/sched/completion.c
> > @@ -300,6 +300,8 @@ EXPORT_SYMBOL(try_wait_for_completion);
> > */
> > bool completion_done(struct completion *x)
> > {
> > + unsigned long flags;
> > +
> > if (!READ_ONCE(x->done))
> > return false;
> >
> > @@ -307,14 +309,9 @@ bool completion_done(struct completion *x)
> > * If ->done, we need to wait for complete() to release ->wait.lock
> > * otherwise we can end up freeing the completion before complete()
> > * is done referencing it.
> > - *
> > - * The RMB pairs with complete()'s RELEASE of ->wait.lock and orders
> > - * the loads of ->done and ->wait.lock such that we cannot observe
> > - * the lock before complete() acquires it while observing the ->done
> > - * after it's acquired the lock.
> > */
> > - smp_rmb();
> > - spin_unlock_wait(&x->wait.lock);
> > + spin_lock_irqsave(&x->wait.lock, flags);
> > + spin_unlock_irqrestore(&x->wait.lock, flags);
> > return true;
> > }
> > EXPORT_SYMBOL(completion_done);
>
> I'm fine with this patch - as long as there are no performance regression reports.
> (which I suspect there won't be.)

If there is, 0day Test Robot hasn't spotted it, nor has anyone else
reported it.

> Would you like to carry this in the RCU tree, due to other changes depending on
> this change - or can I pick this up into the scheduler tree?

Timely question! ;-)

My current plan is to send you a pull request like the following later
today, Pacific Time (but rebased adding Steve Rostedt's Reviewed-by).
This patch is on one of the branches, currently v4.13-rc2..93d8d7a12090
("arch: Remove spin_unlock_wait() arch-specific definitions") in my
-rcu tree.

Ah, and v4.13-rc2..7391304c4959 ("membarrier: Expedited private command")
is mostly outside of RCU as well.

Since I will be rebasing and remerging anyway, if you would prefer that I
split the spin_unlock_wait() and/or misc branches out, I am happy to do so.
If I don't hear otherwise, though, I will send all seven branches using
my usual approach.

So, if you want something different than my usual approach, please just
let me know!

Thanx, Paul

PS. At the moment, there are no code changes to be applied, just
Steve's Reviewed-by.

------------------------------------------------------------------------
Prototype pull request
------------------------------------------------------------------------

The following changes since commit 520eccdfe187591a51ea9ab4c1a024ae4d0f68d9:

Linux 4.13-rc2 (2017-07-23 16:15:17 -0700)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git 20f2d43c114bf57d16580a758120cc65aa991bea

for you to fetch changes up to 20f2d43c114bf57d16580a758120cc65aa991bea:

Merge branches 'doc.2017.07.24c', 'fixes.2017.08.09a', 'hotplug.2017.07.25b', 'misc.2017.08.10a', 'spin_unlock_wait_no.2017.08.11a', 'srcu.2017.07.27c' and 'torture.2017.07.24c' into HEAD (2017-08-11 13:15:44 -0700)

----------------------------------------------------------------
Joe Perches (1):
module: Fix pr_fmt() bug for header use of printk

Luis R. Rodriguez (2):
swait: add idle variants which don't contribute to load average
rcu: use idle versions of swait to make idle-hack clear

Manfred Spraul (1):
net/netfilter/nf_conntrack_core: Fix net_conntrack_lock()

Masami Hiramatsu (1):
rcu/tracing: Set disable_rcu_irq_enter on rcu_eqs_exit()

Mathieu Desnoyers (1):
membarrier: Expedited private command

Oleg Nesterov (1):
task_work: Replace spin_unlock_wait() with lock/unlock pair

Paul E. McKenney (56):
documentation: Fix relation between nohz_full and rcu_nocbs
doc: RCU documentation update
doc: Update memory-barriers.txt for read-to-write dependencies
doc: Add RCU files to docbook-generation files
doc: No longer allowed to use rcu_dereference on non-pointers
doc: Set down RCU's scheduling-clock-interrupt needs
init_task: Remove redundant INIT_TASK_RCU_TREE_PREEMPT() macro
srcu: Move rcu_scheduler_starting() from Tiny RCU to Tiny SRCU
rcutorture: Remove obsolete SRCU-C.boot
srcu: Make process_srcu() be static
rcutorture: Move SRCU status printing to SRCU implementations
rcutorture: Print SRCU lock/unlock totals
rcu: Remove CONFIG_TASKS_RCU ifdef from rcuperf.c
rcutorture: Select CONFIG_PROVE_LOCKING for Tiny SRCU scenario
torture: Add --kconfig argument to kvm.sh
rcutorture: Don't wait for kernel when all builds fail
rcutorture: Enable SRCU readers from timer handler
rcutorture: Place event-traced strings into trace buffer
rcutorture: Use nr_cpus rather than maxcpus to limit test size
rcutorture: Add task's CPU for rcutorture writer stalls
rcutorture: Eliminate unused ts_rem local from rcu_trace_clock_local()
rcu: Add last-CPU to GP-kthread starvation messages
rcutorture: Invoke call_rcu() from timer handler
rcu: Use timer as backstop for NOCB deferred wakeups
atomics: Revert addition of comment header to spin_unlock_wait()
rcu: Migrate callbacks earlier in the CPU-offline timeline
rcu: Make expedited GPs correctly handle hardware CPU insertion
torture: Fix typo suppressing CPU-hotplug statistics
rcu: Remove orphan/adopt event-tracing fields
rcu: Check for NOCB CPUs and empty lists earlier in CB migration
rcu: Make NOCB CPUs migrate CBs directly from outgoing CPU
rcu: Advance outgoing CPU's callbacks before migrating them
rcu: Eliminate rcu_state ->orphan_lock
rcu: Advance callbacks after migration
rcu: Localize rcu_state ->orphan_pend and ->orphan_done
rcu: Remove unused RCU list functions
rcu: Move callback-list warning to irq-disable region
srcu: Provide ordering for CPU not involved in grace period
sched,rcu: Make cond_resched() provide RCU quiescent state
rcu: Drive TASKS_RCU directly off of PREEMPT
rcu: Create reasonable API for do_exit() TASKS_RCU processing
rcu: Add TPS() to event-traced strings
rcu: Move rcu.h to new trivial-function style
rcu: Add event tracing to ->gp_tasks update at GP start
rcu: Add TPS() protection for _rcu_barrier_trace strings
rcu: Add assertions verifying blocked-tasks list
rcu: Add warning to rcu_idle_enter() for irqs enabled
rcu: Remove exports from rcu_idle_exit() and rcu_idle_enter()
sched: Replace spin_unlock_wait() with lock/unlock pair
completion: Replace spin_unlock_wait() with lock/unlock pair
exit: Replace spin_unlock_wait() with lock/unlock pair
ipc: Replace spin_unlock_wait() with lock/unlock pair
drivers/ata: Replace spin_unlock_wait() with lock/unlock pair
locking: Remove spin_unlock_wait() generic definitions
arch: Remove spin_unlock_wait() arch-specific definitions
Merge branches 'doc.2017.07.24c', 'fixes.2017.08.09a', 'hotplug.2017.07.25b', 'misc.2017.08.10a', 'spin_unlock_wait_no.2017.08.11a', 'srcu.2017.07.27c' and 'torture.2017.07.24c' into HEAD

Peter Zijlstra (Intel) (1):
rcu: Make rcu_idle_enter() rely on callers disabling irqs

Tejun Heo (1):
sched: Allow migrating kthreads into online but inactive CPUs

.../RCU/Design/Requirements/Requirements.html | 130 +++++++++++
Documentation/RCU/checklist.txt | 121 +++++++----
Documentation/RCU/rcu.txt | 9 +-
Documentation/RCU/rcu_dereference.txt | 61 ++----
Documentation/RCU/rcubarrier.txt | 5 +
Documentation/RCU/torture.txt | 20 +-
Documentation/RCU/whatisRCU.txt | 5 +-
Documentation/admin-guide/kernel-parameters.txt | 7 +-
Documentation/core-api/kernel-api.rst | 49 +++++
Documentation/memory-barriers.txt | 41 ++--
MAINTAINERS | 2 +-
arch/alpha/include/asm/spinlock.h | 5 -
arch/arc/include/asm/spinlock.h | 5 -
arch/arm/include/asm/spinlock.h | 16 --
arch/arm64/include/asm/spinlock.h | 58 +----
arch/arm64/kernel/process.c | 2 +
arch/blackfin/include/asm/spinlock.h | 5 -
arch/blackfin/kernel/module.c | 39 ++--
arch/hexagon/include/asm/spinlock.h | 5 -
arch/ia64/include/asm/spinlock.h | 21 --
arch/m32r/include/asm/spinlock.h | 5 -
arch/metag/include/asm/spinlock.h | 5 -
arch/mn10300/include/asm/spinlock.h | 5 -
arch/parisc/include/asm/spinlock.h | 7 -
arch/powerpc/include/asm/spinlock.h | 33 ---
arch/s390/include/asm/spinlock.h | 7 -
arch/sh/include/asm/spinlock-cas.h | 5 -
arch/sh/include/asm/spinlock-llsc.h | 5 -
arch/sparc/include/asm/spinlock_32.h | 5 -
arch/tile/include/asm/spinlock_32.h | 2 -
arch/tile/include/asm/spinlock_64.h | 2 -
arch/tile/lib/spinlock_32.c | 23 --
arch/tile/lib/spinlock_64.c | 22 --
arch/xtensa/include/asm/spinlock.h | 5 -
drivers/ata/libata-eh.c | 8 +-
include/asm-generic/qspinlock.h | 14 --
include/linux/init_task.h | 8 +-
include/linux/rcupdate.h | 15 +-
include/linux/rcutiny.h | 8 +-
include/linux/sched.h | 8 +-
include/linux/spinlock.h | 31 ---
include/linux/spinlock_up.h | 6 -
include/linux/srcutiny.h | 13 ++
include/linux/srcutree.h | 3 +-
include/linux/swait.h | 55 +++++
include/trace/events/rcu.h | 7 +-
include/uapi/linux/membarrier.h | 23 +-
ipc/sem.c | 3 +-
kernel/Makefile | 1 -
kernel/cpu.c | 1 +
kernel/exit.c | 10 +-
kernel/locking/qspinlock.c | 117 ----------
kernel/membarrier.c | 70 ------
kernel/rcu/Kconfig | 3 +-
kernel/rcu/rcu.h | 128 ++---------
kernel/rcu/rcu_segcblist.c | 108 +++-------
kernel/rcu/rcu_segcblist.h | 28 +--
kernel/rcu/rcuperf.c | 17 +-
kernel/rcu/rcutorture.c | 83 +++----
kernel/rcu/srcutiny.c | 8 +
kernel/rcu/srcutree.c | 50 ++++-
kernel/rcu/tiny.c | 2 -
kernel/rcu/tiny_plugin.h | 47 ----
kernel/rcu/tree.c | 238 ++++++++-------------
kernel/rcu/tree.h | 15 +-
kernel/rcu/tree_exp.h | 2 +-
kernel/rcu/tree_plugin.h | 238 ++++++++++++---------
kernel/rcu/update.c | 18 +-
kernel/sched/Makefile | 1 +
kernel/sched/completion.c | 11 +-
kernel/sched/core.c | 39 +++-
kernel/sched/membarrier.c | 152 +++++++++++++
kernel/task_work.c | 8 +-
kernel/torture.c | 2 +-
net/netfilter/nf_conntrack_core.c | 52 +++--
.../selftests/rcutorture/bin/config_override.sh | 61 ++++++
.../testing/selftests/rcutorture/bin/functions.sh | 27 ++-
.../testing/selftests/rcutorture/bin/kvm-build.sh | 11 +-
.../selftests/rcutorture/bin/kvm-test-1-run.sh | 58 ++---
tools/testing/selftests/rcutorture/bin/kvm.sh | 34 ++-
.../selftests/rcutorture/configs/rcu/BUSTED.boot | 2 +-
.../selftests/rcutorture/configs/rcu/SRCU-C.boot | 1 -
.../selftests/rcutorture/configs/rcu/SRCU-u | 3 +-
.../selftests/rcutorture/configs/rcu/TREE01.boot | 2 +-
.../selftests/rcutorture/doc/TREE_RCU-kconfig.txt | 2 +-
85 files changed, 1245 insertions(+), 1344 deletions(-)
delete mode 100644 kernel/membarrier.c
delete mode 100644 kernel/rcu/tiny_plugin.h
create mode 100644 kernel/sched/membarrier.c
create mode 100755 tools/testing/selftests/rcutorture/bin/config_override.sh
delete mode 100644 tools/testing/selftests/rcutorture/configs/rcu/SRCU-C.boot

Next message: Aviad Krawczyk: "[PATCH V6 12/21] net-next/hinic: Add qp resources"
Previous message: Aviad Krawczyk: "[PATCH V6 13/21] net-next/hinic: Set qp context"
In reply to: Ingo Molnar: "Re: [PATCH v5 tip/core/rcu 4/9] completion: Replace spin_unlock_wait() with lock/unlock pair"
Next in thread: Ingo Molnar: "Re: [PATCH v5 tip/core/rcu 4/9] completion: Replace spin_unlock_wait() with lock/unlock pair"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]