Miscellaneous fixes for 3.20
Subsystems (3)
cover Miscellaneous fixes for 3.20
Hello!
This series contains miscellaneous fixes for 3.20:
1. Make rcu_nmi_enter() handle nesting, as there are now some
architectures that use RCU read-side critical sections in NMI-like constructs that can nest in one another. Algorithm courtesy of Andy Lutomirski.
2. Remove redundant rcu_is_cpu_rrupt_from_idle() from tiny RCU,
courtesy of Alexander Gordeev.
3. Fix invoke_rcu_callbacks() comment to correctly reflect
required calling environment.
4. Allow 1- and 2-byte smp_load_acquire() and smp_store_release(),
given that the C11 standard requires that compilers support this.
5. Replace smp_read_barrier_depends() with lockless_dereference(),
courtesy of Pranith Kumar.
6. Directly force QS when call_rcu_[bh|sched]() on idle_task for
tiny RCU, courtesy of Lai Jiangshan. Saves more than 1KB.
7. Fix sparse warning from hlist_for_each_entry_continue_rcu_bh(),
courtesy of Ying Xue.
8. Fix hang in CPU hotplug code, courtesy of David Hildenbrand.
9. Fix rcu_barrier() race that could result in too-short wait.
10. Remove obsolete "select IRQ_WORK" from config TREE_RCU, courtesy
of Lai Jiangshan.
Thanx, Paul
------------------------------------------------------------------------
b/include/linux/compiler.h | 2 b/include/linux/rculist.h | 16 +++--- b/include/linux/rcupdate.h | 10 ++-- b/init/Kconfig | 2 b/kernel/cpu.c | 56 +++++++++------------- b/kernel/rcu/rcu.h | 6 ++ b/kernel/rcu/tiny.c | 111 +++------------------------------------------ b/kernel/rcu/tiny_plugin.h | 2 b/kernel/rcu/tree.c | 75 ++++++++++++++++++++---------- b/kernel/rcu/tree.h | 30 ++---------- b/kernel/rcu/tree_plugin.h | 45 +++++++++++------- 11 files changed, 138 insertions(+), 217 deletions(-)
PATCH 1/10 rcu: Make rcu_nmi_enter() handle nesting
From: "Paul E. McKenney" <[email protected]>
The x86 architecture has multiple types of NMI-like interrupts: real NMIs, machine checks, and, for some values of NMI-like, debugging and breakpoint interrupts. These interrupts can nest inside each other. Andy Lutomirski is adding RCU support to these interrupts, so rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.
This commit therefore introduces nesting, using a clever NMI-coordination algorithm suggested by Andy. The trick is to atomically increment ->dynticks (if needed) before manipulating ->dynticks_nmi_nesting on entry (and, accordingly, after on exit). In addition, ->dynticks_nmi_nesting is incremented by one if ->dynticks was incremented and by two otherwise. This means that when rcu_nmi_exit() sees ->dynticks_nmi_nesting equal to one, it knows that ->dynticks must be atomically incremented.
This NMI-coordination algorithms has been validated by the following Promela model:
------------------------------------------------------------------------
/*
* Promela model for Andy Lutomirski's suggested change to rcu_nmi_enter()
* that allows nesting.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, you can access it online at
* http://www.gnu.org/licenses/gpl-2.0.html.
*
* Copyright IBM Corporation, 2014
*
* Author: Paul E. McKenney <[email protected]>
*/
byte dynticks_nmi_nesting = 0;
byte dynticks = 0;
/*
* Promela verision of rcu_nmi_enter().
*/
inline rcu_nmi_enter()
{
byte incby;
byte tmp;
incby = BUSY_INCBY;
assert(dynticks_nmi_nesting >= 0);
if
:: (dynticks & 1) == 0 ->
atomic {
dynticks = dynticks + 1;
}
assert((dynticks & 1) == 1);
incby = 1;
:: else ->
skip;
fi;
tmp = dynticks_nmi_nesting;
tmp = tmp + incby;
dynticks_nmi_nesting = tmp;
assert(dynticks_nmi_nesting >= 1);
}/*
* Promela verision of rcu_nmi_exit().
*/
inline rcu_nmi_exit()
{
byte tmp;
assert(dynticks_nmi_nesting > 0);
assert((dynticks & 1) != 0);
if
:: dynticks_nmi_nesting != 1 ->
tmp = dynticks_nmi_nesting;
tmp = tmp - BUSY_INCBY;
dynticks_nmi_nesting = tmp;
:: else ->
dynticks_nmi_nesting = 0;
atomic {
dynticks = dynticks + 1;
}
assert((dynticks & 1) == 0);
fi;
}/*
* Base-level NMI runs non-atomically. Crudely emulates process-level
* dynticks-idle entry/exit.
*/
proctype base_NMI()
{
byte busy;
busy = 0;
do
:: /* Emulate base-level dynticks and not. */
if
:: 1 -> atomic {
dynticks = dynticks + 1;
}
busy = 1;
:: 1 -> skip;
fi;
/* Verify that we only sometimes have base-level dynticks. */
if
:: busy == 0 -> skip;
:: busy == 1 -> skip;
fi;
/* Model RCU's NMI entry and exit actions. */
rcu_nmi_enter();
assert((dynticks & 1) == 1);
rcu_nmi_exit();
/* Emulated re-entering base-level dynticks and not. */
if
:: !busy -> skip;
:: busy ->
atomic {
dynticks = dynticks + 1;
}
busy = 0;
fi;
/* We had better now be in dyntick-idle mode. */
assert((dynticks & 1) == 0);
od;
}/*
* Nested NMI runs atomically to emulate interrupting base_level(). */
proctype nested_NMI()
{
do
:: /*
* Use an atomic section to model a nested NMI. This is
* guaranteed to interleave into base_NMI() between a pair
* of base_NMI() statements, just as a nested NMI would.
*/
atomic {
/* Verify that we only sometimes are in dynticks. */
if
:: (dynticks & 1) == 0 -> skip;
:: (dynticks & 1) == 1 -> skip;
fi;
/* Model RCU's NMI entry and exit actions. */
rcu_nmi_enter();
assert((dynticks & 1) == 1);
rcu_nmi_exit();
}
od;
}init {
run base_NMI();
run nested_NMI();
}------------------------------------------------------------------------
The following script can be used to run this model if placed in rcu_nmi.spin:
------------------------------------------------------------------------
if ! spin -a rcu_nmi.spin
then
echo Spin errors!!!
exit 1
fi
if ! cc -DSAFETY -o pan pan.c
then
echo Compilation errors!!!
exit 1
fi
./pan -m100000
--- kernel/rcu/tree.c | 66 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 17 deletions(-)
kernel/rcu/tree.c +49 -17
@@ -759,39 +759,71 @@ void rcu_irq_enter(void)/** * rcu_nmi_enter - inform RCU of entry to NMI context * -* If the CPU was idle with dynamic ticks active, and there is no -* irq handler running, this updates rdtp->dynticks_nmi to let the -* RCU grace-period handling know that the CPU is active. +* If the CPU was idle from RCU's viewpoint, update rdtp->dynticks and +* rdtp->dynticks_nmi_nesting to let the RCU grace-period handling know +* that the CPU is active. This implementation permits nested NMIs, as +* long as the nesting level does not overflow an int. (You will probably +* run out of stack space first.) */ void rcu_nmi_enter (void ) {struct rcu_dynticks *rdtp =this_cpu_ptr (&rcu_dynticks); +int incby =2 ; -if (rdtp->dynticks_nmi_nesting ==0 && - (atomic_read (&rdtp->dynticks) &0x1 )) -return ; - rdtp->dynticks_nmi_nesting++; -smp_mb__before_atomic ();/* Force delay from prior write. */ -atomic_inc (&rdtp->dynticks); -/* CPUs seeing atomic_inc() must see later RCU read-side crit sects */ -smp_mb__after_atomic ();/* See above. */ -WARN_ON_ONCE (!(atomic_read (&rdtp->dynticks) &0x1 )); +/* Complain about underflow. */ +WARN_ON_ONCE (rdtp->dynticks_nmi_nesting <0 ); + +/* +* If idle from RCU viewpoint, atomically increment ->dynticks +* to mark non-idle and increment ->dynticks_nmi_nesting by one. +* Otherwise, increment ->dynticks_nmi_nesting by two. This means +* if ->dynticks_nmi_nesting is equal to one, we are guaranteed +* to be in the outermost NMI handler that interrupted an RCU-idle +* period (observation due to Andy Lutomirski). +*/ +if (!(atomic_read (&rdtp->dynticks) &0x1 )) { +smp_mb__before_atomic ();/* Force delay from prior write. */ +atomic_inc (&rdtp->dynticks); +/* atomic_inc() before later RCU read-side crit sects */ +smp_mb__after_atomic ();/* See above. */ +WARN_ON_ONCE (!(atomic_read (&rdtp->dynticks) &0x1 )); + incby =1 ; + } + rdtp->dynticks_nmi_nesting += incby; +barrier (); }/** * rcu_nmi_exit - inform RCU of exit from NMI context * -* If the CPU was idle with dynamic ticks active, and there is no -* irq handler running, this updates rdtp->dynticks_nmi to let the -* RCU grace-period handling know that the CPU is no longer active. +* If we are returning from the outermost NMI handler that interrupted an +* RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting +* to let the RCU grace-period handling know that the CPU is back to +* being RCU-idle. */ void rcu_nmi_exit (void ) {struct rcu_dynticks *rdtp =this_cpu_ptr (&rcu_dynticks); -if (rdtp->dynticks_nmi_nesting ==0 || - --rdtp->dynticks_nmi_nesting !=0 ) +/* +* Check for ->dynticks_nmi_nesting underflow and bad ->dynticks. +* (We are exiting an NMI handler, so RCU better be paying attention +* to us!) +*/ +WARN_ON_ONCE (rdtp->dynticks_nmi_nesting <=0 ); +WARN_ON_ONCE (!(atomic_read (&rdtp->dynticks) &0x1 )); + +/* +* If the nesting level is not 1, the CPU wasn't RCU-idle, so +* leave it in non-RCU-idle state. +*/ +if (rdtp->dynticks_nmi_nesting !=1 ) { + rdtp->dynticks_nmi_nesting -=2 ;return ; + } + +/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */ + rdtp->dynticks_nmi_nesting =0 ;/* CPUs seeing atomic_inc() must see prior RCU read-side crit sects */ smp_mb__before_atomic ();/* See above. */ atomic_inc (&rdtp->dynticks);
Signature · 1 line
1.8.1.5
PATCH 2/10 rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() from tiny RCU
From: Alexander Gordeev <[email protected]>
Let's start assuming that something in the idle loop posts a callback, and scheduling-clock interrupt occurs:
1. The system is idle and stays that way, no runnable tasks.
2. Scheduling-clock interrupt occurs, rcu_check_callbacks() is called as result, which in turn calls rcu_is_cpu_rrupt_from_idle().
3. rcu_is_cpu_rrupt_from_idle() reports the CPU was interrupted from idle, which results in rcu_sched_qs() call, which does a raise_softirq(RCU_SOFTIRQ).
4. Upon return from interrupt, rcu_irq_exit() is invoked, which calls rcu_idle_enter_common(), which in turn calls rcu_sched_qs() again, which does another raise_softirq(RCU_SOFTIRQ).
5. The softirq happens shortly and invokes rcu_process_callbacks(), which invokes __rcu_process_callbacks().
6. So now callbacks can be invoked. At least they can be if ->donetail has been updated. Which it will have been because rcu_sched_qs() invokes rcu_qsctr_help().
In the described scenario rcu_sched_qs() and raise_softirq(RCU_SOFTIRQ) get called twice in steps 3 and 4. This redundancy could be eliminated by removing rcu_is_cpu_rrupt_from_idle() function.
--- kernel/rcu/tiny.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)
kernel/rcu/tiny.c +1 -11
@@ -186,16 +186,6 @@ EXPORT_SYMBOL(__rcu_is_watching);#endif /* defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_RCU_TRACE) */ /* -* Test whether the current CPU was interrupted from idle. Nested -* interrupts don't count, we must be running at the first interrupt -* level. -*/ -static int rcu_is_cpu_rrupt_from_idle (void ) -{ -return rcu_dynticks_nesting <=1 ; -} - -/* * Helper function for rcu_sched_qs() and rcu_bh_qs(). * Also irqs are disabled to avoid confusion due to interrupt handlers * invoking call_rcu().
@@ -250,7 +240,7 @@ void rcu_bh_qs(void)void rcu_check_callbacks (int user) {RCU_TRACE (check_cpu_stalls ()); -if (user ||rcu_is_cpu_rrupt_from_idle ()) +if (user)rcu_sched_qs ();else if (!in_softirq ())rcu_bh_qs ();
Signature · 1 line
1.8.1.5
PATCH 3/10 rcu: Fix invoke_rcu_callbacks() comment
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
Despite what the comment says, it is only softirqs that are disabled, not interrupts. This commit therefore fixes the comment.
--- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/rcu/tree.c +1 -1
@@ -2601,7 +2601,7 @@ static void rcu_process_callbacks(struct softirq_action *unused)* Schedule RCU callback invocation. If the specified type of RCU * does not support RCU priority boosting, just do a direct call, * otherwise wake up the per-CPU kernel kthread. Note that because we -* are running on the current CPU with interrupts disabled, the +* are running on the current CPU with softirqs disabled, the * rcu_cpu_kthread_task cannot disappear out from under us. */ static void invoke_rcu_callbacks (struct rcu_state *rsp,struct rcu_data *rdp)
Signature · 1 line
1.8.1.5
PATCH 4/10 compiler: Allow 1- and 2-byte smp_load_acquire() and smp_store_release()
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
CPUs without single-byte and double-byte loads and stores place some "interesting" requirements on concurrent code. For example (adapted from Peter Hurley's test code), suppose we have the following structure:
struct foo {
spinlock_t lock1;
spinlock_t lock2;
char a; /* Protected by lock1. */
char b; /* Protected by lock2. */
};
struct foo *foop;Of course, it is common (and good) practice to place data protected by different locks in separate cache lines. However, if the locks are rarely acquired (for example, only in rare error cases), and there are a great many instances of the data structure, then memory footprint can trump false-sharing concerns, so that it can be better to place them in the same cache cache line as above.
But if the CPU does not support single-byte loads and stores, a store to foop->a will do a non-atomic read-modify-write operation on foop->b, which will come as a nasty surprise to someone holding foop->lock2. So we now require CPUs to support single-byte and double-byte loads and stores. Therefore, this commit adjusts the definition of __native_word() to allow these sizes to be used by smp_load_acquire() and smp_store_release().
--- include/linux/compiler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/compiler.h +1 -1
@@ -385,7 +385,7 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int/* Is this type a native word size -- useful for atomic operations */ #ifndef __native_word -# define __native_word (t) (sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long)) +# define __native_word (t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))#endif /* Compile time object size, -1 for unknown */
Signature · 1 line
1.8.1.5
PATCH 5/10 rcupdate: Replace smp_read_barrier_depends() with lockless_dereference()
In reply to Paul E. McKenneyFrom: Pranith Kumar <[email protected]>
Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
--- include/linux/rcupdate.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
include/linux/rcupdate.h +5 -5
@@ -582,11 +582,11 @@ static inline void rcu_preempt_sleep_check(void) })#define __rcu_dereference_check (p, c, space) \ ({ \ - typeof(*p) *_________p1 = (typeof(*p) *__force)ACCESS_ONCE(p); \ +/* Dependency order vs. p above. */ \ + typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \rcu_lockdep_assert (c,"suspicious rcu_dereference_check() usage" ); \rcu_dereference_sparse (p, space); \ - smp_read_barrier_depends();/* Dependency order vs. p above. */ \ - ((typeof(*p) __force __kernel *)(_________p1)); \ + ((typeof(*p) __force __kernel *)(________p1)); \ })#define __rcu_dereference_protected (p, c, space) \ ({ \
@@ -603,10 +603,10 @@ static inline void rcu_preempt_sleep_check(void) })#define __rcu_dereference_index_check (p, c) \ ({ \ - typeof(p) _________p1 = ACCESS_ONCE(p); \ +/* Dependency order vs. p above. */ \ +typeof (p) _________p1 =lockless_dereference (p); \rcu_lockdep_assert (c, \"suspicious rcu_dereference_index_check() usage" ); \ - smp_read_barrier_depends();/* Dependency order vs. p above. */ \ (_________p1); \ })
Signature · 1 line
1.8.1.5
PATCH 6/10 tiny_rcu: Directly force QS when call_rcu_[bh|sched]() on idle_task
In reply to Paul E. McKenneyFrom: Lai Jiangshan <[email protected]>
For RCU in UP, context-switch = QS = GP, thus we can force a context-switch when any call_rcu_[bh|sched]() is happened on idle_task. After doing so, rcu_idle/irq_enter/exit() are useless, so we can simply make these functions empty.
More important, this change does not change the functionality logically.
Note: raise_softirq(RCU_SOFTIRQ)/rcu_sched_qs() in rcu_idle_enter() and outmost rcu_irq_exit() will have to wake up the ksoftirqd (due to in_interrupt() == 0).
Before this patch After this patch:call_rcu_sched() in idle; call_rcu_sched() in idle
set resched
do other stuffs; do other stuffs
outmost rcu_irq_exit() outmost rcu_irq_exit() (empty function)
(or rcu_idle_enter()) (or rcu_idle_enter(), also empty function)
start to resched. (see above)
rcu_sched_qs() rcu_sched_qs()
QS,and GP and advance cb QS,and GP and advance cb
wake up the ksoftirqd wake up the ksoftirqd
set resched
resched to ksoftirqd (or other) resched to ksoftirqd (or other)
These two code patches are almost the same.
Size changed after patched:
size kernel/rcu/tiny-old.o kernel/rcu/tiny-patched.o
text data bss dec hex filename 3449 206 8 3663 e4f kernel/rcu/tiny-old.o 2406 144 8 2558 9fe kernel/rcu/tiny-patched.o
--- kernel/rcu/rcu.h | 6 +++ kernel/rcu/tiny.c | 99 ++++-------------------------------------------- kernel/rcu/tiny_plugin.h | 2 +- kernel/rcu/tree.c | 6 --- 4 files changed, 14 insertions(+), 99 deletions(-)
kernel/rcu/rcu.h +6 -0
@@ -137,4 +137,10 @@ int rcu_jiffies_till_stall_check(void);void rcu_early_boot_tests (void ); +/* +* This function really isn't for public consumption, but RCU is special in +* that context switches can allow the state machine to make progress. +*/ +extern void resched_cpu (int cpu); +#endif /* __LINUX_RCU_H */
kernel/rcu/tiny.c +7 -92
144 lines not on this page · raw message
kernel/rcu/tiny_plugin.h +1 -1
@@ -147,7 +147,7 @@ static void check_cpu_stall(struct rcu_ctrlblk *rcp) js =ACCESS_ONCE (rcp->jiffies_stall);if (*rcp->curtail &&ULONG_CMP_GE (j, js)) {pr_err ("INFO: %s stall on CPU (%lu ticks this GP) idle=%llx (t=%lu jiffies q=%ld)\n" , - rcp->name, rcp->ticks_this_gp, rcu_dynticks_nesting, + rcp->name, rcp->ticks_this_gp,DYNTICK_TASK_EXIT_IDLE , jiffies - rcp->gp_start, rcp->qlen);dump_stack (); }
kernel/rcu/tree.c +0 -6
@@ -935,12 +935,6 @@ static int dyntick_save_progress_counter(struct rcu_data *rdp, }/* -* This function really isn't for public consumption, but RCU is special in -* that context switches can allow the state machine to make progress. -*/ -extern void resched_cpu (int cpu); - -/* * Return true if the specified CPU has passed through a quiescent * state by virtue of being in or having passed through an dynticks * idle state since the last call to dyntick_save_progress_counter()
Signature · 1 line
1.8.1.5
PATCH 7/10 rculist: Fix sparse warning
In reply to Paul E. McKenneyFrom: Ying Xue <[email protected]>
This fixes the following sparse warnings:
make C=1 CF=-D__CHECK_ENDIAN__ net/ipv6/addrconf.onet/ipv6/addrconf.c:3495:9: error: incompatible types in comparison expression (different address spaces) net/ipv6/addrconf.c:3495:9: error: incompatible types in comparison expression (different address spaces) net/ipv6/addrconf.c:3495:9: error: incompatible types in comparison expression (different address spaces) net/ipv6/addrconf.c:3495:9: error: incompatible types in comparison expression (different address spaces)
To silence these spare complaints, an RCU annotation should be added to "next" pointer of hlist_node structure through hlist_next_rcu() macro when iterating over a hlist with hlist_for_each_entry_continue_rcu_bh().
By the way, this commit also resolves the same error appearing in hlist_for_each_entry_continue_rcu().
--- include/linux/rculist.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
include/linux/rculist.h +8 -8
@@ -524,11 +524,11 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,* @member: the name of the hlist_node within the struct. */ #define hlist_for_each_entry_continue_rcu (pos, member) \ - for (pos = hlist_entry_safe(rcu_dereference((pos)->member.next),\ - typeof(*(pos)), member); \ + for (pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ + &(pos)->member)), typeof(*(pos)), member); \ pos; \ - pos = hlist_entry_safe(rcu_dereference((pos)->member.next),\ - typeof(*(pos)), member)) + pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ + &(pos)->member)), typeof(*(pos)), member))/** * hlist_for_each_entry_continue_rcu_bh - iterate over a hlist continuing after current point
@@ -536,11 +536,11 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,* @member: the name of the hlist_node within the struct. */ #define hlist_for_each_entry_continue_rcu_bh (pos, member) \ - for (pos = hlist_entry_safe(rcu_dereference_bh((pos)->member.next),\ - typeof(*(pos)), member); \ + for (pos = hlist_entry_safe(rcu_dereference_bh(hlist_next_rcu( \ + &(pos)->member)), typeof(*(pos)), member); \ pos; \ - pos = hlist_entry_safe(rcu_dereference_bh((pos)->member.next),\ - typeof(*(pos)), member)) + pos = hlist_entry_safe(rcu_dereference_bh(hlist_next_rcu( \ + &(pos)->member)), typeof(*(pos)), member))/** * hlist_for_each_entry_from_rcu - iterate over a hlist continuing from current point
Signature · 1 line
1.8.1.5
PATCH 8/10 hotplugcpu: Avoid deadlocks by waking active_writer
In reply to Paul E. McKenneyFrom: David Hildenbrand <[email protected]>
Commit b2c4623dcd07 ("rcu: More on deadlock between CPU hotplug and expedited grace periods") introduced another problem that can easily be reproduced by starting/stopping cpus in a loop.
E.g.:
for i in seq 5000; do
echo 1 > /sys/devices/system/cpu/cpu1/online echo 0 > /sys/devices/system/cpu/cpu1/online
done
Will result in:
INFO: task /cpu_start_stop:1 blocked for more than 120 seconds. Call Trace: ([<00000000006a028e>] __schedule+0x406/0x91c) [<0000000000130f60>] cpu_hotplug_begin+0xd0/0xd4 [<0000000000130ff6>] _cpu_up+0x3e/0x1c4 [<0000000000131232>] cpu_up+0xb6/0xd4 [<00000000004a5720>] device_online+0x80/0xc0 [<00000000004a57f0>] online_store+0x90/0xb0 ...
And a deadlock.
Problem is that if the last ref in put_online_cpus() can't get the cpu_hotplug.lock the puts_pending count is incremented, but a sleeping active_writer might never be woken up, therefore never exiting the loop in cpu_hotplug_begin().
This fix removes puts_pending and turns refcount into an atomic variable. We also introduce a wait queue for the active_writer, to avoid possible races and use-after-free. There is no need to take the lock in put_online_cpus() anymore.
Can't reproduce it with this fix.
--- kernel/cpu.c | 56 +++++++++++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 33 deletions(-)
kernel/cpu.c +23 -33
@@ -58,22 +58,23 @@ static int cpu_hotplug_disabled;static struct {struct task_struct *active_writer; -struct mutex lock;/* Synchronizes accesses to refcount, */ +/* wait queue to wake up the active_writer */ +wait_queue_head_t wq; +/* verifies that no writer will get active while readers are active */ +struct mutex lock;/* * Also blocks the new readers during * an ongoing cpu hotplug operation. */ -int refcount; -/* And allows lockless put_online_cpus(). */ -atomic_t puts_pending; +atomic_t refcount;#ifdef CONFIG_DEBUG_LOCK_ALLOC struct lockdep_map dep_map;#endif } cpu_hotplug = { .active_writer =NULL , + .wq =__WAIT_QUEUE_HEAD_INITIALIZER (cpu_hotplug.wq), .lock =__MUTEX_INITIALIZER (cpu_hotplug.lock), - .refcount =0 , #ifdef CONFIG_DEBUG_LOCK_ALLOC .dep_map = {.name = "cpu_hotplug.lock" }, #endif
@@ -86,15 +87,6 @@ static struct {#define cpuhp_lock_acquire () lock_map_acquire(&cpu_hotplug.dep_map)#define cpuhp_lock_release () lock_map_release(&cpu_hotplug.dep_map) -static void apply_puts_pending (int max) -{ -int delta; - -if (atomic_read (&cpu_hotplug.puts_pending) >= max) { - delta =atomic_xchg (&cpu_hotplug.puts_pending,0 ); - cpu_hotplug.refcount -= delta; - } -}void get_online_cpus (void ) {
@@ -103,8 +95,7 @@ void get_online_cpus(void)return ;cpuhp_lock_acquire_read ();mutex_lock (&cpu_hotplug.lock); -apply_puts_pending (65536 ); - cpu_hotplug.refcount++; +atomic_inc (&cpu_hotplug.refcount);mutex_unlock (&cpu_hotplug.lock); }EXPORT_SYMBOL_GPL (get_online_cpus);
@@ -116,8 +107,7 @@ bool try_get_online_cpus(void)if (!mutex_trylock (&cpu_hotplug.lock))return false;cpuhp_lock_acquire_tryread (); -apply_puts_pending (65536 ); - cpu_hotplug.refcount++; +atomic_inc (&cpu_hotplug.refcount);mutex_unlock (&cpu_hotplug.lock);return true; }
@@ -125,20 +115,18 @@ EXPORT_SYMBOL_GPL(try_get_online_cpus);void put_online_cpus (void ) { +int refcount; +if (cpu_hotplug.active_writer == current)return ; -if (!mutex_trylock (&cpu_hotplug.lock)) { -atomic_inc (&cpu_hotplug.puts_pending); -cpuhp_lock_release (); -return ; - } -if (WARN_ON (!cpu_hotplug.refcount)) - cpu_hotplug.refcount++;/* try to fix things up */ + refcount =atomic_dec_return (&cpu_hotplug.refcount); +if (WARN_ON (refcount <0 ))/* try to fix things up */ +atomic_inc (&cpu_hotplug.refcount); + +if (refcount <=0 &&waitqueue_active (&cpu_hotplug.wq)) +wake_up (&cpu_hotplug.wq); -if (!--cpu_hotplug.refcount &&unlikely (cpu_hotplug.active_writer)) -wake_up_process (cpu_hotplug.active_writer); -mutex_unlock (&cpu_hotplug.lock);cpuhp_lock_release (); }
@@ -168,18 +156,20 @@ EXPORT_SYMBOL_GPL(put_online_cpus);*/ void cpu_hotplug_begin (void ) { - cpu_hotplug.active_writer = current; +DEFINE_WAIT (wait); + cpu_hotplug.active_writer = current;cpuhp_lock_acquire (); +for (;;) {mutex_lock (&cpu_hotplug.lock); -apply_puts_pending (1 ); -if (likely (!cpu_hotplug.refcount)) -break ; -__set_current_state (TASK_UNINTERRUPTIBLE ); +prepare_to_wait (&cpu_hotplug.wq, &wait,TASK_UNINTERRUPTIBLE ); +if (likely (!atomic_read (&cpu_hotplug.refcount))) +break ;mutex_unlock (&cpu_hotplug.lock);schedule (); } +finish_wait (&cpu_hotplug.wq, &wait); }void cpu_hotplug_done (void )
Signature · 1 line
1.8.1.5
PATCH 9/10 rcu: Fix rcu_barrier() race that could result in too-short wait
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
The rcu_barrier() no-callbacks check for no-CBs CPUs has race conditions. It checks a given CPU's lists of callbacks, and if all three no-CBs lists are empty, ignores that CPU. However, these three lists could potentially be empty even when callbacks are present if the check executed just as the callbacks were being moved from one list to another. It turns out that recent versions of rcutorture can spot this race.
This commit plugs this hole by consolidating the per-list counts of no-CBs callbacks into a single count, which is incremented before the corresponding callback is posted and after it is invoked. Then rcu_barrier() checks this single count to reliably determine whether the corresponding CPU has no-CBs callbacks.
--- kernel/rcu/tree.c | 1 + kernel/rcu/tree.h | 29 +++++++---------------------- kernel/rcu/tree_plugin.h | 45 ++++++++++++++++++++++++++++----------------- 3 files changed, 36 insertions(+), 39 deletions(-)
kernel/rcu/tree.c +1 -0
@@ -3344,6 +3344,7 @@ static void _rcu_barrier(struct rcu_state *rsp) }else {_rcu_barrier_trace (rsp,"OnlineNoCB" , cpu, rsp->n_barrier_done); +smp_mb__before_atomic ();atomic_inc (&rsp->barrier_cpu_count);__call_rcu (&rdp->barrier_head, rcu_barrier_callback, rsp, cpu,0 );
kernel/rcu/tree.h +7 -22
@@ -340,14 +340,10 @@ struct rcu_data {#ifdef CONFIG_RCU_NOCB_CPU struct rcu_head *nocb_head;/* CBs waiting for kthread. */ struct rcu_head **nocb_tail; -atomic_long_t nocb_q_count;/* # CBs waiting for kthread */ -atomic_long_t nocb_q_count_lazy;/* (approximate). */ +atomic_long_t nocb_q_count;/* # CBs waiting for nocb */ +atomic_long_t nocb_q_count_lazy;/* invocation (all stages). */ struct rcu_head *nocb_follower_head;/* CBs ready to invoke. */ struct rcu_head **nocb_follower_tail; -atomic_long_t nocb_follower_count;/* # CBs ready to invoke. */ -atomic_long_t nocb_follower_count_lazy;/* (approximate). */ -int nocb_p_count;/* # CBs being invoked by kthread */ -int nocb_p_count_lazy;/* (approximate). */ wait_queue_head_t nocb_wq;/* For nocb kthreads to sleep on. */ struct task_struct *nocb_kthread;int nocb_defer_wakeup;/* Defer wakeup of nocb_kthread. */
@@ -356,8 +352,6 @@ struct rcu_data {struct rcu_head *nocb_gp_head____cacheline_internodealigned_in_smp ;/* CBs waiting for GP. */ struct rcu_head **nocb_gp_tail; -long nocb_gp_count; -long nocb_gp_count_lazy;bool nocb_leader_sleep;/* Is the nocb leader thread asleep? */ struct rcu_data *nocb_next_follower;/* Next follower in wakeup chain. */
@@ -622,24 +616,15 @@ static void rcu_dynticks_task_exit(void);#endif /* #ifndef RCU_TREE_NONCORE */ #ifdef CONFIG_RCU_TRACE -#ifdef CONFIG_RCU_NOCB_CPU -/* Sum up queue lengths for tracing. */ +/* Read out queue lengths for tracing. */ static inline void rcu_nocb_q_lengths (struct rcu_data *rdp,long *ql,long *qll) { - *ql =atomic_long_read (&rdp->nocb_q_count) + - rdp->nocb_p_count + -atomic_long_read (&rdp->nocb_follower_count) + - rdp->nocb_p_count + rdp->nocb_gp_count; - *qll =atomic_long_read (&rdp->nocb_q_count_lazy) + - rdp->nocb_p_count_lazy + -atomic_long_read (&rdp->nocb_follower_count_lazy) + - rdp->nocb_p_count_lazy + rdp->nocb_gp_count_lazy; -} +#ifdef CONFIG_RCU_NOCB_CPU + *ql =atomic_long_read (&rdp->nocb_q_count); + *qll =atomic_long_read (&rdp->nocb_q_count_lazy);#else /* #ifdef CONFIG_RCU_NOCB_CPU */ -static inline void rcu_nocb_q_lengths (struct rcu_data *rdp,long *ql,long *qll) -{ *ql =0 ; *qll =0 ; -}#endif /* #else #ifdef CONFIG_RCU_NOCB_CPU */ +}#endif /* #ifdef CONFIG_RCU_TRACE */
kernel/rcu/tree_plugin.h +28 -17
94 lines not on this page · raw message
Signature · 1 line
1.8.1.5
PATCH 10/10 rcu: Remove "select IRQ_WORK" from config TREE_RCU
In reply to Paul E. McKenneyFrom: Lai Jiangshan <[email protected]>
The 48a7639ce80c ("rcu: Make callers awaken grace-period kthread") removed the irq_work_queue(), so the TREE_RCU doesn't need irq work any more. This commit therefore updates RCU's Kconfig and
--- init/Kconfig | 2 -- kernel/rcu/tree.h | 1 - 2 files changed, 3 deletions(-)
init/Kconfig +0 -2
@@ -470,7 +470,6 @@ choice config TREE_RCU bool "Tree-based hierarchical RCU" depends on !PREEMPT && SMP - select IRQ_WORK help This option selects the RCU implementation that is designed for very large SMP system with hundreds or
@@ -480,7 +479,6 @@ config TREE_RCU config PREEMPT_RCU bool "Preemptible tree-based hierarchical RCU" depends on PREEMPT - select IRQ_WORK help This option selects the RCU implementation that is designed for very large SMP systems with hundreds or
kernel/rcu/tree.h +0 -1
@@ -27,7 +27,6 @@#include <linux/threads.h> #include <linux/cpumask.h> #include <linux/seqlock.h> -#include <linux/irq_work.h> /* * Define shape of hierarchy based on NR_CPUS, CONFIG_RCU_FANOUT, and
Signature · 1 line
1.8.1.5