From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:35:11
Hi,
This patchset removes CPU hotplug's dependence on stop_machine() from the CPU
offline path and provides an alternative (set of APIs) to preempt_disable() to
prevent CPUs from going offline, which can be invoked from atomic context.
The motivation behind the removal of stop_machine() is to avoid its ill-effects
and thus improve the design of CPU hotplug. (More description regarding this
is available in the patches).
All the users of preempt_disable()/local_irq_disable() who used to use it to
prevent CPU offline, have been converted to the new primitives introduced in the
patchset. Also, the CPU_DYING notifiers have been audited to check whether
they can cope up with the removal of stop_machine() or whether they need to
use new locks for synchronization (all CPU_DYING notifiers looked OK, without
the need for any new locks).
Applies on v3.8-rc4. It currently has some locking issues with cpu idle (on
which even lockdep didn't provide any insight unfortunately). So for now, it
works with CONFIG_CPU_IDLE=n.
Overview of the patches:
-----------------------
Patches 1 to 6 introduce a generic, flexible Per-CPU Reader-Writer Locking
scheme.
Patch 7 uses this synchronization mechanism to build the
get/put_online_cpus_atomic() APIs which can be used from atomic context, to
prevent CPUs from going offline.
Patch 8 is a cleanup; it converts preprocessor macros to static inline
functions.
Patches 9 to 42 convert various call-sites to use the new APIs.
Patch 43 is the one which actually removes stop_machine() from the CPU
offline path.
Patch 44 decouples stop_machine() and CPU hotplug from Kconfig.
Patch 45 updates the documentation to reflect the new APIs.
Changes in v5:
--------------
Exposed a new generic locking scheme: Flexible Per-CPU Reader-Writer locks,
based on the synchronization schemes already discussed in the previous
versions, and used it in CPU hotplug, to implement the new APIs.
Audited the CPU_DYING notifiers in the kernel source tree and replaced
usages of preempt_disable() with the new get/put_online_cpus_atomic() APIs
where necessary.
Changes in v4:
--------------
The synchronization scheme has been simplified quite a bit, which makes it
look a lot less complex than before. Some highlights:
* Implicit ACKs:
The earlier design required the readers to explicitly ACK the writer's
signal. The new design uses implicit ACKs instead. The reader switching
over to rwlock implicitly tells the writer to stop waiting for that reader.
* No atomic operations:
Since we got rid of explicit ACKs, we no longer have the need for a reader
and a writer to update the same counter. So we can get rid of atomic ops
too.
Changes in v3:
--------------
* Dropped the _light() and _full() variants of the APIs. Provided a single
interface: get/put_online_cpus_atomic().
* Completely redesigned the synchronization mechanism again, to make it
fast and scalable at the reader-side in the fast-path (when no hotplug
writers are active). This new scheme also ensures that there is no
possibility of deadlocks due to circular locking dependency.
In summary, this provides the scalability and speed of per-cpu rwlocks
(without actually using them), while avoiding the downside (deadlock
possibilities) which is inherent in any per-cpu locking scheme that is
meant to compete with preempt_disable()/enable() in terms of flexibility.
The problem with using per-cpu locking to replace preempt_disable()/enable
was explained here:
https://lkml.org/lkml/2012/12/6/290
Basically we use per-cpu counters (for scalability) when no writers are
active, and then switch to global rwlocks (for lock-safety) when a writer
becomes active. It is a slightly complex scheme, but it is based on
standard principles of distributed algorithms.
Changes in v2:
-------------
* Completely redesigned the synchronization scheme to avoid using any extra
cpumasks.
* Provided APIs for 2 types of atomic hotplug readers: "light" (for
light-weight) and "full". We wish to have more "light" readers than
the "full" ones, to avoid indirectly inducing the "stop_machine effect"
without even actually using stop_machine().
And the patches show that it _is_ generally true: 5 patches deal with
"light" readers, whereas only 1 patch deals with a "full" reader.
Also, the "light" readers happen to be in very hot paths. So it makes a
lot of sense to have such a distinction and a corresponding light-weight
API.
Links to previous versions:
v4: https://lkml.org/lkml/2012/12/11/209
v3: https://lkml.org/lkml/2012/12/7/287
v2: https://lkml.org/lkml/2012/12/5/322
v1: https://lkml.org/lkml/2012/12/4/88
--
Paul E. McKenney (1):
cpu: No more __stop_machine() in _cpu_down()
Srivatsa S. Bhat (44):
percpu_rwlock: Introduce the global reader-writer lock backend
percpu_rwlock: Introduce per-CPU variables for the reader and the writer
percpu_rwlock: Provide a way to define and init percpu-rwlocks at compile time
percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
percpu_rwlock: Make percpu-rwlocks IRQ-safe, optimally
percpu_rwlock: Allow writers to be readers, and add lockdep annotations
CPU hotplug: Provide APIs to prevent CPU offline from atomic context
CPU hotplug: Convert preprocessor macros to static inline functions
smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
smp, cpu hotplug: Fix on_each_cpu_*() to prevent CPU offline properly
sched/timer: Use get/put_online_cpus_atomic() to prevent CPU offline
sched/migration: Use raw_spin_lock/unlock since interrupts are already disabled
sched/rt: Use get/put_online_cpus_atomic() to prevent CPU offline
rcu, CPU hotplug: Fix comment referring to stop_machine()
tick: Use get/put_online_cpus_atomic() to prevent CPU offline
time/clocksource: Use get/put_online_cpus_atomic() to prevent CPU offline
softirq: Use get/put_online_cpus_atomic() to prevent CPU offline
irq: Use get/put_online_cpus_atomic() to prevent CPU offline
net: Use get/put_online_cpus_atomic() to prevent CPU offline
block: Use get/put_online_cpus_atomic() to prevent CPU offline
crypto: pcrypt - Protect access to cpu_online_mask with get/put_online_cpus()
infiniband: ehca: Use get/put_online_cpus_atomic() to prevent CPU offline
[SCSI] fcoe: Use get/put_online_cpus_atomic() to prevent CPU offline
staging: octeon: Use get/put_online_cpus_atomic() to prevent CPU offline
x86: Use get/put_online_cpus_atomic() to prevent CPU offline
perf/x86: Use get/put_online_cpus_atomic() to prevent CPU offline
KVM: Use get/put_online_cpus_atomic() to prevent CPU offline from atomic context
kvm/vmx: Use get/put_online_cpus_atomic() to prevent CPU offline
x86/xen: Use get/put_online_cpus_atomic() to prevent CPU offline
alpha/smp: Use get/put_online_cpus_atomic() to prevent CPU offline
blackfin/smp: Use get/put_online_cpus_atomic() to prevent CPU offline
cris/smp: Use get/put_online_cpus_atomic() to prevent CPU offline
hexagon/smp: Use get/put_online_cpus_atomic() to prevent CPU offline
ia64: Use get/put_online_cpus_atomic() to prevent CPU offline
m32r: Use get/put_online_cpus_atomic() to prevent CPU offline
MIPS: Use get/put_online_cpus_atomic() to prevent CPU offline
mn10300: Use get/put_online_cpus_atomic() to prevent CPU offline
parisc: Use get/put_online_cpus_atomic() to prevent CPU offline
powerpc: Use get/put_online_cpus_atomic() to prevent CPU offline
sh: Use get/put_online_cpus_atomic() to prevent CPU offline
sparc: Use get/put_online_cpus_atomic() to prevent CPU offline
tile: Use get/put_online_cpus_atomic() to prevent CPU offline
CPU hotplug, stop_machine: Decouple CPU hotplug from stop_machine() in Kconfig
Documentation/cpu-hotplug: Remove references to stop_machine()
Documentation/cpu-hotplug.txt | 17 +-
arch/alpha/kernel/smp.c | 19 +-
arch/arm/Kconfig | 1
arch/blackfin/Kconfig | 1
arch/blackfin/mach-common/smp.c | 6 -
arch/cris/arch-v32/kernel/smp.c | 8 +
arch/hexagon/kernel/smp.c | 5 +
arch/ia64/Kconfig | 1
arch/ia64/kernel/irq_ia64.c | 13 +
arch/ia64/kernel/perfmon.c | 6 +
arch/ia64/kernel/smp.c | 23 ++
arch/ia64/mm/tlb.c | 6 -
arch/m32r/kernel/smp.c | 12 +
arch/mips/Kconfig | 1
arch/mips/kernel/cevt-smtc.c | 8 +
arch/mips/kernel/smp.c | 16 +-
arch/mips/kernel/smtc.c | 3
arch/mips/mm/c-octeon.c | 4
arch/mn10300/Kconfig | 1
arch/mn10300/kernel/smp.c | 2
arch/mn10300/mm/cache-smp.c | 5 +
arch/mn10300/mm/tlb-smp.c | 15 +-
arch/parisc/Kconfig | 1
arch/parisc/kernel/smp.c | 4
arch/powerpc/Kconfig | 1
arch/powerpc/mm/mmu_context_nohash.c | 2
arch/s390/Kconfig | 1
arch/sh/Kconfig | 1
arch/sh/kernel/smp.c | 12 +
arch/sparc/Kconfig | 1
arch/sparc/kernel/leon_smp.c | 2
arch/sparc/kernel/smp_64.c | 9 +
arch/sparc/kernel/sun4d_smp.c | 2
arch/sparc/kernel/sun4m_smp.c | 3
arch/tile/kernel/smp.c | 4
arch/x86/Kconfig | 1
arch/x86/include/asm/ipi.h | 5 +
arch/x86/kernel/apic/apic_flat_64.c | 10 +
arch/x86/kernel/apic/apic_numachip.c | 5 +
arch/x86/kernel/apic/es7000_32.c | 5 +
arch/x86/kernel/apic/io_apic.c | 7 +
arch/x86/kernel/apic/ipi.c | 10 +
arch/x86/kernel/apic/x2apic_cluster.c | 4
arch/x86/kernel/apic/x2apic_uv_x.c | 4
arch/x86/kernel/cpu/mcheck/therm_throt.c | 4
arch/x86/kernel/cpu/perf_event_intel_uncore.c | 5 +
arch/x86/kvm/vmx.c | 8 +
arch/x86/mm/tlb.c | 14 +
arch/x86/xen/mmu.c | 11 +
arch/x86/xen/smp.c | 9 +
block/blk-softirq.c | 4
crypto/pcrypt.c | 4
drivers/infiniband/hw/ehca/ehca_irq.c | 8 +
drivers/scsi/fcoe/fcoe.c | 7 +
drivers/staging/octeon/ethernet-rx.c | 3
include/linux/cpu.h | 8 +
include/linux/percpu-rwlock.h | 86 +++++++++
include/linux/stop_machine.h | 2
init/Kconfig | 2
kernel/cpu.c | 61 ++++++
kernel/irq/manage.c | 7 +
kernel/rcutree.c | 9 -
kernel/sched/core.c | 36 +++-
kernel/sched/fair.c | 5 -
kernel/sched/rt.c | 3
kernel/smp.c | 65 ++++---
kernel/softirq.c | 3
kernel/time/clocksource.c | 5 +
kernel/time/tick-broadcast.c | 2
kernel/timer.c | 2
lib/Kconfig | 3
lib/Makefile | 1
lib/percpu-rwlock.c | 242 +++++++++++++++++++++++++
net/core/dev.c | 9 +
virt/kvm/kvm_main.c | 10 +
75 files changed, 776 insertions(+), 129 deletions(-)
create mode 100644 include/linux/percpu-rwlock.h
create mode 100644 lib/percpu-rwlock.c
Thanks,
Srivatsa S. Bhat
IBM Linux Technology Center
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:35:24
A straight-forward (and obvious) algorithm to implement Per-CPU Reader-Writer
locks can also lead to too many deadlock possibilities which can make it very
hard/impossible to use. This is explained in the example below, which helps
justify the need for a different algorithm to implement flexible Per-CPU
Reader-Writer locks.
We can use global rwlocks as shown below safely, without fear of deadlocks:
Readers:
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(&my_rwlock);
2. read_lock(&my_rwlock); spin_lock(&random_lock);
Writer:
CPU 2:
------
write_lock(&my_rwlock);
We can observe that there is no possibility of deadlocks or circular locking
dependencies here. Its perfectly safe.
Now consider a blind/straight-forward conversion of global rwlocks to per-CPU
rwlocks like this:
The reader locks its own per-CPU rwlock for read, and proceeds.
Something like: read_lock(per-cpu rwlock of this cpu);
The writer acquires all per-CPU rwlocks for write and only then proceeds.
Something like:
for_each_online_cpu(cpu)
write_lock(per-cpu rwlock of 'cpu');
Now let's say that for performance reasons, the above scenario (which was
perfectly safe when using global rwlocks) was converted to use per-CPU rwlocks.
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(my_rwlock of CPU 1);
2. read_lock(my_rwlock of CPU 0); spin_lock(&random_lock);
Writer:
CPU 2:
------
for_each_online_cpu(cpu)
write_lock(my_rwlock of 'cpu');
Consider what happens if the writer begins his operation in between steps 1
and 2 at the reader side. It becomes evident that we end up in a (previously
non-existent) deadlock due to a circular locking dependency between the 3
entities, like this:
(holds Waiting for
random_lock) CPU 0 -------------> CPU 2 (holds my_rwlock of CPU 0
for write)
^ |
| |
Waiting| | Waiting
for | | for
| V
------ CPU 1 <------
(holds my_rwlock of
CPU 1 for read)
So obviously this "straight-forward" way of implementing percpu rwlocks is
deadlock-prone. One simple measure for (or characteristic of) safe percpu
rwlock should be that if a user replaces global rwlocks with per-CPU rwlocks
(for performance reasons), he shouldn't suddenly end up in numerous deadlock
possibilities which never existed before. The replacement should continue to
remain safe, and perhaps improve the performance.
Observing the robustness of global rwlocks in providing a fair amount of
deadlock safety, we implement per-CPU rwlocks as nothing but global rwlocks,
as a first step.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/percpu-rwlock.h | 49 ++++++++++++++++++++++++++++++++
lib/Kconfig | 3 ++
lib/Makefile | 1 +
lib/percpu-rwlock.c | 63 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 116 insertions(+)
create mode 100644 include/linux/percpu-rwlock.h
create mode 100644 lib/percpu-rwlock.c
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:35:32
Per-CPU rwlocks ought to give better performance than global rwlocks.
That is where the "per-CPU" component comes in. So introduce the necessary
per-CPU variables that would be necessary at the reader and the writer sides,
and add the support for dynamically initializing per-CPU rwlocks.
These per-CPU variables will be used subsequently to implement the core
algorithm behind per-CPU rwlocks.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/percpu-rwlock.h | 4 ++++
lib/percpu-rwlock.c | 21 +++++++++++++++++++++
2 files changed, 25 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:35:43
Add the support for defining and initializing percpu-rwlocks at compile time
for those users who would like to use percpu-rwlocks really early in the boot
process (even before dynamic per-CPU allocations can begin).
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/percpu-rwlock.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:36:01
Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
lock-ordering related problems (unlike per-cpu locks). However, global
rwlocks lead to unnecessary cache-line bouncing even when there are no
writers present, which can slow down the system needlessly.
Per-cpu counters can help solve the cache-line bouncing problem. So we
actually use the best of both: per-cpu counters (no-waiting) at the reader
side in the fast-path, and global rwlocks in the slowpath.
[ Fastpath = no writer is active; Slowpath = a writer is active ]
IOW, the readers just increment/decrement their per-cpu refcounts (disabling
interrupts during the updates, if necessary) when no writer is active.
When a writer becomes active, he signals all readers to switch to global
rwlocks for the duration of his activity. The readers switch over when it
is safe for them (ie., when they are about to start a fresh, non-nested
read-side critical section) and start using (holding) the global rwlock for
read in their subsequent critical sections.
The writer waits for every existing reader to switch, and then acquires the
global rwlock for write and enters his critical section. Later, the writer
signals all readers that he is done, and that they can go back to using their
per-cpu refcounts again.
Note that the lock-safety (despite the per-cpu scheme) comes from the fact
that the readers can *choose* _when_ to switch to rwlocks upon the writer's
signal. And the readers don't wait on anybody based on the per-cpu counters.
The only true synchronization that involves waiting at the reader-side in this
scheme, is the one arising from the global rwlock, which is safe from circular
locking dependency issues.
Reader-writer locks and per-cpu counters are recursive, so they can be
used in a nested fashion in the reader-path, which makes per-CPU rwlocks also
recursive. Also, this design of switching the synchronization scheme ensures
that you can safely nest and use these locks in a very flexible manner.
I'm indebted to Michael Wang and Xiao Guangrong for their numerous thoughtful
suggestions and ideas, which inspired and influenced many of the decisions in
this as well as previous designs. Thanks a lot Michael and Xiao!
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/percpu-rwlock.h | 10 +++
lib/percpu-rwlock.c | 128 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 136 insertions(+), 2 deletions(-)
@@ -64,21 +64,145 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)voidpercpu_read_lock(structpercpu_rwlock*pcpu_rwlock){-read_lock(&pcpu_rwlock->global_rwlock);+preempt_disable();++/* First and foremost, let the writer know that a reader is active */+this_cpu_inc(*pcpu_rwlock->reader_refcnt);++/*+*Ifwearealreadyusingper-cpurefcounts,itisnotsafetoswitch+*thesynchronizationscheme.Socontinueusingtherefcounts.+*/+if(reader_nested_percpu(pcpu_rwlock)){+gotoout;+}else{+/*+*Thewriteto'reader_refcnt'mustbevisiblebeforewe+*read'writer_signal'.+*/+smp_mb();/* Paired with smp_rmb() in sync_reader() */++if(likely(!writer_active(pcpu_rwlock))){+gotoout;+}else{+/* Writer is active, so switch to global rwlock. */+read_lock(&pcpu_rwlock->global_rwlock);++/*+*Wemighthaveracedwithawritergoinginactive+*beforewetooktheread-lock.Sore-evaluatewhether+*westillneedtoholdtherwlockorifwecanswitch+*backtoper-cpurefcounts.(Thisalsohelpsavoid+*heterogeneousnestingofreaders).+*/+if(writer_active(pcpu_rwlock))+this_cpu_dec(*pcpu_rwlock->reader_refcnt);+else+read_unlock(&pcpu_rwlock->global_rwlock);+}+}++out:+/* Prevent reordering of any subsequent reads */+smp_rmb();}voidpercpu_read_unlock(structpercpu_rwlock*pcpu_rwlock){-read_unlock(&pcpu_rwlock->global_rwlock);+/*+*Weneverallowheterogeneousnestingofreaders.Soitistrivial+*tofindoutthekindofreaderweare,andundotheoperation+*donebyourcorrespondingpercpu_read_lock().+*/+if(__this_cpu_read(*pcpu_rwlock->reader_refcnt)){+this_cpu_dec(*pcpu_rwlock->reader_refcnt);+smp_wmb();/* Paired with smp_rmb() in sync_reader() */+}else{+read_unlock(&pcpu_rwlock->global_rwlock);+}++preempt_enable();+}++staticinlinevoidraise_writer_signal(structpercpu_rwlock*pcpu_rwlock,+unsignedintcpu)+{+per_cpu(*pcpu_rwlock->writer_signal,cpu)=true;+}++staticinlinevoiddrop_writer_signal(structpercpu_rwlock*pcpu_rwlock,+unsignedintcpu)+{+per_cpu(*pcpu_rwlock->writer_signal,cpu)=false;+}++staticvoidannounce_writer_active(structpercpu_rwlock*pcpu_rwlock)+{+unsignedintcpu;++for_each_online_cpu(cpu)+raise_writer_signal(pcpu_rwlock,cpu);++smp_mb();/* Paired with smp_rmb() in percpu_read_[un]lock() */+}++staticvoidannounce_writer_inactive(structpercpu_rwlock*pcpu_rwlock)+{+unsignedintcpu;++drop_writer_signal(pcpu_rwlock,smp_processor_id());++for_each_online_cpu(cpu)+drop_writer_signal(pcpu_rwlock,cpu);++smp_mb();/* Paired with smp_rmb() in percpu_read_[un]lock() */+}++/*+*Waitforthereadertoseethewriter'ssignalandswitchfrompercpu+*refcountstoglobalrwlock.+*+*Ifthereaderisstillusingpercpurefcounts,waitforhimtoswitch.+*Else,wecansafelygoahead,becauseeitherthereaderhasalready+*switchedover,orthenextreaderthatcomesalongonthatCPUwill+*noticethewriter'ssignalandwillswitchovertotherwlock.+*/+staticinlinevoidsync_reader(structpercpu_rwlock*pcpu_rwlock,+unsignedintcpu)+{+smp_rmb();/* Paired with smp_[w]mb() in percpu_read_[un]lock() */++while(reader_uses_percpu_refcnt(pcpu_rwlock,cpu))+cpu_relax();+}++staticvoidsync_all_readers(structpercpu_rwlock*pcpu_rwlock)+{+unsignedintcpu;++for_each_online_cpu(cpu)+sync_reader(pcpu_rwlock,cpu);}voidpercpu_write_lock(structpercpu_rwlock*pcpu_rwlock){+/*+*Tellallreadersthatawriterisbecomingactive,sothatthey+*startswitchingovertotheglobalrwlock.+*/+announce_writer_active(pcpu_rwlock);+sync_all_readers(pcpu_rwlock);write_lock(&pcpu_rwlock->global_rwlock);}voidpercpu_write_unlock(structpercpu_rwlock*pcpu_rwlock){+/*+*Informallreadersthatwearedone,sothattheycanswitchback+*totheirper-cpurefcounts.(Wedon'tneedtowaitforthemto+*seeit).+*/+announce_writer_inactive(pcpu_rwlock);write_unlock(&pcpu_rwlock->global_rwlock);}
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:36:14
If interrupt handlers can also be readers, then one of the ways to make
per-CPU rwlocks safe, is to disable interrupts at the reader side before
trying to acquire the per-CPU rwlock and keep it disabled throughout the
duration of the read-side critical section.
The goal is to avoid cases such as:
1. writer is active and it holds the global rwlock for write
2. a regular reader comes in and marks itself as present (by incrementing
its per-CPU refcount) before checking whether writer is active.
3. an interrupt hits the reader;
[If it had not hit, the reader would have noticed that the writer is
active and would have decremented its refcount and would have tried
to acquire the global rwlock for read].
Since the interrupt handler also happens to be a reader, it notices
the non-zero refcount (which was due to the reader who got interrupted)
and thinks that this is a nested read-side critical section and
proceeds to take the fastpath, which is wrong. The interrupt handler
should have noticed that the writer is active and taken the rwlock
for read.
So, disabling interrupts can help avoid this problem (at the cost of keeping
the interrupts disabled for quite long).
But Oleg had a brilliant idea by which we can do much better than that:
we can manage with disabling interrupts _just_ during the updates (writes to
per-CPU refcounts) to safe-guard against races with interrupt handlers.
Beyond that, we can keep the interrupts enabled and still be safe w.r.t
interrupt handlers that can act as readers.
Basically the idea is that we differentiate between the *part* of the
per-CPU refcount that we use for reference counting vs the part that we use
merely to make the writer wait for us to switch over to the right
synchronization scheme.
The scheme involves splitting the per-CPU refcounts into 2 parts:
eg: the lower 16 bits are used to track the nesting depth of the reader
(a "nested-counter"), and the remaining (upper) bits are used to merely mark
the presence of the reader.
As long as the overall reader_refcnt is non-zero, the writer waits for the
reader (assuming that the reader is still actively using per-CPU refcounts for
synchronization).
The reader first sets one of the higher bits to mark its presence, and then
uses the lower 16 bits to manage the nesting depth. So, an interrupt handler
coming in as illustrated above will be able to distinguish between "this is
a nested read-side critical section" vs "we have merely marked our presence
to make the writer wait for us to switch" by looking at the same refcount.
Thus, it makes it unnecessary to keep interrupts disabled throughout the
read-side critical section, despite having the possibility of interrupt
handlers being readers themselves.
Implement this logic and rename the locking functions appropriately, to
reflect what they do.
Based-on-idea-by: Oleg Nesterov [off-list ref]
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/percpu-rwlock.h | 15 ++++++++++-----
lib/percpu-rwlock.c | 41 +++++++++++++++++++++++++++--------------
2 files changed, 37 insertions(+), 19 deletions(-)
@@ -62,19 +62,19 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)pcpu_rwlock->writer_signal=NULL;}-voidpercpu_read_lock(structpercpu_rwlock*pcpu_rwlock)+voidpercpu_read_lock_irqsafe(structpercpu_rwlock*pcpu_rwlock){preempt_disable();/* First and foremost, let the writer know that a reader is active */-this_cpu_inc(*pcpu_rwlock->reader_refcnt);+this_cpu_add(*pcpu_rwlock->reader_refcnt,READER_PRESENT);/**Ifwearealreadyusingper-cpurefcounts,itisnotsafetoswitch*thesynchronizationscheme.Socontinueusingtherefcounts.*/if(reader_nested_percpu(pcpu_rwlock)){-gotoout;+this_cpu_inc(*pcpu_rwlock->reader_refcnt);}else{/**Thewriteto'reader_refcnt'mustbevisiblebeforewe
@@ -83,9 +83,19 @@ void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)smp_mb();/* Paired with smp_rmb() in sync_reader() */if(likely(!writer_active(pcpu_rwlock))){-gotoout;+this_cpu_inc(*pcpu_rwlock->reader_refcnt);}else{/* Writer is active, so switch to global rwlock. */++/*+*Whilewearespinningon->global_rwlock,an+*interruptcanhitus,andtheinterrupthandler+*mightcallthisfunction.Thedistinctionbetween+*READER_PRESENTandtherefcnthelpsensurethatthe+*interrupthandleralsotakesthisbranchandspins+*onthe->global_rwlock,aslongasthewriteris+*active.+*/read_lock(&pcpu_rwlock->global_rwlock);/*
@@ -95,26 +105,27 @@ void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)*backtoper-cpurefcounts.(Thisalsohelpsavoid*heterogeneousnestingofreaders).*/-if(writer_active(pcpu_rwlock))-this_cpu_dec(*pcpu_rwlock->reader_refcnt);-else+if(!writer_active(pcpu_rwlock)){+this_cpu_inc(*pcpu_rwlock->reader_refcnt);read_unlock(&pcpu_rwlock->global_rwlock);+}}}-out:+this_cpu_sub(*pcpu_rwlock->reader_refcnt,READER_PRESENT);+/* Prevent reordering of any subsequent reads */smp_rmb();}-voidpercpu_read_unlock(structpercpu_rwlock*pcpu_rwlock)+voidpercpu_read_unlock_irqsafe(structpercpu_rwlock*pcpu_rwlock){/**Weneverallowheterogeneousnestingofreaders.Soitistrivial*tofindoutthekindofreaderweare,andundotheoperation*donebyourcorrespondingpercpu_read_lock().*/-if(__this_cpu_read(*pcpu_rwlock->reader_refcnt)){+if(reader_nested_percpu(pcpu_rwlock)){this_cpu_dec(*pcpu_rwlock->reader_refcnt);smp_wmb();/* Paired with smp_rmb() in sync_reader() */}else{
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:36:38
CPU hotplug (which will be the first user of per-CPU rwlocks) has a special
requirement with respect to locking: the writer, after acquiring the per-CPU
rwlock for write, must be allowed to take the same lock for read, without
deadlocking and without getting complaints from lockdep. In comparison, this
is similar to what get_online_cpus()/put_online_cpus() does today: it allows
a hotplug writer (who holds the cpu_hotplug.lock mutex) to invoke it without
locking issues, because it silently returns if the caller is the hotplug
writer itself.
This can be easily achieved with per-CPU rwlocks as well (even without a
"is this a writer?" check) by incrementing the per-CPU refcount of the writer
immediately after taking the global rwlock for write, and then decrementing
the per-CPU refcount before releasing the global rwlock.
This ensures that any reader that comes along on that CPU while the writer is
active (on that same CPU), notices the non-zero value of the nested counter
and assumes that it is a nested read-side critical section and proceeds by
just incrementing the refcount. Thus we prevent the reader from taking the
global rwlock for read, which prevents the writer from deadlocking itself.
Add that support and teach lockdep about this special locking scheme so
that it knows that this sort of usage is valid. Also add the required lockdep
annotations to enable it to detect common locking problems with per-CPU
rwlocks.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
lib/percpu-rwlock.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
@@ -84,6 +84,10 @@ void percpu_read_lock_irqsafe(struct percpu_rwlock *pcpu_rwlock)if(likely(!writer_active(pcpu_rwlock))){this_cpu_inc(*pcpu_rwlock->reader_refcnt);++/* Pretend that we take global_rwlock for lockdep */+rwlock_acquire_read(&pcpu_rwlock->global_rwlock.dep_map,+0,0,_RET_IP_);}else{/* Writer is active, so switch to global rwlock. */
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:36:56
There are places where preempt_disable() or local_irq_disable() are used
to prevent any CPU from going offline during the critical section. Let us
call them as "atomic hotplug readers" ("atomic" because they run in atomic,
non-preemptible contexts).
Today, preempt_disable() or its equivalent works because the hotplug writer
uses stop_machine() to take CPUs offline. But once stop_machine() is gone
from the CPU hotplug offline path, the readers won't be able to prevent
CPUs from going offline using preempt_disable().
So the intent here is to provide synchronization APIs for such atomic hotplug
readers, to prevent (any) CPUs from going offline, without depending on
stop_machine() at the writer-side. The new APIs will look something like
this: get_online_cpus_atomic() and put_online_cpus_atomic()
Some important design requirements and considerations:
-----------------------------------------------------
1. Scalable synchronization at the reader-side, especially in the fast-path
Any synchronization at the atomic hotplug readers side must be highly
scalable - avoid global single-holder locks/counters etc. Because, these
paths currently use the extremely fast preempt_disable(); our replacement
to preempt_disable() should not become ridiculously costly and also should
not serialize the readers among themselves needlessly.
At a minimum, the new APIs must be extremely fast at the reader side
atleast in the fast-path, when no CPU offline writers are active.
2. preempt_disable() was recursive. The replacement should also be recursive.
3. No (new) lock-ordering restrictions
preempt_disable() was super-flexible. It didn't impose any ordering
restrictions or rules for nesting. Our replacement should also be equally
flexible and usable.
4. No deadlock possibilities
Regular per-cpu locking is not the way to go if we want to have relaxed
rules for lock-ordering. Because, we can end up in circular-locking
dependencies as explained in https://lkml.org/lkml/2012/12/6/290
So, avoid the usual per-cpu locking schemes (per-cpu locks/per-cpu atomic
counters with spin-on-contention etc) as much as possible, to avoid
numerous deadlock possibilities from creeping in.
Implementation of the design:
----------------------------
We use per-CPU reader-writer locks for synchronization because:
a. They are quite fast and scalable in the fast-path (when no writers are
active), since they use fast per-cpu counters in those paths.
b. They are recursive at the reader side.
c. They provide a good amount of safety against deadlocks; they don't
spring new deadlock possibilities on us from out of nowhere. As a
result, they have relaxed locking rules and are quite flexible, and
thus are best suited for replacing usages of preempt_disable() or
local_irq_disable() at the reader side.
Together, these satisfy all the requirements mentioned above.
I'm indebted to Michael Wang and Xiao Guangrong for their numerous thoughtful
suggestions and ideas, which inspired and influenced many of the decisions in
this as well as previous designs. Thanks a lot Michael and Xiao!
Cc: Russell King <redacted>
Cc: Mike Frysinger <redacted>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Ralf Baechle <redacted>
Cc: David Howells <dhowells@redhat.com>
Cc: "James E.J. Bottomley" <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Martin Schwidefsky <redacted>
Cc: Paul Mundt <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: uclinux-dist-devel@blackfin.uclinux.org
Cc: linux-ia64@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linux-am33-list@redhat.com
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: sparclinux@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/arm/Kconfig | 1 +
arch/blackfin/Kconfig | 1 +
arch/ia64/Kconfig | 1 +
arch/mips/Kconfig | 1 +
arch/mn10300/Kconfig | 1 +
arch/parisc/Kconfig | 1 +
arch/powerpc/Kconfig | 1 +
arch/s390/Kconfig | 1 +
arch/sh/Kconfig | 1 +
arch/sparc/Kconfig | 1 +
arch/x86/Kconfig | 1 +
include/linux/cpu.h | 4 +++
kernel/cpu.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
13 files changed, 69 insertions(+), 3 deletions(-)
@@ -198,6 +200,8 @@ static inline void cpu_hotplug_driver_unlock(void)#define get_online_cpus() do { } while (0)#define put_online_cpus() do { } while (0)+#define get_online_cpus_atomic() do { } while (0)+#define put_online_cpus_atomic() do { } while (0)#define hotcpu_notifier(fn, pri) do { (void)(fn); } while (0)/* These aren't inline functions due to a GCC bug. */#define register_hotcpu_notifier(nb) ({ (void)(nb); 0; })
@@ -246,15 +291,21 @@ struct take_cpu_down_param {staticint__reftake_cpu_down(void*_param){structtake_cpu_down_param*param=_param;-interr;+unsignedlongflags;+interr=0;++percpu_write_lock_irqsave(&hotplug_pcpu_rwlock,&flags);/* Ensure this CPU doesn't handle any more interrupts. */err=__cpu_disable();if(err<0)-returnerr;+gotoout;cpu_notify(CPU_DYING|param->mod,param->hcpu);-return0;++out:+percpu_write_unlock_irqrestore(&hotplug_pcpu_rwlock,&flags);+returnerr;}/* Requires cpu_add_remove_lock to be held */
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:37:04
On 12/05/2012 06:10 AM, Andrew Morton wrote:
"static inline C functions would be preferred if possible. Feel free to
fix up the wrong crufty surrounding code as well ;-)"
Convert the macros in the CPU hotplug code to static inline C functions.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/cpu.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -198,10 +198,10 @@ static inline void cpu_hotplug_driver_unlock(void)#else /* CONFIG_HOTPLUG_CPU */-#define get_online_cpus() do { } while (0)-#define put_online_cpus() do { } while (0)-#define get_online_cpus_atomic() do { } while (0)-#define put_online_cpus_atomic() do { } while (0)+staticinlinevoidget_online_cpus(void){}+staticinlinevoidput_online_cpus(void){}+staticinlinevoidget_online_cpus_atomic(void){}+staticinlinevoidput_online_cpus_atomic(void){}#define hotcpu_notifier(fn, pri) do { (void)(fn); } while (0)/* These aren't inline functions due to a GCC bug. */#define register_hotcpu_notifier(nb) ({ (void)(nb); 0; })
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:37:11
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() to prevent CPUs from going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/smp.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
@@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,conststructcpumask*nodemask;intret;+get_online_cpus_atomic();/* Try for same CPU (cheapest) */-cpu=get_cpu();+cpu=smp_processor_id();+if(cpumask_test_cpu(cpu,mask))gotocall;
@@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,cpu=cpumask_any_and(mask,cpu_online_mask);call:ret=smp_call_function_single(cpu,func,info,wait);-put_cpu();+put_online_cpus_atomic();returnret;}EXPORT_SYMBOL_GPL(smp_call_function_any);
@@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,/* No online cpus? We're done. */if(cpu>=nr_cpu_ids)-return;+gotoout_unlock;/* Do we have another CPU which isn't us? */next_cpu=cpumask_next_and(cpu,mask,cpu_online_mask);if(next_cpu==this_cpu)-next_cpu=cpumask_next_and(next_cpu,mask,cpu_online_mask);+next_cpu=cpumask_next_and(next_cpu,mask,+cpu_online_mask);/* Fastpath: do that cpu by itself. */if(next_cpu>=nr_cpu_ids){smp_call_function_single(cpu,func,info,wait);-return;+gotoout_unlock;}data=&__get_cpu_var(cfd_data);
@@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,/* Some callers race with other cpus changing the passed mask */if(unlikely(!refs)){csd_unlock(&data->csd);-return;+gotoout_unlock;}raw_spin_lock_irqsave(&call_function.lock,flags);
@@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,/* Optionally wait for the CPUs to complete */if(wait)csd_lock_wait(&data->csd);++out_unlock:+put_online_cpus_atomic();}EXPORT_SYMBOL(smp_call_function_many);
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:37:30
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() to prevent CPUs from going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/smp.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:37:52
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from going
offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/sched/core.c | 24 +++++++++++++++++++++---
kernel/sched/fair.c | 5 ++++-
kernel/timer.c | 2 ++
3 files changed, 27 insertions(+), 4 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:38:12
We need not use the raw_spin_lock_irqsave/restore primitives because
all CPU_DYING notifiers run with interrupts disabled. So just use
raw_spin_lock/unlock.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/sched/core.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:38:24
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/sched/rt.c | 3 +++
1 file changed, 3 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:38:44
Don't refer to stop_machine() in the CPU hotplug path, since we are going
to get rid of it. Also, move the comment referring to callback adoption
to the CPU_DEAD case, because that's where it happens now.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/rcutree.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:39:05
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/time/tick-broadcast.c | 2 ++
1 file changed, 2 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:39:30
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from going
offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: John Stultz <redacted>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/time/clocksource.c | 5 +++++
1 file changed, 5 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:39:51
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Frederic Weisbecker <redacted>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/softirq.c | 3 +++
1 file changed, 3 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:40:10
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/irq/manage.c | 7 +++++++
1 file changed, 7 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:40:31
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: netdev@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
net/core/dev.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:40:47
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
block/blk-softirq.c | 4 ++++
1 file changed, 4 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:40:59
The pcrypt_aead_init_tfm() function access the cpu_online_mask without
disabling CPU hotplug. And it looks like it can afford to sleep, so use
the get/put_online_cpus() APIs to protect against CPU hotplug.
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
crypto/pcrypt.c | 4 ++++
1 file changed, 4 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:41:17
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Roland Dreier <roland@kernel.org>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
drivers/infiniband/hw/ehca/ehca_irq.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:41:21
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Robert Love <redacted>
Cc: "James E.J. Bottomley" <redacted>
Cc: devel@open-fcoe.org
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
drivers/scsi/fcoe/fcoe.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:41:37
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: David Daney <redacted>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
drivers/staging/octeon/ethernet-rx.c | 3 +++
1 file changed, 3 insertions(+)
@@ -97,6 +98,7 @@ static void cvm_oct_enable_one_cpu(void)return;/* ... if a CPU is available, Turn on NAPI polling for that CPU. */+get_online_cpus_atomic();for_each_online_cpu(cpu){if(!cpu_test_and_set(cpu,core_state.cpu_state)){v=smp_call_function_single(cpu,cvm_oct_enable_napi,
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:41:46
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Daniel J Blueman <redacted>
Cc: Steffen Persvold <redacted>
Cc: Joerg Roedel <redacted>
Cc: linux-edac@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/x86/include/asm/ipi.h | 5 +++++
arch/x86/kernel/apic/apic_flat_64.c | 10 ++++++++++
arch/x86/kernel/apic/apic_numachip.c | 5 +++++
arch/x86/kernel/apic/es7000_32.c | 5 +++++
arch/x86/kernel/apic/io_apic.c | 7 +++++--
arch/x86/kernel/apic/ipi.c | 10 ++++++++++
arch/x86/kernel/apic/x2apic_cluster.c | 4 ++++
arch/x86/kernel/apic/x2apic_uv_x.c | 4 ++++
arch/x86/kernel/cpu/mcheck/therm_throt.c | 4 ++--
arch/x86/mm/tlb.c | 14 +++++++-------
10 files changed, 57 insertions(+), 11 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:42:14
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Marcelo Tosatti <redacted>
Cc: Gleb Natapov <redacted>
Cc: kvm@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
virt/kvm/kvm_main.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:42:38
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context (in vmx_vcpu_load() to prevent CPUs from
going offline while clearing vmcs).
Reported-by: Michael Wang <redacted>
Debugged-by: Xiao Guangrong [off-list ref]
Cc: Marcelo Tosatti <redacted>
Cc: Gleb Natapov <redacted>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/x86/kvm/vmx.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
@@ -1557,10 +1557,14 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)structvcpu_vmx*vmx=to_vmx(vcpu);u64phys_addr=__pa(per_cpu(vmxarea,cpu));-if(!vmm_exclusive)+if(!vmm_exclusive){kvm_cpu_vmxon(phys_addr);-elseif(vmx->loaded_vmcs->cpu!=cpu)+}elseif(vmx->loaded_vmcs->cpu!=cpu){+/* Prevent any CPU from going offline */+get_online_cpus_atomic();loaded_vmcs_clear(vmx->loaded_vmcs);+put_online_cpus_atomic();+}if(per_cpu(current_vmcs,cpu)!=vmx->loaded_vmcs->vmcs){per_cpu(current_vmcs,cpu)=vmx->loaded_vmcs->vmcs;
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:43:01
The CPU_DYING notifier modifies the per-cpu pointer pmu->box, and this can
race with functions such as uncore_pmu_to_box() and uncore_pci_remove() when
we remove stop_machine() from the CPU offline path. So protect them using
get/put_online_cpus_atomic().
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Arnaldo Carvalho de Melo <redacted>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/x86/kernel/cpu/perf_event_intel_uncore.c | 5 +++++
1 file changed, 5 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:43:11
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Konrad Rzeszutek Wilk <redacted>
Cc: Jeremy Fitzhardinge <redacted>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: xen-devel@lists.xensource.com
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/x86/xen/mmu.c | 11 +++++++++--
arch/x86/xen/smp.c | 9 +++++++++
2 files changed, 18 insertions(+), 2 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:43:18
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Also, remove the non-ASCII character present in this file!
Cc: linux-alpha@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/alpha/kernel/smp.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:43:37
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Mike Frysinger <redacted>
Cc: Bob Liu <redacted>
Cc: Steven Miao <redacted>
Cc: uclinux-dist-devel@blackfin.uclinux.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/blackfin/mach-common/smp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:44:11
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Mikael Starvik <redacted>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: linux-cris-kernel@axis.com
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/cris/arch-v32/kernel/smp.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:44:20
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Richard Kuo <redacted>
Cc: linux-hexagon@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/hexagon/kernel/smp.c | 5 +++++
1 file changed, 5 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:44:28
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <redacted>
Cc: linux-ia64@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/ia64/kernel/irq_ia64.c | 13 +++++++++++++
arch/ia64/kernel/perfmon.c | 6 ++++++
arch/ia64/kernel/smp.c | 23 ++++++++++++++++-------
arch/ia64/mm/tlb.c | 6 ++++--
4 files changed, 39 insertions(+), 9 deletions(-)
@@ -302,9 +307,11 @@ int irq_prepare_move(int irq, int cpu)unsignedlongflags;intret;+get_online_cpus_atomic();spin_lock_irqsave(&vector_lock,flags);ret=__irq_prepare_move(irq,cpu);spin_unlock_irqrestore(&vector_lock,flags);+put_online_cpus_atomic();returnret;}
@@ -293,12 +301,13 @@ voidsmp_flush_tlb_mm(structmm_struct*mm){cpumask_var_tcpus;-preempt_disable();++get_online_cpus_atomic();/* this happens for the common case of a single-threaded fork(): */if(likely(mm==current->active_mm&&atomic_read(&mm->mm_users)==1)){local_finish_flush_tlb_mm(mm);-preempt_enable();+put_online_cpus_atomic();return;}if(!alloc_cpumask_var(&cpus,GFP_ATOMIC)){
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:44:40
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Hirokazu Takata <redacted>
Cc: linux-m32r@ml.linux-m32r.org
Cc: linux-m32r-ja@ml.linux-m32r.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/m32r/kernel/smp.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:45:06
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs fom
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Ralf Baechle <redacted>
Cc: David Daney <redacted>
Cc: linux-mips@linux-mips.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/mips/kernel/cevt-smtc.c | 8 ++++++++
arch/mips/kernel/smp.c | 16 ++++++++--------
arch/mips/kernel/smtc.c | 3 +++
arch/mips/mm/c-octeon.c | 4 ++--
4 files changed, 21 insertions(+), 10 deletions(-)
@@ -84,6 +85,8 @@ static int mips_next_event(unsigned long delta,unsignedlongnextcomp=0L;intvpe=current_cpu_data.vpe_id;intcpu=smp_processor_id();++get_online_cpus_atomic();local_irq_save(flags);mtflags=dmt();
@@ -164,6 +167,7 @@ static int mips_next_event(unsigned long delta,}emt(mtflags);local_irq_restore(flags);+put_online_cpus_atomic();return0;}
@@ -221,6 +226,9 @@ repeat:}}++put_online_cpus_atomic()+/* Reprogram for interrupt at next soonest timestamp for VPE */if(ISVALID(nextstamp)){write_c0_compare(nextstamp);
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:45:29
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: David Howells <dhowells@redhat.com>
Cc: Koichi Yasutake <redacted>
Cc: linux-am33-list@redhat.com
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/mn10300/kernel/smp.c | 2 ++
arch/mn10300/mm/cache-smp.c | 5 +++++
arch/mn10300/mm/tlb-smp.c | 15 +++++++++------
3 files changed, 16 insertions(+), 6 deletions(-)
@@ -94,6 +95,8 @@ void smp_cache_call(unsigned long opr_mask,smp_cache_mask=opr_mask;smp_cache_start=start;smp_cache_end=end;++get_online_cpus_atomic();cpumask_copy(&smp_cache_ipi_map,cpu_online_mask);cpumask_clear_cpu(smp_processor_id(),&smp_cache_ipi_map);
@@ -102,4 +105,6 @@ void smp_cache_call(unsigned long opr_mask,while(!cpumask_empty(&smp_cache_ipi_map))/* nothing. lockup detection does not belong here */mb();++put_online_cpus_atomic();}
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:45:39
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: "James E.J. Bottomley" <redacted>
Cc: linux-parisc@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/parisc/kernel/smp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:45:59
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/powerpc/mm/mmu_context_nohash.c | 2 ++
1 file changed, 2 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:46:13
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Paul Mundt <redacted>
Cc: linux-sh@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/sh/kernel/smp.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:46:25
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Sam Ravnborg <redacted>
Cc: sparclinux@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/sparc/kernel/leon_smp.c | 2 ++
arch/sparc/kernel/smp_64.c | 9 +++++----
arch/sparc/kernel/sun4d_smp.c | 2 ++
arch/sparc/kernel/sun4m_smp.c | 3 +++
4 files changed, 12 insertions(+), 4 deletions(-)
@@ -420,6 +420,7 @@ static void leon_cross_call(smpfunc_t func, cpumask_t mask, unsigned long arg1,unsignedlongflags;spin_lock_irqsave(&cross_call_lock,flags);+get_online_cpus_atomic();{/* If you make changes here, make sure gcc generates proper code... */
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:46:45
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Chris Metcalf <redacted>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
arch/tile/kernel/smp.c | 4 ++++
1 file changed, 4 insertions(+)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:47:09
From: Paul E. McKenney <redacted>
The _cpu_down() function invoked as part of the CPU-hotplug offlining
process currently invokes __stop_machine(), which is slow and inflicts
substantial real-time latencies on the entire system. This patch
substitutes stop_one_cpu() for __stop_machine() in order to improve
both performance and real-time latency.
There were a number of uses of preempt_disable() or local_irq_disable()
that were intended to block CPU-hotplug offlining. These were fixed by
using get/put_online_cpus_atomic(), which is the new synchronization
primitive to prevent CPU offline, while invoking from atomic context.
Signed-off-by: Paul E. McKenney <redacted>
Signed-off-by: Paul E. McKenney <redacted>
[ srivatsa.bhat@linux.vnet.ibm.com: Refer to the new sync primitives for
readers (in the changelog); s/stop_cpus/stop_one_cpu and fix comment
referring to stop_machine in the code]
Signed-off-by: Srivatsa S. Bhat <redacted>
---
kernel/cpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -337,7 +337,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)}smpboot_park_threads(cpu);-err=__stop_machine(take_cpu_down,&tcd_param,cpumask_of(cpu));+err=stop_one_cpu(cpu,take_cpu_down,&tcd_param);if(err){/* CPU didn't die: tell everyone. Can't complain. */smpboot_unpark_threads(cpu);
@@ -349,7 +349,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)/**Themigration_call()CPU_DYINGcallbackwillhaveremovedall*runnabletasksfromthecpu,there'sonlytheidletaskleftnow-*thatthemigrationthreadisdonedoingthestop_machinething.+*thatthemigrationthreadisdonedoingthestop_one_cpu()thing.**Waitforthestopthreadtogoaway.*/
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:47:36
... and also cleanup a comment that refers to CPU hotplug being dependent on
stop_machine().
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
include/linux/stop_machine.h | 2 +-
init/Kconfig | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 07:48:07
Since stop_machine() is no longer used in the CPU offline path, we cannot
disable CPU hotplug using preempt_disable()/local_irq_disable() etc. We
need to use the newly introduced get/put_online_cpus_atomic() APIs.
Reflect this in the documentation.
Cc: Rob Landley <redacted>
Cc: linux-doc@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
---
Documentation/cpu-hotplug.txt | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
@@ -113,13 +113,15 @@ Never use anything other than cpumask_t to represent bitmap of CPUs. #include <linux/cpu.h> get_online_cpus() and put_online_cpus():-The above calls are used to inhibit cpu hotplug operations. While the+The above calls are used to inhibit cpu hotplug operations, when invoked from+non-atomic context (because the above functions can sleep). While the cpu_hotplug.refcount is non zero, the cpu_online_mask will not change.-If you merely need to avoid cpus going away, you could also use-preempt_disable() and preempt_enable() for those sections.-Just remember the critical section cannot call any-function that can sleep or schedule this process away. The preempt_disable()-will work as long as stop_machine_run() is used to take a cpu down.++However, if you are executing in atomic context (ie., you can't afford to+sleep), and you merely need to avoid cpus going offline, you can use+get_online_cpus_atomic() and put_online_cpus_atomic() for those sections.+Just remember the critical section cannot call any function that can sleep or+schedule this process away. CPU Hotplug - Frequently Asked Questions.
@@ -360,6 +362,9 @@ A: There are two ways. If your code can be run in interrupt context, use return err; }+ If my_func_on_cpu() itself cannot block, use get/put_online_cpus_atomic()+ instead of get/put_online_cpus() to prevent CPUs from going offline.+ Q: How do we determine how many CPUs are available for hotplug. A: There is no clear spec defined way from ACPI that can give us that information today. Based on some input from Natalie of Unisys,
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2013-01-22 18:45:21
On Tue, 22 Jan 2013 13:03:22 +0530
"Srivatsa S. Bhat" [off-list ref] wrote:
A straight-forward (and obvious) algorithm to implement Per-CPU Reader-Writer
locks can also lead to too many deadlock possibilities which can make it very
hard/impossible to use. This is explained in the example below, which helps
justify the need for a different algorithm to implement flexible Per-CPU
Reader-Writer locks.
We can use global rwlocks as shown below safely, without fear of deadlocks:
Readers:
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(&my_rwlock);
2. read_lock(&my_rwlock); spin_lock(&random_lock);
Writer:
CPU 2:
------
write_lock(&my_rwlock);
We can observe that there is no possibility of deadlocks or circular locking
dependencies here. Its perfectly safe.
Now consider a blind/straight-forward conversion of global rwlocks to per-CPU
rwlocks like this:
The reader locks its own per-CPU rwlock for read, and proceeds.
Something like: read_lock(per-cpu rwlock of this cpu);
The writer acquires all per-CPU rwlocks for write and only then proceeds.
Something like:
for_each_online_cpu(cpu)
write_lock(per-cpu rwlock of 'cpu');
Now let's say that for performance reasons, the above scenario (which was
perfectly safe when using global rwlocks) was converted to use per-CPU rwlocks.
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(my_rwlock of CPU 1);
2. read_lock(my_rwlock of CPU 0); spin_lock(&random_lock);
Writer:
CPU 2:
------
for_each_online_cpu(cpu)
write_lock(my_rwlock of 'cpu');
Consider what happens if the writer begins his operation in between steps 1
and 2 at the reader side. It becomes evident that we end up in a (previously
non-existent) deadlock due to a circular locking dependency between the 3
entities, like this:
(holds Waiting for
random_lock) CPU 0 -------------> CPU 2 (holds my_rwlock of CPU 0
for write)
^ |
| |
Waiting| | Waiting
for | | for
| V
------ CPU 1 <------
(holds my_rwlock of
CPU 1 for read)
So obviously this "straight-forward" way of implementing percpu rwlocks is
deadlock-prone. One simple measure for (or characteristic of) safe percpu
rwlock should be that if a user replaces global rwlocks with per-CPU rwlocks
(for performance reasons), he shouldn't suddenly end up in numerous deadlock
possibilities which never existed before. The replacement should continue to
remain safe, and perhaps improve the performance.
Observing the robustness of global rwlocks in providing a fair amount of
deadlock safety, we implement per-CPU rwlocks as nothing but global rwlocks,
as a first step.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
We got rid of brlock years ago, do we have to reintroduce it like this?
The problem was that brlock caused starvation.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2013-01-22 19:32:39
On Tue, 2013-01-22 at 13:03 +0530, Srivatsa S. Bhat wrote:
A straight-forward (and obvious) algorithm to implement Per-CPU Reader-Writer
locks can also lead to too many deadlock possibilities which can make it very
hard/impossible to use. This is explained in the example below, which helps
justify the need for a different algorithm to implement flexible Per-CPU
Reader-Writer locks.
We can use global rwlocks as shown below safely, without fear of deadlocks:
Readers:
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(&my_rwlock);
2. read_lock(&my_rwlock); spin_lock(&random_lock);
Writer:
CPU 2:
------
write_lock(&my_rwlock);
I thought global locks are now fair. That is, a reader will block if a
writer is waiting. Hence, the above should deadlock on the current
rwlock_t types.
We need to fix those locations (or better yet, remove all rwlocks ;-)
-- Steve
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 19:44:00
On 01/23/2013 12:15 AM, Stephen Hemminger wrote:
On Tue, 22 Jan 2013 13:03:22 +0530
"Srivatsa S. Bhat" [off-list ref] wrote:
quoted
A straight-forward (and obvious) algorithm to implement Per-CPU Reader-Writer
locks can also lead to too many deadlock possibilities which can make it very
hard/impossible to use. This is explained in the example below, which helps
justify the need for a different algorithm to implement flexible Per-CPU
Reader-Writer locks.
We can use global rwlocks as shown below safely, without fear of deadlocks:
Readers:
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(&my_rwlock);
2. read_lock(&my_rwlock); spin_lock(&random_lock);
Writer:
CPU 2:
------
write_lock(&my_rwlock);
We can observe that there is no possibility of deadlocks or circular locking
dependencies here. Its perfectly safe.
Now consider a blind/straight-forward conversion of global rwlocks to per-CPU
rwlocks like this:
The reader locks its own per-CPU rwlock for read, and proceeds.
Something like: read_lock(per-cpu rwlock of this cpu);
The writer acquires all per-CPU rwlocks for write and only then proceeds.
Something like:
for_each_online_cpu(cpu)
write_lock(per-cpu rwlock of 'cpu');
Now let's say that for performance reasons, the above scenario (which was
perfectly safe when using global rwlocks) was converted to use per-CPU rwlocks.
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(my_rwlock of CPU 1);
2. read_lock(my_rwlock of CPU 0); spin_lock(&random_lock);
Writer:
CPU 2:
------
for_each_online_cpu(cpu)
write_lock(my_rwlock of 'cpu');
Consider what happens if the writer begins his operation in between steps 1
and 2 at the reader side. It becomes evident that we end up in a (previously
non-existent) deadlock due to a circular locking dependency between the 3
entities, like this:
(holds Waiting for
random_lock) CPU 0 -------------> CPU 2 (holds my_rwlock of CPU 0
for write)
^ |
| |
Waiting| | Waiting
for | | for
| V
------ CPU 1 <------
(holds my_rwlock of
CPU 1 for read)
So obviously this "straight-forward" way of implementing percpu rwlocks is
deadlock-prone. One simple measure for (or characteristic of) safe percpu
rwlock should be that if a user replaces global rwlocks with per-CPU rwlocks
(for performance reasons), he shouldn't suddenly end up in numerous deadlock
possibilities which never existed before. The replacement should continue to
remain safe, and perhaps improve the performance.
Observing the robustness of global rwlocks in providing a fair amount of
deadlock safety, we implement per-CPU rwlocks as nothing but global rwlocks,
as a first step.
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
We got rid of brlock years ago, do we have to reintroduce it like this?
The problem was that brlock caused starvation.
Um? I still see it in include/linux/lglock.h and its users in fs/ directory.
BTW, I'm not advocating that everybody start converting their global reader-writer
locks to per-cpu rwlocks, because such a conversion probably won't make sense
in all scenarios.
The thing is, for CPU hotplug in particular, the "preempt_disable() at the reader;
stop_machine() at the writer" scheme had some very desirable properties at the
reader side (even though people might hate stop_machine() with all their
heart ;-)), namely :
At the reader side:
o No need to hold locks to prevent CPU offline
o Extremely fast/optimized updates (the preempt count)
o No need for heavy memory barriers
o Extremely flexible nesting rules
So this made perfect sense at the reader for CPU hotplug, because it is expected
that CPU hotplug operations are very infrequent, and it is well-known that quite
a few atomic hotplug readers are in very hot paths. The problem was that the
stop_machine() at the writer was not only a little too heavy, but also inflicted
real-time latencies on the system because it needed cooperation from _all_ CPUs
synchronously, to take one CPU down.
So the idea is to get rid of stop_machine() without hurting the reader side.
And this scheme of per-cpu rwlocks comes close to ensuring that. (You can look
at the previous versions of this patchset [links given in cover letter] to see
what other schemes we hashed out before coming to this one).
The only reason I exposed this as a generic locking scheme was because Tejun
pointed out that, complex locking schemes implemented in individual subsystems
is not such a good idea. And also this comes at a time when per-cpu rwsemaphores
have just been introduced in the kernel and Oleg had ideas about converting the
cpu hotplug (sleepable) locking to use them.
Regards,
Srivatsa S. Bhat
From: Srivatsa S. Bhat <hidden> Date: 2013-01-22 20:00:30
On 01/23/2013 01:02 AM, Steven Rostedt wrote:
On Tue, 2013-01-22 at 13:03 +0530, Srivatsa S. Bhat wrote:
quoted
A straight-forward (and obvious) algorithm to implement Per-CPU Reader-Writer
locks can also lead to too many deadlock possibilities which can make it very
hard/impossible to use. This is explained in the example below, which helps
justify the need for a different algorithm to implement flexible Per-CPU
Reader-Writer locks.
We can use global rwlocks as shown below safely, without fear of deadlocks:
Readers:
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(&my_rwlock);
2. read_lock(&my_rwlock); spin_lock(&random_lock);
Writer:
CPU 2:
------
write_lock(&my_rwlock);
I thought global locks are now fair. That is, a reader will block if a
writer is waiting. Hence, the above should deadlock on the current
rwlock_t types.
Oh is it? Last I checked, lockdep didn't complain about this ABBA scenario!
We need to fix those locations (or better yet, remove all rwlocks ;-)
:-)
The challenge with stop_machine() removal is that the replacement on the
reader side must have the (locking) flexibility comparable to preempt_disable().
Otherwise, that solution most likely won't be viable because we'll hit way
too many locking problems and go crazy by the time we convert them over..(if
we can, that is!)
Regards,
Srivatsa S. Bhat
From: Steven Rostedt <rostedt@goodmis.org> Date: 2013-01-22 20:54:29
On Wed, 2013-01-23 at 01:28 +0530, Srivatsa S. Bhat wrote:
quoted
I thought global locks are now fair. That is, a reader will block if a
writer is waiting. Hence, the above should deadlock on the current
rwlock_t types.
Oh is it? Last I checked, lockdep didn't complain about this ABBA scenario!
It doesn't and Peter Zijlstra said we need to fix that ;-) It only
recently became an issue with the new "fair" locking of rwlocks.
-- Steve
Hello, Srivatsa.
First of all, I'm not sure whether we need to be this step-by-step
when introducing something new. It's not like we're transforming an
existing implementation and it doesn't seem to help understanding the
series that much either.
On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
lock-ordering related problems (unlike per-cpu locks). However, global
So, unfortunately, this already seems broken, right? The problem here
seems to be that previously, say, read_lock() implied
preempt_disable() but as this series aims to move away from it, it
introduces the problem of locking order between such locks and the new
contruct.
The only two options are either punishing writers or identifying and
updating all such possible deadlocks. percpu_rwsem does the former,
right? I don't know how feasible the latter would be. Srivatsa,
you've been looking at all the places which would require conversion,
how difficult would doing the latter be?
It could be just personal preference but I find the above one line
wrappers more obfuscating than anything else. What's the point of
wrapping writer_signal = true/false into a separate function? These
simple wrappers just add layers that people have to dig through to
figure out what's going on without adding anything of value. I'd much
prefer collapsing these into the percpu_write_[un]lock().
Thanks.
--
tejun
From: Srivatsa S. Bhat <hidden> Date: 2013-01-23 19:36:02
On 01/24/2013 12:25 AM, Tejun Heo wrote:
Hello, Srivatsa.
First of all, I'm not sure whether we need to be this step-by-step
when introducing something new. It's not like we're transforming an
existing implementation and it doesn't seem to help understanding the
series that much either.
Hmm.. I split it up into steps to help explain the reasoning behind
the code sufficiently, rather than spring all of the intricacies at
one go (which would make it very hard to write the changelog/comments
also). The split made it easier for me to document it well in the
changelog, because I could deal with reasonable chunks of code/complexity
at a time. IMHO that helps people reading it for the first time to
understand the logic easily.
On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
quoted
Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
lock-ordering related problems (unlike per-cpu locks). However, global
So, unfortunately, this already seems broken, right? The problem here
seems to be that previously, say, read_lock() implied
preempt_disable() but as this series aims to move away from it, it
introduces the problem of locking order between such locks and the new
contruct.
Not sure I got your point correctly. Are you referring to Steve's comment
that rwlocks are probably fair now (and hence not really safe when used
like this)? If yes, I haven't actually verified that yet, but yes, that
will make this hard to use, since we need to take care of locking rules.
But suppose rwlocks are unfair (as I had assumed them to be), then we
have absolutely no problems and no lock-ordering to worry about.
The only two options are either punishing writers or identifying and
updating all such possible deadlocks. percpu_rwsem does the former,
right? I don't know how feasible the latter would be.
I don't think we can avoid looking into all the possible deadlocks,
as long as we use rwlocks inside get/put_online_cpus_atomic() (assuming
rwlocks are fair). Even with Oleg's idea of using synchronize_sched()
at the writer, we still need to take care of locking rules, because the
synchronize_sched() only helps avoid the memory barriers at the reader,
and doesn't help get rid of the rwlocks themselves.
So in short, I don't see how we can punish the writers and thereby somehow
avoid looking into possible deadlocks (if rwlocks are fair).
Srivatsa,
you've been looking at all the places which would require conversion,
how difficult would doing the latter be?
The problem is that some APIs like smp_call_function() will need to use
get/put_online_cpus_atomic(). That is when the locking becomes tricky
in the subsystem which invokes these APIs with other (subsystem-specific,
internal) locks held. So we could potentially use a convention such as
"Make get/put_online_cpus_atomic() your outer-most calls, within which
you nest the other locks" to rule out all ABBA deadlock possibilities...
But we might still hit some hard-to-convert places..
BTW, Steve, fair rwlocks doesn't mean the following scenario will result
in a deadlock right?
CPU 0 CPU 1
read_lock(&rwlock)
write_lock(&rwlock) //spins, because CPU 0
//has acquired the lock for read
read_lock(&rwlock)
^^^^^
What happens here? Does CPU 0 start spinning (and hence deadlock) or will
it continue realizing that it already holds the rwlock for read?
If the above ends in a deadlock, then its next to impossible to convert
all the places safely (because the above mentioned convention will simply
fall apart).
It could be just personal preference but I find the above one line
wrappers more obfuscating than anything else. What's the point of
wrapping writer_signal = true/false into a separate function? These
simple wrappers just add layers that people have to dig through to
figure out what's going on without adding anything of value. I'd much
prefer collapsing these into the percpu_write_[un]lock().
Sure, I see your point. I'll change that.
Thanks a lot for your feedback Tejun!
Regards,
Srivatsa S. Bhat
Hello, Srivatsa.
On Thu, Jan 24, 2013 at 01:03:52AM +0530, Srivatsa S. Bhat wrote:
Hmm.. I split it up into steps to help explain the reasoning behind
the code sufficiently, rather than spring all of the intricacies at
one go (which would make it very hard to write the changelog/comments
also). The split made it easier for me to document it well in the
changelog, because I could deal with reasonable chunks of code/complexity
at a time. IMHO that helps people reading it for the first time to
understand the logic easily.
I don't know. It's a judgement call I guess. I personally would much
prefer having ample documentation as comments in the source itself or
as a separate Documentation/ file as that's what most people are gonna
be looking at to figure out what's going on. Maybe just compact it a
bit and add more in-line documentation instead?
quoted
The only two options are either punishing writers or identifying and
updating all such possible deadlocks. percpu_rwsem does the former,
right? I don't know how feasible the latter would be.
I don't think we can avoid looking into all the possible deadlocks,
as long as we use rwlocks inside get/put_online_cpus_atomic() (assuming
rwlocks are fair). Even with Oleg's idea of using synchronize_sched()
at the writer, we still need to take care of locking rules, because the
synchronize_sched() only helps avoid the memory barriers at the reader,
and doesn't help get rid of the rwlocks themselves.
Well, percpu_rwlock don't have to use rwlock for the slow path. It
can implement its own writer starving locking scheme. It's not like
implementing slow path global rwlock logic is difficult.
CPU 0 CPU 1
read_lock(&rwlock)
write_lock(&rwlock) //spins, because CPU 0
//has acquired the lock for read
read_lock(&rwlock)
^^^^^
What happens here? Does CPU 0 start spinning (and hence deadlock) or will
it continue realizing that it already holds the rwlock for read?
I don't think rwlock allows nesting write lock inside read lock.
read_lock(); write_lock() will always deadlock.
Thanks.
--
tejun
From: Michel Lespinasse <hidden> Date: 2013-01-24 04:15:10
On Tue, Jan 22, 2013 at 11:32 AM, Steven Rostedt [off-list ref] wrote:
On Tue, 2013-01-22 at 13:03 +0530, Srivatsa S. Bhat wrote:
quoted
A straight-forward (and obvious) algorithm to implement Per-CPU Reader-Writer
locks can also lead to too many deadlock possibilities which can make it very
hard/impossible to use. This is explained in the example below, which helps
justify the need for a different algorithm to implement flexible Per-CPU
Reader-Writer locks.
We can use global rwlocks as shown below safely, without fear of deadlocks:
Readers:
CPU 0 CPU 1
------ ------
1. spin_lock(&random_lock); read_lock(&my_rwlock);
2. read_lock(&my_rwlock); spin_lock(&random_lock);
Writer:
CPU 2:
------
write_lock(&my_rwlock);
I thought global locks are now fair. That is, a reader will block if a
writer is waiting. Hence, the above should deadlock on the current
rwlock_t types.
I believe you are mistaken here. struct rw_semaphore is fair (and
blocking), but rwlock_t is unfair. The reason we can't easily make
rwlock_t fair is because tasklist_lock currently depends on the
rwlock_t unfairness - tasklist_lock readers typically don't disable
local interrupts, and tasklist_lock may be acquired again from within
an interrupt, which would deadlock if rwlock_t was fair and a writer
was queued by the time the interrupt is processed.
We need to fix those locations (or better yet, remove all rwlocks ;-)
tasklist_lock is the main remaining user. I'm not sure about removing
rwlock_t, but I would like to at least make it fair somehow :)
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
From: Srivatsa S. Bhat <hidden> Date: 2013-01-24 04:32:19
On 01/24/2013 01:27 AM, Tejun Heo wrote:
Hello, Srivatsa.
On Thu, Jan 24, 2013 at 01:03:52AM +0530, Srivatsa S. Bhat wrote:
quoted
Hmm.. I split it up into steps to help explain the reasoning behind
the code sufficiently, rather than spring all of the intricacies at
one go (which would make it very hard to write the changelog/comments
also). The split made it easier for me to document it well in the
changelog, because I could deal with reasonable chunks of code/complexity
at a time. IMHO that helps people reading it for the first time to
understand the logic easily.
I don't know. It's a judgement call I guess. I personally would much
prefer having ample documentation as comments in the source itself or
as a separate Documentation/ file as that's what most people are gonna
be looking at to figure out what's going on. Maybe just compact it a
bit and add more in-line documentation instead?
OK, I'll think about this.
quoted
quoted
The only two options are either punishing writers or identifying and
updating all such possible deadlocks. percpu_rwsem does the former,
right? I don't know how feasible the latter would be.
I don't think we can avoid looking into all the possible deadlocks,
as long as we use rwlocks inside get/put_online_cpus_atomic() (assuming
rwlocks are fair). Even with Oleg's idea of using synchronize_sched()
at the writer, we still need to take care of locking rules, because the
synchronize_sched() only helps avoid the memory barriers at the reader,
and doesn't help get rid of the rwlocks themselves.
Well, percpu_rwlock don't have to use rwlock for the slow path. It
can implement its own writer starving locking scheme. It's not like
implementing slow path global rwlock logic is difficult.
Great idea! So probably I could use atomic ops or something similar in the
slow path to implement the scheme we need...
quoted
CPU 0 CPU 1
read_lock(&rwlock)
write_lock(&rwlock) //spins, because CPU 0
//has acquired the lock for read
read_lock(&rwlock)
^^^^^
What happens here? Does CPU 0 start spinning (and hence deadlock) or will
it continue realizing that it already holds the rwlock for read?
I don't think rwlock allows nesting write lock inside read lock.
read_lock(); write_lock() will always deadlock.
Sure, I understand that :-) My question was, what happens when *two* CPUs
are involved, as in, the read_lock() is invoked only on CPU 0 whereas the
write_lock() is invoked on CPU 1.
For example, the same scenario shown above, but with slightly different
timing, will NOT result in a deadlock:
Scenario 2:
CPU 0 CPU 1
read_lock(&rwlock)
read_lock(&rwlock) //doesn't spin
write_lock(&rwlock) //spins, because CPU 0
//has acquired the lock for read
So I was wondering whether the "fairness" logic of rwlocks would cause
the second read_lock() to spin (in the first scenario shown above) because
a writer is already waiting (and hence new readers should spin) and thus
cause a deadlock.
Regards,
Srivatsa S. Bhat
On Tue, Jan 22, 2013 at 11:32 AM, Steven Rostedt [off-list ref] wrote:
quoted
I thought global locks are now fair. That is, a reader will block if a
writer is waiting. Hence, the above should deadlock on the current
rwlock_t types.
I believe you are mistaken here. struct rw_semaphore is fair (and
blocking), but rwlock_t is unfair. The reason we can't easily make
rwlock_t fair is because tasklist_lock currently depends on the
rwlock_t unfairness - tasklist_lock readers typically don't disable
local interrupts, and tasklist_lock may be acquired again from within
an interrupt, which would deadlock if rwlock_t was fair and a writer
was queued by the time the interrupt is processed.
Yes.
And, iirc, it was even documented somewhere that while rwlock_t is not
really nice, it is good to share the locking with interrupts. You do
not need to disable irqs.
Oleg.
On Tue, Jan 22, 2013 at 3:41 PM, Srivatsa S. Bhat
[off-list ref] wrote:
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() or local_irq_disable() to prevent CPUs from
going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Cc: Mike Frysinger <redacted>
Cc: Bob Liu <redacted>
Cc: Steven Miao <redacted>
Cc: uclinux-dist-devel@blackfin.uclinux.org
Signed-off-by: Srivatsa S. Bhat <redacted>
Hello, Bob.
On Mon, Jan 28, 2013 at 1:09 AM, Bob Liu [off-list ref] wrote:
Thanks, will be applied to my blackfin arch tree.
I think we still have some work ahead of us to have this patchset
ready for inclusion and even then it probably would be best to route
these patches together, so probably not a very good idea to apply this
to blackfin right now.
Thanks.
--
tejun
From: Srivatsa S. Bhat <hidden> Date: 2013-01-29 01:16:15
On 01/29/2013 06:06 AM, Tejun Heo wrote:
Hello, Bob.
On Mon, Jan 28, 2013 at 1:09 AM, Bob Liu [off-list ref] wrote:
quoted
Thanks, will be applied to my blackfin arch tree.
I think we still have some work ahead of us to have this patchset
ready for inclusion and even then it probably would be best to route
these patches together, so probably not a very good idea to apply this
to blackfin right now.
Thanks Tejun for pointing that out! I'll address the review comments
soon and respin the patchset.
Regards,
Srivatsa S. Bhat
From: Srivatsa S. Bhat <hidden> Date: 2013-02-04 13:49:50
On 01/22/2013 01:03 PM, Srivatsa S. Bhat wrote:
Hi,
This patchset removes CPU hotplug's dependence on stop_machine() from the CPU
offline path and provides an alternative (set of APIs) to preempt_disable() to
prevent CPUs from going offline, which can be invoked from atomic context.
The motivation behind the removal of stop_machine() is to avoid its ill-effects
and thus improve the design of CPU hotplug. (More description regarding this
is available in the patches).
All the users of preempt_disable()/local_irq_disable() who used to use it to
prevent CPU offline, have been converted to the new primitives introduced in the
patchset. Also, the CPU_DYING notifiers have been audited to check whether
they can cope up with the removal of stop_machine() or whether they need to
use new locks for synchronization (all CPU_DYING notifiers looked OK, without
the need for any new locks).
Applies on v3.8-rc4. It currently has some locking issues with cpu idle (on
which even lockdep didn't provide any insight unfortunately). So for now, it
works with CONFIG_CPU_IDLE=n.
I ran this patchset on a POWER 7 machine with 32 cores (128 logical CPUs)
[POWER doesn't have the cpu idle issue]. And the results (latency or the time
taken for a single CPU offline) are shown below.
Experiment:
----------
Run a heavy workload (genload from LTP) that generates significant system time;
With '# online CPUs' online, measure the time it takes to complete the stop-m/c
phase in mainline and the equivalent phase in the patched kernel for 1 CPU
offline operation. (It is important to note here that the measurement shows the
average time it takes to perform a *single* CPU offline operation).
Expected results:
----------------
Since stop-machine doesn't scale with no. of online CPUs, we expect the
mainline kernel to take longer and longer for taking 1 CPU offline, with
increasing no. of online CPUs. The patched kernel is expected to take a
constant amount of time, irrespective of the number of online CPUs, because it
has a scalable design.
Experimental results:
---------------------
Avg. latency of 1 CPU offline (ms) [stop-cpu/stop-m/c latency]
# online CPUs Mainline (with stop-m/c) This patchset (no stop-m/c)
8 17.04 7.73
16 18.05 6.44
32 17.31 7.39
64 32.40 9.28
128 98.23 7.35
Analysis and conclusion:
------------------------
The patched kernel performs pretty well and meets our expectations. It beats
mainline easily. As shown in the table above and the graph attached with this
mail, it has the following advantages:
1. Avg. latency is less than mainline (roughly half that of even the least
in mainline).
2. The avg. latency is a constant, irrespective of number of online CPUs in
the system, which proves that the design/synchronization scheme is scalable.
3. Throughout the duration shown above, mainline disables interrupts on all
CPUs. But the patched kernel not only has a smaller duration of hotplug,
but also keeps interrupts enabled on other CPUs, which makes CPU offline
less disruptive on latency-sensitive workloads running on the system.
So, this gives us an idea of how this patchset actually performs. Of course
there are bugs and issues that still need fixing (even mainline crashes with
hotplug sometimes), but I did the above experiment to verify whether the
design is working as expected and whether it really shows significant
improvements over mainline. And thankfully, it does :-)
Regards,
Srivatsa S. Bhat
From: Srivatsa S. Bhat <hidden> Date: 2013-02-08 16:47:01
On 02/08/2013 09:11 PM, Russell King - ARM Linux wrote:
On Thu, Feb 07, 2013 at 11:41:34AM +0530, Srivatsa S. Bhat wrote:
quoted
On 02/07/2013 09:44 AM, Rusty Russell wrote:
quoted
"Srivatsa S. Bhat" [off-list ref] writes:
quoted
On 01/22/2013 01:03 PM, Srivatsa S. Bhat wrote:
Avg. latency of 1 CPU offline (ms) [stop-cpu/stop-m/c latency]
# online CPUs Mainline (with stop-m/c) This patchset (no stop-m/c)
8 17.04 7.73
16 18.05 6.44
32 17.31 7.39
64 32.40 9.28
128 98.23 7.35
Nice!
Thank you :-)
quoted
I wonder how the ARM guys feel with their quad-cpu systems...
That would be definitely interesting to know :-)
That depends what exactly you'd like tested (and how) and whether you'd
like it to be a test-chip based quad core, or an OMAP dual-core SoC.
The effect of stop_machine() doesn't really depend on the CPU architecture
used underneath or the platform. It depends only on the _number_ of
_logical_ CPUs used.
And stop_machine() has 2 noticeable drawbacks:
1. It makes the hotplug operation itself slow
2. and it causes disruptions to the workloads running on the other
CPUs by hijacking the entire machine for significant amounts of time.
In my experiments (mentioned above), I tried to measure how my patchset
improves (reduces) the duration of hotplug (CPU offline) itself. Which is
also slightly indicative of the impact it has on the rest of the system.
But what would be nice to test, is a setup where the workloads running on
the rest of the system are latency-sensitive, and measure the impact of
CPU offline on them, with this patchset applied. That would tell us how
far is this useful in making CPU hotplug less disruptive on the system.
Of course, it would be nice to also see whether we observe any reduction
in hotplug duration itself (point 1 above) on ARM platforms with lot
of CPUs. [This could potentially speed up suspend/resume, which is used
rather heavily on ARM platforms].
The benefits from this patchset over mainline (both in terms of points
1 and 2 above) is expected to increase, with increasing number of CPUs in
the system.
Regards,
Srivatsa S. Bhat
From: Srivatsa S. Bhat <hidden> Date: 2013-02-08 18:12:08
On 02/08/2013 10:14 PM, Srivatsa S. Bhat wrote:
On 02/08/2013 09:11 PM, Russell King - ARM Linux wrote:
quoted
On Thu, Feb 07, 2013 at 11:41:34AM +0530, Srivatsa S. Bhat wrote:
quoted
On 02/07/2013 09:44 AM, Rusty Russell wrote:
quoted
"Srivatsa S. Bhat" [off-list ref] writes:
quoted
On 01/22/2013 01:03 PM, Srivatsa S. Bhat wrote:
Avg. latency of 1 CPU offline (ms) [stop-cpu/stop-m/c latency]
# online CPUs Mainline (with stop-m/c) This patchset (no stop-m/c)
8 17.04 7.73
16 18.05 6.44
32 17.31 7.39
64 32.40 9.28
128 98.23 7.35
Nice!
Thank you :-)
quoted
I wonder how the ARM guys feel with their quad-cpu systems...
That would be definitely interesting to know :-)
That depends what exactly you'd like tested (and how) and whether you'd
like it to be a test-chip based quad core, or an OMAP dual-core SoC.
The effect of stop_machine() doesn't really depend on the CPU architecture
used underneath or the platform. It depends only on the _number_ of
_logical_ CPUs used.
And stop_machine() has 2 noticeable drawbacks:
1. It makes the hotplug operation itself slow
2. and it causes disruptions to the workloads running on the other
CPUs by hijacking the entire machine for significant amounts of time.
In my experiments (mentioned above), I tried to measure how my patchset
improves (reduces) the duration of hotplug (CPU offline) itself. Which is
also slightly indicative of the impact it has on the rest of the system.
But what would be nice to test, is a setup where the workloads running on
the rest of the system are latency-sensitive, and measure the impact of
CPU offline on them, with this patchset applied. That would tell us how
far is this useful in making CPU hotplug less disruptive on the system.
Of course, it would be nice to also see whether we observe any reduction
in hotplug duration itself (point 1 above) on ARM platforms with lot
of CPUs. [This could potentially speed up suspend/resume, which is used
rather heavily on ARM platforms].
The benefits from this patchset over mainline (both in terms of points
1 and 2 above) is expected to increase, with increasing number of CPUs in
the system.
Adding Vincent to CC, who had previously evaluated the performance and
latency implications of CPU hotplug on ARM platforms, IIRC.
Regards,
Srivatsa S. Bhat
From: Paul E. McKenney <hidden> Date: 2013-02-08 23:10:36
On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
lock-ordering related problems (unlike per-cpu locks). However, global
rwlocks lead to unnecessary cache-line bouncing even when there are no
writers present, which can slow down the system needlessly.
Per-cpu counters can help solve the cache-line bouncing problem. So we
actually use the best of both: per-cpu counters (no-waiting) at the reader
side in the fast-path, and global rwlocks in the slowpath.
[ Fastpath = no writer is active; Slowpath = a writer is active ]
IOW, the readers just increment/decrement their per-cpu refcounts (disabling
interrupts during the updates, if necessary) when no writer is active.
When a writer becomes active, he signals all readers to switch to global
rwlocks for the duration of his activity. The readers switch over when it
is safe for them (ie., when they are about to start a fresh, non-nested
read-side critical section) and start using (holding) the global rwlock for
read in their subsequent critical sections.
The writer waits for every existing reader to switch, and then acquires the
global rwlock for write and enters his critical section. Later, the writer
signals all readers that he is done, and that they can go back to using their
per-cpu refcounts again.
Note that the lock-safety (despite the per-cpu scheme) comes from the fact
that the readers can *choose* _when_ to switch to rwlocks upon the writer's
signal. And the readers don't wait on anybody based on the per-cpu counters.
The only true synchronization that involves waiting at the reader-side in this
scheme, is the one arising from the global rwlock, which is safe from circular
locking dependency issues.
Reader-writer locks and per-cpu counters are recursive, so they can be
used in a nested fashion in the reader-path, which makes per-CPU rwlocks also
recursive. Also, this design of switching the synchronization scheme ensures
that you can safely nest and use these locks in a very flexible manner.
I'm indebted to Michael Wang and Xiao Guangrong for their numerous thoughtful
suggestions and ideas, which inspired and influenced many of the decisions in
this as well as previous designs. Thanks a lot Michael and Xiao!
Looks pretty close! Some comments interspersed below. Please either
fix the code or my confusion, as the case may be. ;-)
Thanx, Paul
@@ -64,21 +64,145 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)voidpercpu_read_lock(structpercpu_rwlock*pcpu_rwlock){-read_lock(&pcpu_rwlock->global_rwlock);+preempt_disable();++/* First and foremost, let the writer know that a reader is active */+this_cpu_inc(*pcpu_rwlock->reader_refcnt);++/*+*Ifwearealreadyusingper-cpurefcounts,itisnotsafetoswitch+*thesynchronizationscheme.Socontinueusingtherefcounts.+*/+if(reader_nested_percpu(pcpu_rwlock)){+gotoout;+}else{+/*+*Thewriteto'reader_refcnt'mustbevisiblebeforewe+*read'writer_signal'.+*/+smp_mb();/* Paired with smp_rmb() in sync_reader() */++if(likely(!writer_active(pcpu_rwlock))){+gotoout;+}else{+/* Writer is active, so switch to global rwlock. */+read_lock(&pcpu_rwlock->global_rwlock);++/*+*Wemighthaveracedwithawritergoinginactive+*beforewetooktheread-lock.Sore-evaluatewhether+*westillneedtoholdtherwlockorifwecanswitch+*backtoper-cpurefcounts.(Thisalsohelpsavoid+*heterogeneousnestingofreaders).+*/+if(writer_active(pcpu_rwlock))
The above writer_active() can be reordered with the following this_cpu_dec(),
strange though it might seem. But this is OK because holding the rwlock
is conservative. But might be worth a comment.
We need an smp_mb() here to keep the critical section ordered before the
this_cpu_dec() below. Otherwise, if a writer shows up just after we
exit the fastpath, that writer is not guaranteed to see the effects of
our critical section. Equivalently, the prior read-side critical section
just might see some of the writer's updates, which could be a bit of
a surprise to the reader.
+ /*
+ * We never allow heterogeneous nesting of readers. So it is trivial
+ * to find out the kind of reader we are, and undo the operation
+ * done by our corresponding percpu_read_lock().
+ */
+ if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
+ this_cpu_dec(*pcpu_rwlock->reader_refcnt);
+ smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
Given an smp_mb() above, I don't understand the need for this smp_wmb().
Isn't the idea that if the writer sees ->reader_refcnt decremented to
zero, it also needs to see the effects of the corresponding reader's
critical section?
Or am I missing something subtle here? In any case, if this smp_wmb()
really is needed, there should be some subsequent write that the writer
might observe. From what I can see, there is no subsequent write from
this reader that the writer cares about.
Why do we drop ourselves twice? More to the point, why is it important to
drop ourselves first?
+
+ for_each_online_cpu(cpu)
+ drop_writer_signal(pcpu_rwlock, cpu);
+
+ smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
+}
+
+/*
+ * Wait for the reader to see the writer's signal and switch from percpu
+ * refcounts to global rwlock.
+ *
+ * If the reader is still using percpu refcounts, wait for him to switch.
+ * Else, we can safely go ahead, because either the reader has already
+ * switched over, or the next reader that comes along on that CPU will
+ * notice the writer's signal and will switch over to the rwlock.
+ */
+static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
+ unsigned int cpu)
+{
+ smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
As I understand it, the purpose of this memory barrier is to ensure
that the stores in drop_writer_signal() happen before the reads from
->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
race between a new reader attempting to use the fastpath and this writer
acquiring the lock. Unless I am confused, this must be smp_mb() rather
than smp_rmb().
Also, why not just have a single smp_mb() at the beginning of
sync_all_readers() instead of executing one barrier per CPU?
+
+ while (reader_uses_percpu_refcnt(pcpu_rwlock, cpu))
+ cpu_relax();
+}
+
+static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock)
+{
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu)
+ sync_reader(pcpu_rwlock, cpu);
}
void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
{
+ /*
+ * Tell all readers that a writer is becoming active, so that they
+ * start switching over to the global rwlock.
+ */
+ announce_writer_active(pcpu_rwlock);
+ sync_all_readers(pcpu_rwlock);
write_lock(&pcpu_rwlock->global_rwlock);
}
void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
{
+ /*
+ * Inform all readers that we are done, so that they can switch back
+ * to their per-cpu refcounts. (We don't need to wait for them to
+ * see it).
+ */
+ announce_writer_inactive(pcpu_rwlock);
write_unlock(&pcpu_rwlock->global_rwlock);
}
From: Paul E. McKenney <hidden> Date: 2013-02-08 23:44:17
On Tue, Jan 22, 2013 at 01:04:11PM +0530, Srivatsa S. Bhat wrote:
If interrupt handlers can also be readers, then one of the ways to make
per-CPU rwlocks safe, is to disable interrupts at the reader side before
trying to acquire the per-CPU rwlock and keep it disabled throughout the
duration of the read-side critical section.
The goal is to avoid cases such as:
1. writer is active and it holds the global rwlock for write
2. a regular reader comes in and marks itself as present (by incrementing
its per-CPU refcount) before checking whether writer is active.
3. an interrupt hits the reader;
[If it had not hit, the reader would have noticed that the writer is
active and would have decremented its refcount and would have tried
to acquire the global rwlock for read].
Since the interrupt handler also happens to be a reader, it notices
the non-zero refcount (which was due to the reader who got interrupted)
and thinks that this is a nested read-side critical section and
proceeds to take the fastpath, which is wrong. The interrupt handler
should have noticed that the writer is active and taken the rwlock
for read.
So, disabling interrupts can help avoid this problem (at the cost of keeping
the interrupts disabled for quite long).
But Oleg had a brilliant idea by which we can do much better than that:
we can manage with disabling interrupts _just_ during the updates (writes to
per-CPU refcounts) to safe-guard against races with interrupt handlers.
Beyond that, we can keep the interrupts enabled and still be safe w.r.t
interrupt handlers that can act as readers.
Basically the idea is that we differentiate between the *part* of the
per-CPU refcount that we use for reference counting vs the part that we use
merely to make the writer wait for us to switch over to the right
synchronization scheme.
The scheme involves splitting the per-CPU refcounts into 2 parts:
eg: the lower 16 bits are used to track the nesting depth of the reader
(a "nested-counter"), and the remaining (upper) bits are used to merely mark
the presence of the reader.
As long as the overall reader_refcnt is non-zero, the writer waits for the
reader (assuming that the reader is still actively using per-CPU refcounts for
synchronization).
The reader first sets one of the higher bits to mark its presence, and then
uses the lower 16 bits to manage the nesting depth. So, an interrupt handler
coming in as illustrated above will be able to distinguish between "this is
a nested read-side critical section" vs "we have merely marked our presence
to make the writer wait for us to switch" by looking at the same refcount.
Thus, it makes it unnecessary to keep interrupts disabled throughout the
read-side critical section, despite having the possibility of interrupt
handlers being readers themselves.
Implement this logic and rename the locking functions appropriately, to
reflect what they do.
One nit below. The issues called out in the previous patch still seem
to me to apply.
Thanx, Paul
@@ -62,19 +62,19 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)pcpu_rwlock->writer_signal=NULL;}-voidpercpu_read_lock(structpercpu_rwlock*pcpu_rwlock)+voidpercpu_read_lock_irqsafe(structpercpu_rwlock*pcpu_rwlock){preempt_disable();/* First and foremost, let the writer know that a reader is active */-this_cpu_inc(*pcpu_rwlock->reader_refcnt);+this_cpu_add(*pcpu_rwlock->reader_refcnt,READER_PRESENT);/**Ifwearealreadyusingper-cpurefcounts,itisnotsafetoswitch*thesynchronizationscheme.Socontinueusingtherefcounts.*/if(reader_nested_percpu(pcpu_rwlock)){-gotoout;+this_cpu_inc(*pcpu_rwlock->reader_refcnt);
Hmmm... If the reader is nested, it -doesn't- need the memory barrier at
the end of this function. If there is lots of nesting, it might be
worth getting rid of it.
quoted hunk
} else {
/*
* The write to 'reader_refcnt' must be visible before we
@@ -83,9 +83,19 @@ void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock) smp_mb(); /* Paired with smp_rmb() in sync_reader() */ if (likely(!writer_active(pcpu_rwlock))) {- goto out;+ this_cpu_inc(*pcpu_rwlock->reader_refcnt); } else { /* Writer is active, so switch to global rwlock. */++ /*+ * While we are spinning on ->global_rwlock, an+ * interrupt can hit us, and the interrupt handler+ * might call this function. The distinction between+ * READER_PRESENT and the refcnt helps ensure that the+ * interrupt handler also takes this branch and spins+ * on the ->global_rwlock, as long as the writer is+ * active.+ */ read_lock(&pcpu_rwlock->global_rwlock); /*
@@ -95,26 +105,27 @@ void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock) * back to per-cpu refcounts. (This also helps avoid * heterogeneous nesting of readers). */- if (writer_active(pcpu_rwlock))- this_cpu_dec(*pcpu_rwlock->reader_refcnt);- else+ if (!writer_active(pcpu_rwlock)) {+ this_cpu_inc(*pcpu_rwlock->reader_refcnt); read_unlock(&pcpu_rwlock->global_rwlock);+ } } }-out:+ this_cpu_sub(*pcpu_rwlock->reader_refcnt, READER_PRESENT);+ /* Prevent reordering of any subsequent reads */ smp_rmb(); }-void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)+void percpu_read_unlock_irqsafe(struct percpu_rwlock *pcpu_rwlock) { /* * We never allow heterogeneous nesting of readers. So it is trivial * to find out the kind of reader we are, and undo the operation * done by our corresponding percpu_read_lock(). */- if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {+ if (reader_nested_percpu(pcpu_rwlock)) { this_cpu_dec(*pcpu_rwlock->reader_refcnt); smp_wmb(); /* Paired with smp_rmb() in sync_reader() */ } else {
@@ -184,7 +195,8 @@ static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock) sync_reader(pcpu_rwlock, cpu); }-void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)+void percpu_write_lock_irqsave(struct percpu_rwlock *pcpu_rwlock,+ unsigned long *flags) { /* * Tell all readers that a writer is becoming active, so that they
@@ -192,10 +204,11 @@ void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock) */ announce_writer_active(pcpu_rwlock); sync_all_readers(pcpu_rwlock);- write_lock(&pcpu_rwlock->global_rwlock);+ write_lock_irqsave(&pcpu_rwlock->global_rwlock, *flags); }-void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)+void percpu_write_unlock_irqrestore(struct percpu_rwlock *pcpu_rwlock,+ unsigned long *flags) { /* * Inform all readers that we are done, so that they can switch back
From: Paul E. McKenney <hidden> Date: 2013-02-08 23:48:23
On Tue, Jan 22, 2013 at 01:04:23PM +0530, Srivatsa S. Bhat wrote:
CPU hotplug (which will be the first user of per-CPU rwlocks) has a special
requirement with respect to locking: the writer, after acquiring the per-CPU
rwlock for write, must be allowed to take the same lock for read, without
deadlocking and without getting complaints from lockdep. In comparison, this
is similar to what get_online_cpus()/put_online_cpus() does today: it allows
a hotplug writer (who holds the cpu_hotplug.lock mutex) to invoke it without
locking issues, because it silently returns if the caller is the hotplug
writer itself.
This can be easily achieved with per-CPU rwlocks as well (even without a
"is this a writer?" check) by incrementing the per-CPU refcount of the writer
immediately after taking the global rwlock for write, and then decrementing
the per-CPU refcount before releasing the global rwlock.
This ensures that any reader that comes along on that CPU while the writer is
active (on that same CPU), notices the non-zero value of the nested counter
and assumes that it is a nested read-side critical section and proceeds by
just incrementing the refcount. Thus we prevent the reader from taking the
global rwlock for read, which prevents the writer from deadlocking itself.
Add that support and teach lockdep about this special locking scheme so
that it knows that this sort of usage is valid. Also add the required lockdep
annotations to enable it to detect common locking problems with per-CPU
rwlocks.
Very nice! The write-side interrupt disabling ensures that the task
stays on CPU, as required.
One request: Could we please have a comment explaining the reasons for
the writer incrementing and decrementing the reader reference count?
It looked really really strange to me until I came back and read the
commit log. ;-)
Thanx, Paul
quoted hunk
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
---
lib/percpu-rwlock.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
@@ -84,6 +84,10 @@ void percpu_read_lock_irqsafe(struct percpu_rwlock *pcpu_rwlock)if(likely(!writer_active(pcpu_rwlock))){this_cpu_inc(*pcpu_rwlock->reader_refcnt);++/* Pretend that we take global_rwlock for lockdep */+rwlock_acquire_read(&pcpu_rwlock->global_rwlock.dep_map,+0,0,_RET_IP_);}else{/* Writer is active, so switch to global rwlock. */
From: Paul E. McKenney <hidden> Date: 2013-02-08 23:50:56
On Tue, Jan 22, 2013 at 01:04:54PM +0530, Srivatsa S. Bhat wrote:
There are places where preempt_disable() or local_irq_disable() are used
to prevent any CPU from going offline during the critical section. Let us
call them as "atomic hotplug readers" ("atomic" because they run in atomic,
non-preemptible contexts).
Today, preempt_disable() or its equivalent works because the hotplug writer
uses stop_machine() to take CPUs offline. But once stop_machine() is gone
from the CPU hotplug offline path, the readers won't be able to prevent
CPUs from going offline using preempt_disable().
So the intent here is to provide synchronization APIs for such atomic hotplug
readers, to prevent (any) CPUs from going offline, without depending on
stop_machine() at the writer-side. The new APIs will look something like
this: get_online_cpus_atomic() and put_online_cpus_atomic()
Some important design requirements and considerations:
-----------------------------------------------------
1. Scalable synchronization at the reader-side, especially in the fast-path
Any synchronization at the atomic hotplug readers side must be highly
scalable - avoid global single-holder locks/counters etc. Because, these
paths currently use the extremely fast preempt_disable(); our replacement
to preempt_disable() should not become ridiculously costly and also should
not serialize the readers among themselves needlessly.
At a minimum, the new APIs must be extremely fast at the reader side
atleast in the fast-path, when no CPU offline writers are active.
2. preempt_disable() was recursive. The replacement should also be recursive.
3. No (new) lock-ordering restrictions
preempt_disable() was super-flexible. It didn't impose any ordering
restrictions or rules for nesting. Our replacement should also be equally
flexible and usable.
4. No deadlock possibilities
Regular per-cpu locking is not the way to go if we want to have relaxed
rules for lock-ordering. Because, we can end up in circular-locking
dependencies as explained in https://lkml.org/lkml/2012/12/6/290
So, avoid the usual per-cpu locking schemes (per-cpu locks/per-cpu atomic
counters with spin-on-contention etc) as much as possible, to avoid
numerous deadlock possibilities from creeping in.
Implementation of the design:
----------------------------
We use per-CPU reader-writer locks for synchronization because:
a. They are quite fast and scalable in the fast-path (when no writers are
active), since they use fast per-cpu counters in those paths.
b. They are recursive at the reader side.
c. They provide a good amount of safety against deadlocks; they don't
spring new deadlock possibilities on us from out of nowhere. As a
result, they have relaxed locking rules and are quite flexible, and
thus are best suited for replacing usages of preempt_disable() or
local_irq_disable() at the reader side.
Together, these satisfy all the requirements mentioned above.
I'm indebted to Michael Wang and Xiao Guangrong for their numerous thoughtful
suggestions and ideas, which inspired and influenced many of the decisions in
this as well as previous designs. Thanks a lot Michael and Xiao!
Cc: Russell King <redacted>
Cc: Mike Frysinger <redacted>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Ralf Baechle <redacted>
Cc: David Howells <dhowells@redhat.com>
Cc: "James E.J. Bottomley" <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Martin Schwidefsky <redacted>
Cc: Paul Mundt <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: uclinux-dist-devel@blackfin.uclinux.org
Cc: linux-ia64@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linux-am33-list@redhat.com
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: sparclinux@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
With the change suggested by Namhyung:
Reviewed-by: Paul E. McKenney <redacted>
@@ -198,6 +200,8 @@ static inline void cpu_hotplug_driver_unlock(void)#define get_online_cpus() do { } while (0)#define put_online_cpus() do { } while (0)+#define get_online_cpus_atomic() do { } while (0)+#define put_online_cpus_atomic() do { } while (0)#define hotcpu_notifier(fn, pri) do { (void)(fn); } while (0)/* These aren't inline functions due to a GCC bug. */#define register_hotcpu_notifier(nb) ({ (void)(nb); 0; })
@@ -246,15 +291,21 @@ struct take_cpu_down_param {staticint__reftake_cpu_down(void*_param){structtake_cpu_down_param*param=_param;-interr;+unsignedlongflags;+interr=0;++percpu_write_lock_irqsave(&hotplug_pcpu_rwlock,&flags);/* Ensure this CPU doesn't handle any more interrupts. */err=__cpu_disable();if(err<0)-returnerr;+gotoout;cpu_notify(CPU_DYING|param->mod,param->hcpu);-return0;++out:+percpu_write_unlock_irqrestore(&hotplug_pcpu_rwlock,&flags);+returnerr;}/* Requires cpu_add_remove_lock to be held */
From: Paul E. McKenney <hidden> Date: 2013-02-08 23:51:36
On Tue, Jan 22, 2013 at 01:05:02PM +0530, Srivatsa S. Bhat wrote:
On 12/05/2012 06:10 AM, Andrew Morton wrote:
"static inline C functions would be preferred if possible. Feel free to
fix up the wrong crufty surrounding code as well ;-)"
Convert the macros in the CPU hotplug code to static inline C functions.
Signed-off-by: Srivatsa S. Bhat <redacted>
@@ -198,10 +198,10 @@ static inline void cpu_hotplug_driver_unlock(void)#else /* CONFIG_HOTPLUG_CPU */-#define get_online_cpus() do { } while (0)-#define put_online_cpus() do { } while (0)-#define get_online_cpus_atomic() do { } while (0)-#define put_online_cpus_atomic() do { } while (0)+staticinlinevoidget_online_cpus(void){}+staticinlinevoidput_online_cpus(void){}+staticinlinevoidget_online_cpus_atomic(void){}+staticinlinevoidput_online_cpus_atomic(void){}#define hotcpu_notifier(fn, pri) do { (void)(fn); } while (0)/* These aren't inline functions due to a GCC bug. */#define register_hotcpu_notifier(nb) ({ (void)(nb); 0; })
From: Paul E. McKenney <hidden> Date: 2013-02-09 00:07:53
On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
Once stop_machine() is gone from the CPU offline path, we won't be able to
depend on preempt_disable() to prevent CPUs from going offline from under us.
Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
while invoking from atomic context.
Signed-off-by: Srivatsa S. Bhat <redacted>
Would it make sense for get_online_cpus_atomic() to return the current
CPU number? Looks good otherwise.
Thanx, Paul
@@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,conststructcpumask*nodemask;intret;+get_online_cpus_atomic();/* Try for same CPU (cheapest) */-cpu=get_cpu();+cpu=smp_processor_id();+if(cpumask_test_cpu(cpu,mask))gotocall;
@@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,cpu=cpumask_any_and(mask,cpu_online_mask);call:ret=smp_call_function_single(cpu,func,info,wait);-put_cpu();+put_online_cpus_atomic();returnret;}EXPORT_SYMBOL_GPL(smp_call_function_any);
@@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,/* No online cpus? We're done. */if(cpu>=nr_cpu_ids)-return;+gotoout_unlock;/* Do we have another CPU which isn't us? */next_cpu=cpumask_next_and(cpu,mask,cpu_online_mask);if(next_cpu==this_cpu)-next_cpu=cpumask_next_and(next_cpu,mask,cpu_online_mask);+next_cpu=cpumask_next_and(next_cpu,mask,+cpu_online_mask);/* Fastpath: do that cpu by itself. */if(next_cpu>=nr_cpu_ids){smp_call_function_single(cpu,func,info,wait);-return;+gotoout_unlock;}data=&__get_cpu_var(cfd_data);
@@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,/* Some callers race with other cpus changing the passed mask */if(unlikely(!refs)){csd_unlock(&data->csd);-return;+gotoout_unlock;}raw_spin_lock_irqsave(&call_function.lock,flags);
@@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,/* Optionally wait for the CPUs to complete */if(wait)csd_lock_wait(&data->csd);++out_unlock:+put_online_cpus_atomic();}EXPORT_SYMBOL(smp_call_function_many);
From: Paul E. McKenney <hidden> Date: 2013-02-09 00:18:38
On Tue, Jan 22, 2013 at 01:15:22PM +0530, Srivatsa S. Bhat wrote:
... and also cleanup a comment that refers to CPU hotplug being dependent on
stop_machine().
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Srivatsa S. Bhat <redacted>
Reviewed-by: Paul E. McKenney <redacted>
(Hey, I thought I owed myself an easy one!)
From: Paul E. McKenney <hidden> Date: 2013-02-09 00:28:42
On Tue, Jan 22, 2013 at 01:06:34PM +0530, Srivatsa S. Bhat wrote:
Don't refer to stop_machine() in the CPU hotplug path, since we are going
to get rid of it. Also, move the comment referring to callback adoption
to the CPU_DEAD case, because that's where it happens now.
Signed-off-by: Srivatsa S. Bhat <redacted>
Ouch! That comment is indeed obsolete and must die.
I queued this to -rcu with your Signed-off-by. However, I omitted
the added comment, as it is imcomplete -- it is easy to look at
rcu_cleanup_dead_cpu() to see what it does.
Thanx, Paul
From: Paul E. McKenney <hidden> Date: 2013-02-09 00:40:51
On Tue, Jan 22, 2013 at 01:15:48PM +0530, Srivatsa S. Bhat wrote:
Since stop_machine() is no longer used in the CPU offline path, we cannot
disable CPU hotplug using preempt_disable()/local_irq_disable() etc. We
need to use the newly introduced get/put_online_cpus_atomic() APIs.
Reflect this in the documentation.
Cc: Rob Landley <redacted>
Cc: linux-doc@vger.kernel.org
Signed-off-by: Srivatsa S. Bhat <redacted>
@@ -113,13 +113,15 @@ Never use anything other than cpumask_t to represent bitmap of CPUs. #include <linux/cpu.h> get_online_cpus() and put_online_cpus():-The above calls are used to inhibit cpu hotplug operations. While the+The above calls are used to inhibit cpu hotplug operations, when invoked from+non-atomic context (because the above functions can sleep). While the cpu_hotplug.refcount is non zero, the cpu_online_mask will not change.-If you merely need to avoid cpus going away, you could also use-preempt_disable() and preempt_enable() for those sections.-Just remember the critical section cannot call any-function that can sleep or schedule this process away. The preempt_disable()-will work as long as stop_machine_run() is used to take a cpu down.++However, if you are executing in atomic context (ie., you can't afford to+sleep), and you merely need to avoid cpus going offline, you can use+get_online_cpus_atomic() and put_online_cpus_atomic() for those sections.+Just remember the critical section cannot call any function that can sleep or+schedule this process away. CPU Hotplug - Frequently Asked Questions.
@@ -360,6 +362,9 @@ A: There are two ways. If your code can be run in interrupt context, use return err; }+ If my_func_on_cpu() itself cannot block, use get/put_online_cpus_atomic()+ instead of get/put_online_cpus() to prevent CPUs from going offline.+ Q: How do we determine how many CPUs are available for hotplug. A: There is no clear spec defined way from ACPI that can give us that information today. Based on some input from Natalie of Unisys,