Fixes for 3.12
Subsystems (4)
cover Fixes for 3.12
Hello!
This series contains a number of miscellaneous fixes:
1. Switch to expedited grace periods during hibernation operations
(courtesy of Borislav Petkov).
2. Restrict above expediting to systems of 256 or fewer CPUs.
3. Make RCU debugobjects fixup actions leak callbacks rather than
risking corrupting the callback lists.
4. Propagate above callback leaking to debugobjects's callers.
5. Make call_rcu() leak callbacks when debugobjects indicates an error.
6. Switch to expedited grace periods during suspend/resume operations
(Courtesy of Bjørn Mork).
7. Avoid needless self-wakeups from the grace-period kthreads.
8. Make list_first_or_null_rcu() use list_entry_rcu() (courtesy of
Tejun Heo).
9. Fix irq_work_queue() build error for TREE_PREEMPT_RCU for some
configurations (courtesy of James Hogan).
10. Simplify _rcu_barrier() processing per Linus Torvalds feedback.
11. Avoid signed-overflow undefined behavior in time_after() and friends.
Thanx, Paul
b/include/linux/debugobjects.h | 6 +- b/include/linux/jiffies.h | 8 +-- b/include/linux/rculist.h | 5 +- b/init/Kconfig | 1 b/kernel/rcu.h | 10 ++-- b/kernel/rcupdate.c | 100 ----------------------------------------- b/kernel/rcutree.c | 62 ++++++++++++++++++++++--- b/lib/debugobjects.c | 20 +++++--- 8 files changed, 86 insertions(+), 126 deletions(-)
PATCH 1/11 rcu: Expedite grace periods during suspend/resume
From: Borislav Petkov <[email protected]>
CONFIG_RCU_FAST_NO_HZ can increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.
[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]
--- kernel/rcutree.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
kernel/rcutree.c +18 -0
@@ -54,6 +54,7 @@#include <linux/stop_machine.h> #include <linux/random.h> #include <linux/ftrace_event.h> +#include <linux/suspend.h> #include "rcutree.h" #include <trace/events/rcu.h>
@@ -3032,6 +3033,22 @@ static int rcu_cpu_notify(struct notifier_block *self,return NOTIFY_OK ; } +static int rcu_pm_notify (struct notifier_block *self, +unsigned long action,void *hcpu) +{ +switch (action) { +case PM_HIBERNATION_PREPARE : + rcu_expedited =1 ; +break ; +case PM_POST_RESTORE : + rcu_expedited =0 ; +break ; +default : +break ; + } +return NOTIFY_OK ; +} +/* * Spawn the kthread that handles this RCU flavor's grace periods. */
@@ -3273,6 +3290,7 @@ void __init rcu_init(void)* or the scheduler are operational. */ cpu_notifier (rcu_cpu_notify,0 ); +pm_notifier (rcu_pm_notify,0 );for_each_online_cpu (cpu)rcu_cpu_notify (NULL ,CPU_UP_PREPARE , (void *)(long )cpu); }
Signature · 1 line
1.8.1.5
PATCH 2/11 rcu: Expedite during suspend and resume only on smallish systems
From: "Paul E. McKenney" <[email protected]>
Expedited grace periods are of dubious benefit on very large systems, so this commit restricts their automated use during suspend and resume to systems of 256 or fewer CPUs.
--- kernel/rcutree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/rcutree.c +2 -1
@@ -3038,7 +3038,8 @@ static int rcu_pm_notify(struct notifier_block *self, {switch (action) {case PM_HIBERNATION_PREPARE : - rcu_expedited =1 ; +if (nr_cpu_ids <=256 )/* Expediting bad for large systems. */ + rcu_expedited =1 ;break ;case PM_POST_RESTORE : rcu_expedited =0 ;
Signature · 1 line
1.8.1.5
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 02/11] rcu: Expedite during suspend and resume only on smallish systems
Quoted 7 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
Expedited grace periods are of dubious benefit on very large systems, so this commit restricts their automated use during suspend and resume to systems of 256 or fewer CPUs.
Signed-off-by Paul E. McKenney <[email protected]>
This seems odd. If expedited grace periods don't help on large systems, shouldn't you just compile them out entirely and ignore rcu_expedited, rather than just in this one special case?
In any case, if this patch still makes sense, please squash it into the previous one.
- Josh Triplett
Re: [PATCH tip/core/rcu 02/11] rcu: Expedite during suspend and resume only on smallish systems
Quoted 7 lines · Josh Triplett
Quoted 6 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
Expedited grace periods are of dubious benefit on very large systems, so this commit restricts their automated use during suspend and resume to systems of 256 or fewer CPUs.
Quoted 5 lines
Quoted 1 line
Signed-off-by Paul E. McKenney <[email protected]>This seems odd. If expedited grace periods don't help on large systems, shouldn't you just compile them out entirely and ignore
rcu_expedited, rather than just in this one special case?
Longer term, a bunch of stuff will need optimization for larger systems. Expedited RCU grace periods, rcu_barrier(), possibly even grace-period initialization and cleanup. Also smp_call_function(), along with other primitives that have broadcast semantics. These changes would allow this hack to be removed.
The most straightforward approach would be to provision helper kthreads, increasing the number of helper kthreads with the number of CPUs. But I would like to see actual problems due to the lack of scalability before adding more complexity to RCU. ;-)
Quoted 2 lines
In any case, if this patch still makes sense, please squash it into the previous one.
Fair enough, done!
Thanx, Paul
PATCH 3/11 rcu: Simplify debug-objects fixups
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
The current debug-objects fixups are complex and heavyweight, and the fixups are not complete: Even with the fixups, RCU's callback lists can still be corrupted. This commit therefore strips the fixups down to their minimal form, eliminating two of the three.
It would be even better if (for example) call_rcu() simply leaked any problematic callbacks, but for that to happen, the debug-objects system would need to inform its caller of suspicious situations. This is the subject of a later commit in this series.
--- kernel/rcupdate.c | 100 ------------------------------------------------------ 1 file changed, 100 deletions(-)
kernel/rcupdate.c +0 -100
@@ -212,43 +212,6 @@ static inline void debug_rcu_head_free(struct rcu_head *head) }/* -* fixup_init is called when: -* - an active object is initialized -*/ -static int rcuhead_fixup_init (void *addr,enum debug_obj_state state) -{ -struct rcu_head *head = addr; - -switch (state) { -case ODEBUG_STATE_ACTIVE : -/* -* Ensure that queued callbacks are all executed. -* If we detect that we are nested in a RCU read-side critical -* section, we should simply fail, otherwise we would deadlock. -* In !PREEMPT configurations, there is no way to tell if we are -* in a RCU read-side critical section or not, so we never -* attempt any fixup and just print a warning. -*/ -#ifndef CONFIG_PREEMPT -WARN_ON_ONCE (1 ); -return 0 ; -#endif -if (rcu_preempt_depth () !=0 ||preempt_count () !=0 || -irqs_disabled ()) { -WARN_ON_ONCE (1 ); -return 0 ; - } -rcu_barrier (); -rcu_barrier_sched (); -rcu_barrier_bh (); -debug_object_init (head, &rcuhead_debug_descr); -return 1 ; -default : -return 0 ; - } -} - -/* * fixup_activate is called when: * - an active object is activated * - an unknown object is activated (might be a statically initialized object)
@@ -268,69 +231,8 @@ static int rcuhead_fixup_activate(void *addr, enum debug_obj_state state)debug_object_init (head, &rcuhead_debug_descr);debug_object_activate (head, &rcuhead_debug_descr);return 0 ; - -case ODEBUG_STATE_ACTIVE : -/* -* Ensure that queued callbacks are all executed. -* If we detect that we are nested in a RCU read-side critical -* section, we should simply fail, otherwise we would deadlock. -* In !PREEMPT configurations, there is no way to tell if we are -* in a RCU read-side critical section or not, so we never -* attempt any fixup and just print a warning. -*/ -#ifndef CONFIG_PREEMPT -WARN_ON_ONCE (1 ); -return 0 ; -#endif -if (rcu_preempt_depth () !=0 ||preempt_count () !=0 || -irqs_disabled ()) { -WARN_ON_ONCE (1 ); -return 0 ; - } -rcu_barrier (); -rcu_barrier_sched (); -rcu_barrier_bh (); -debug_object_activate (head, &rcuhead_debug_descr); -return 1 ;default : -return 0 ; - } -} - -/* -* fixup_free is called when: -* - an active object is freed -*/ -static int rcuhead_fixup_free (void *addr,enum debug_obj_state state) -{ -struct rcu_head *head = addr; - -switch (state) { -case ODEBUG_STATE_ACTIVE : -/* -* Ensure that queued callbacks are all executed. -* If we detect that we are nested in a RCU read-side critical -* section, we should simply fail, otherwise we would deadlock. -* In !PREEMPT configurations, there is no way to tell if we are -* in a RCU read-side critical section or not, so we never -* attempt any fixup and just print a warning. -*/ -#ifndef CONFIG_PREEMPT -WARN_ON_ONCE (1 ); -return 0 ; -#endif -if (rcu_preempt_depth () !=0 ||preempt_count () !=0 || -irqs_disabled ()) { -WARN_ON_ONCE (1 ); -return 0 ; - } -rcu_barrier (); -rcu_barrier_sched (); -rcu_barrier_bh (); -debug_object_free (head, &rcuhead_debug_descr);return 1 ; -default : -return 0 ; } }
@@ -369,9 +271,7 @@ EXPORT_SYMBOL_GPL(destroy_rcu_head_on_stack);struct debug_obj_descr rcuhead_debug_descr = { .name ="rcu_head" , - .fixup_init = rcuhead_fixup_init, .fixup_activate = rcuhead_fixup_activate, - .fixup_free = rcuhead_fixup_free, };EXPORT_SYMBOL_GPL (rcuhead_debug_descr);#endif /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
Signature · 1 line
1.8.1.5
PATCH 4/11 debugobjects: Make debug_object_activate() return status
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
In order to better respond to things like duplicate invocations of call_rcu(), RCU needs to see the status of a call to debug_object_activate(). This would allow RCU to leak the callback in order to avoid adding freelist-reuse mischief to the duplicate invoations. This commit therefore makes debug_object_activate() return status, zero for success and -EINVAL for failure.
--- include/linux/debugobjects.h | 6 +++--- lib/debugobjects.c | 20 ++++++++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-)
include/linux/debugobjects.h +3 -3
@@ -63,7 +63,7 @@ struct debug_obj_descr {extern void debug_object_init (void *addr,struct debug_obj_descr *descr);extern void debug_object_init_on_stack (void *addr,struct debug_obj_descr *descr); -extern void debug_object_activate (void *addr,struct debug_obj_descr *descr); +extern int debug_object_activate (void *addr,struct debug_obj_descr *descr);extern void debug_object_deactivate (void *addr,struct debug_obj_descr *descr);extern void debug_object_destroy (void *addr,struct debug_obj_descr *descr);extern void debug_object_free (void *addr,struct debug_obj_descr *descr);
@@ -85,8 +85,8 @@ static inline void debug_object_init (void *addr, struct debug_obj_descr *descr) { }static inline void debug_object_init_on_stack (void *addr,struct debug_obj_descr *descr) { } -static inline void -debug_object_activate (void *addr,struct debug_obj_descr *descr) { } +static inline int +debug_object_activate (void *addr,struct debug_obj_descr *descr) {return 0 ; }static inline void debug_object_deactivate (void *addr,struct debug_obj_descr *descr) { } static inline void
lib/debugobjects.c +14 -6
@@ -381,19 +381,21 @@ void debug_object_init_on_stack(void *addr, struct debug_obj_descr *descr)* debug_object_activate - debug checks when an object is activated * @addr: address of the object * @descr: pointer to an object specific debug description structure +* Returns 0 for success, -EINVAL for check failed. */ -void debug_object_activate (void *addr,struct debug_obj_descr *descr) +int debug_object_activate (void *addr,struct debug_obj_descr *descr) {enum debug_obj_state state;struct debug_bucket *db;struct debug_obj *obj;unsigned long flags; +int ret;struct debug_obj o = { .object = addr, .state =ODEBUG_STATE_NOTAVAILABLE , .descr = descr };if (!debug_objects_enabled) -return ; +return 0 ; db =get_bucket ((unsigned long ) addr);
@@ -405,23 +407,26 @@ void debug_object_activate(void *addr, struct debug_obj_descr *descr)case ODEBUG_STATE_INIT :case ODEBUG_STATE_INACTIVE : obj->state =ODEBUG_STATE_ACTIVE ; + ret =0 ;break ;case ODEBUG_STATE_ACTIVE :debug_print_object (obj,"activate" ); state = obj->state;raw_spin_unlock_irqrestore (&db->lock, flags); -debug_object_fixup (descr->fixup_activate, addr, state); -return ; + ret =debug_object_fixup (descr->fixup_activate, addr, state); +return ret ? -EINVAL :0 ;case ODEBUG_STATE_DESTROYED :debug_print_object (obj,"activate" ); + ret = -EINVAL ;break ;default : + ret =0 ;break ; }raw_spin_unlock_irqrestore (&db->lock, flags); -return ; +return ret; }raw_spin_unlock_irqrestore (&db->lock, flags);
@@ -431,8 +436,11 @@ void debug_object_activate(void *addr, struct debug_obj_descr *descr)* true or not. */ if (debug_object_fixup (descr->fixup_activate, addr, -ODEBUG_STATE_NOTAVAILABLE )) +ODEBUG_STATE_NOTAVAILABLE )) {debug_print_object (&o,"activate" ); +return -EINVAL ; + } +return 0 ; }/**
Signature · 1 line
1.8.1.5
PATCH 5/11 rcu: Make call_rcu() leak callbacks for debug-object errors
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
If someone does a duplicate call_rcu(), the worst thing the second call_rcu() could do would be to actually queue the callback the second time because doing so corrupts whatever list the callback was already queued on. This commit therefore makes __call_rcu() check the new return value from debug-objects and leak the callback upon error. This commit also substitutes rcu_leak_callback() for whatever callback function was previously in place in order to avoid freeing the callback out from under any readers that might still be referencing it.
These changes increase the probability that the debug-objects error messages will actually make it somewhere visible.
--- kernel/rcu.h | 10 +++++++--- kernel/rcutree.c | 14 +++++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-)
kernel/rcu.h +7 -3
@@ -67,12 +67,15 @@extern struct debug_obj_descr rcuhead_debug_descr; -static inline void debug_rcu_head_queue (struct rcu_head *head) +static inline int debug_rcu_head_queue (struct rcu_head *head) { -debug_object_activate (head, &rcuhead_debug_descr); +int r1; + + r1 =debug_object_activate (head, &rcuhead_debug_descr);debug_object_active_state (head, &rcuhead_debug_descr,STATE_RCU_HEAD_READY ,STATE_RCU_HEAD_QUEUED ); +return r1; }static inline void debug_rcu_head_unqueue (struct rcu_head *head)
@@ -83,8 +86,9 @@ static inline void debug_rcu_head_unqueue(struct rcu_head *head) debug_object_deactivate(head, &rcuhead_debug_descr); }#else /* !CONFIG_DEBUG_OBJECTS_RCU_HEAD */ -static inline void debug_rcu_head_queue (struct rcu_head *head) +static inline int debug_rcu_head_queue (struct rcu_head *head) { +return 0 ; }static inline void debug_rcu_head_unqueue (struct rcu_head *head)
kernel/rcutree.c +13 -1
@@ -2305,6 +2305,13 @@ static void __call_rcu_core(struct rcu_state *rsp, struct rcu_data *rdp, }/* +* RCU callback function to leak a callback. +*/ +static void rcu_leak_callback (struct rcu_head *rhp) +{ +} + +/* * Helper function for call_rcu() and friends. The cpu argument will * normally be -1, indicating "currently running CPU". It may specify * a CPU only if that CPU is a no-CBs CPU. Currently, only _rcu_barrier()
@@ -2318,7 +2325,12 @@ __call_rcu(struct rcu_head *head, void (*func)(struct rcu_head *rcu),struct rcu_data *rdp;WARN_ON_ONCE ((unsigned long )head &0x3 );/* Misaligned rcu_head! */ -debug_rcu_head_queue (head); +if (debug_rcu_head_queue (head)) { +/* Probable double call_rcu(), so leak the callback. */ +ACCESS_ONCE (head->func) = rcu_leak_callback; +WARN_ONCE (1 ,"__call_rcu(): Leaked duplicate callback\n" ); +return ; + } head->func = func; head->next =NULL ;
Signature · 1 line
1.8.1.5
PATCH 6/11 rcu: Switch to exedited grace periods for suspend as well as hibernation
In reply to Paul E. McKenneyFrom: Bjørn Mork <[email protected]>
Commit 587ff2cf ("rcu: Expedite grace periods during suspend/resume") enabled expedited grace periods for hibernation, but not for suspend. The same issue applies to both cases, so this commit simply applies the same logic by adding additional cases to the switch statement.
Note that this commit also switches from PM_POST_RESTORE to the combination of PM_POST_HIBERNATION and PM_POST_SUSPEND. A separate patch from Borislav Petkov corrects the documentation to indicate that this is necessary.
--- kernel/rcutree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/rcutree.c +3 -1
@@ -3050,10 +3050,12 @@ static int rcu_pm_notify(struct notifier_block *self, {switch (action) {case PM_HIBERNATION_PREPARE : +case PM_SUSPEND_PREPARE :if (nr_cpu_ids <=256 )/* Expediting bad for large systems. */ rcu_expedited =1 ;break ; -case PM_POST_RESTORE : +case PM_POST_HIBERNATION : +case PM_POST_SUSPEND : rcu_expedited =0 ;break ;default :
Signature · 1 line
1.8.1.5
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 06/11] rcu: Switch to exedited grace periods for suspend as well as hibernation
Quoted 7 lines · Paul E. McKenney
From: Bjørn Mork <[email protected]>
Commit
587ff2cf("rcu: Expedite grace periods during suspend/resume") enabled expedited grace periods for hibernation, but not for suspend. The same issue applies to both cases, so this commit simply applies the same logic by adding additional cases to the switch statement.
Quoted 7 lines
Note that this commit also switches from
PM_POST_RESTOREto the combination ofPM_POST_HIBERNATIONandPM_POST_SUSPEND. A separate patch from Borislav Petkov corrects the documentation to indicate that this is necessary.Signed-off-by Bjørn Mork <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>
Please squash this together with the other two relevant patches in this series.
- Josh Triplett
Re: [PATCH tip/core/rcu 06/11] rcu: Switch to exedited grace periods for suspend as well as hibernation
Quoted 13 lines · Josh Triplett
Quoted 12 lines · Paul E. McKenney
From: Bjørn Mork <[email protected]>
Commit
587ff2cf("rcu: Expedite grace periods during suspend/resume") enabled expedited grace periods for hibernation, but not for suspend. The same issue applies to both cases, so this commit simply applies the same logic by adding additional cases to the switch statement.Note that this commit also switches from
PM_POST_RESTOREto the combination ofPM_POST_HIBERNATIONandPM_POST_SUSPEND. A separate patch from Borislav Petkov corrects the documentation to indicate that this is necessary.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Bjørn Mork <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>Please squash this together with the other two relevant patches in this series.
Done! (Of course, retaining both Borislav's and Bjørn's Signed-off-bys.)
Thanx, Paul
PATCH 7/11 rcu: Avoid redundant grace-period kthread wakeups
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
When setting up an in-the-future "advanced" grace period, the code needs to wake up the relevant grace-period kthread, which it currently does unconditionally. However, this results in needless wakeups in the case where the advanced grace period is being set up by the grace-period kthread itself, which is a non-uncommon situation. This commit therefore checks to see if the running thread is the grace-period kthread, and avoids doing the irq_work_queue()-mediated wakeup in that case.
--- kernel/rcutree.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
kernel/rcutree.c +5 -3
@@ -1576,10 +1576,12 @@ rcu_start_gp_advanced(struct rcu_state *rsp, struct rcu_node *rnp,/* * We can't do wakeups while holding the rnp->lock, as that -* could cause possible deadlocks with the rq->lock. Deter -* the wakeup to interrupt context. +* could cause possible deadlocks with the rq->lock. Defer +* the wakeup to interrupt context. And don't bother waking +* up the running kthread. */ -irq_work_queue (&rsp->wakeup_work); +if (current != rsp->gp_kthread) +irq_work_queue (&rsp->wakeup_work); }/*
Signature · 1 line
1.8.1.5
PATCH 8/11 rculist: list_first_or_null_rcu() should use list_entry_rcu()
In reply to Paul E. McKenneyFrom: Tejun Heo <[email protected]>
list_first_or_null() should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in several ways.
* It compares __kernel @__ptr with __rcu @__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)
* It doesn't perform rcu_dereference*() and computes the entry address using container_of() directly from the __rcu pointer which is inconsitent with other rculist interface. As a result, all three in-kernel users - net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.
* While ->next dereference passes through list_next_rcu(), the compiler is still free to fetch ->next more than once and thus nullify the "__ptr != __next" condition check.
Fix it by making list_first_or_null_rcu() dereference ->next directly using ACCESS_ONCE() and then use list_entry_rcu() on it like other rculist accessors.
v2: Paul pointed out that the compiler may fetch the pointer more than
once nullifying the condition check. ACCESS_ONCE() added on ->next dereference.
v3: Restored () around macro param which was accidentally removed.
Spotted by Paul.
--- include/linux/rculist.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
include/linux/rculist.h +3 -2
@@ -267,8 +267,9 @@ static inline void list_splice_init_rcu(struct list_head *list,*/ #define list_first_or_null_rcu (ptr, type, member) \ ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = ACCESS_ONCE(__ptr->next); \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
Signature · 1 line
1.8.1.5
PATCH 9/11 rcu: Select IRQ_WORK from TREE_PREEMPT_RCU
In reply to Paul E. McKenneyFrom: James Hogan <[email protected]>
TREE_RCU and TREE_PREEMPT_RCU both cause kernel/rcutree.c to be built, but only TREE_RCU selects IRQ_WORK, which can result in an undefined reference to irq_work_queue for some (random) configs:
kernel/built-in.o In function `rcu_start_gp_advanced':kernel/rcutree.c:1564: undefined reference to `irq_work_queue'
Select IRQ_WORK from TREE_PREEMPT_RCU too to fix this.
--- init/Kconfig | 1 + 1 file changed, 1 insertion(+)
init/Kconfig +1 -0
@@ -470,6 +470,7 @@ config TREE_RCU config TREE_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
Signature · 1 line
1.8.1.5
PATCH 10/11 rcu: Simplify _rcu_barrier() processing
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
This commit drops an unneeded ACCESS_ONCE() and simplifies an "our work is done" check in _rcu_barrier(). This applies feedback from Linus
(https://lkml.org/lkml/2013/7/26/777) that he gave to similar code in an unrelated patch.
--- kernel/rcutree.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
kernel/rcutree.c +13 -2
@@ -2817,9 +2817,20 @@ static void _rcu_barrier(struct rcu_state *rsp)* transition. The "if" expression below therefore rounds the old * value up to the next even number and adds two before comparing. */ - snap_done =ACCESS_ONCE (rsp->n_barrier_done); + snap_done = rsp->n_barrier_done;_rcu_barrier_trace (rsp,"Check" ,-1 , snap_done); -if (ULONG_CMP_GE (snap_done, ((snap +1 ) & ~0x1 ) +2 )) { + +/* +* If the value in snap is odd, we needed to wait for the current +* rcu_barrier() to complete, then wait for the next one, in other +* words, we need the value of snap_done to be three larger than +* the value of snap. On the other hand, if the value in snap is +* even, we only had to wait for the next rcu_barrier() to complete, +* in other words, we need the value of snap_done to be only two +* greater than the value of snap. The "(snap + 3) & 0x1" computes +* this for us (thank you, Linus!). +*/ +if (ULONG_CMP_GE (snap_done, (snap +3 ) & ~0x1 )) {_rcu_barrier_trace (rsp,"EarlyExit" ,-1 , snap_done);smp_mb ();/* caller's subsequent code after above check. */ mutex_unlock (&rsp->barrier_mutex);
Signature · 1 line
1.8.1.5
PATCH 11/11 jiffies: Avoid undefined behavior from signed overflow
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of time_after(), time_after_eq(), time_after64(), and time_after_eq64() to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.
Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be happy to act as a witness for its being committed to an insane asylum. (Although I have nothing against saturating arithmetic or signals in some cases, these things really should not be the default.)
[ paulmck: Included time_after64() and time_after_eq64(), as suggested
by Eric Dumazet, also fixed commit message.]
--- include/linux/jiffies.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
include/linux/jiffies.h +4 -4
@@ -101,13 +101,13 @@ static inline u64 get_jiffies_64(void)#define time_after (a,b) \ (typecheck(unsigned long, a) && \ typecheck(unsigned long, b) && \ - ((long)(b) - (long)(a) < 0)) + ((long)((b) - (a)) < 0))#define time_before (a,b) time_after(b,a)#define time_after_eq (a,b) \ (typecheck(unsigned long, a) && \ typecheck(unsigned long, b) && \ - ((long)(a) - (long)(b) >= 0)) + ((long)((a) - (b)) >= 0))#define time_before_eq (a,b) time_after_eq(b,a)/*
@@ -130,13 +130,13 @@ static inline u64 get_jiffies_64(void)#define time_after64 (a,b) \ (typecheck(__u64, a) && \ typecheck(__u64, b) && \ - ((__s64)(b) - (__s64)(a) < 0)) + ((__s64)((b) - (a)) < 0))#define time_before64 (a,b) time_after64(b,a)#define time_after_eq64 (a,b) \ (typecheck(__u64, a) && \ typecheck(__u64, b) && \ - ((__s64)(a) - (__s64)(b) >= 0)) + ((__s64)((a) - (b)) >= 0))#define time_before_eq64 (a,b) time_after_eq64(b,a)#define time_in_range64 (a, b, c) \
Signature · 1 line
1.8.1.5
Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow
Quoted 11 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of
time_after(),time_after_eq(),time_after64(), andtime_after_eq64()to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.
Quoted 4 lines
Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be
s/free come/free to come/
Quoted 1 line
happy to act as a witness for its being committed to an insane asylum.
With the typo above fixed:
Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow
Quoted 12 lines · Josh Triplett
Quoted 11 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of
time_after(),time_after_eq(),time_after64(), andtime_after_eq64()to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.
Quoted 6 lines
Quoted 4 lines
Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be
s/free come/free to come/
Good catch, fixed!
Thanx, Paul
Quoted 5 lines
Quoted 1 line
happy to act as a witness for its being committed to an insane asylum.
With the typo above fixed:
Reviewed-by Josh Triplett <[email protected]>
Josh Triplett
maintainerRe: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow
Quoted 13 lines · Paul E. McKenney
Quoted 12 lines · Josh Triplett
Quoted 11 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of
time_after(),time_after_eq(),time_after64(), andtime_after_eq64()to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be
s/free come/free to come/
Good catch, fixed!
Just realized when looking at this again that there's another typo:
"two-complement" should be "two's-complement".
Quoted 8 lines
Thanx, Paul
Quoted 5 lines
Quoted 1 line
happy to act as a witness for its being committed to an insane asylum.
With the typo above fixed:
Reviewed-by Josh Triplett <[email protected]>
Re: [PATCH tip/core/rcu 11/11] jiffies: Avoid undefined behavior from signed overflow
Quoted 19 lines · Josh Triplett
Quoted 18 lines · Paul E. McKenney
Quoted 17 lines · Josh Triplett
Quoted 15 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
According to the C standard 3.4.3p3, overflow of a signed integer results in undefined behavior. This commit therefore changes the definitions of
time_after(),time_after_eq(),time_after64(), andtime_after_eq64()to avoid this undefined behavior. The trick is that the subtraction is done using unsigned arithmetic, which according to 6.2.5p9 cannot overflow because it is defined as modulo arithmetic. This has the added (though admittedly quite small) benefit of shortening two lines of code by four characters each.Note that the C standard considers the cast from unsigned to signed to be implementation-defined, see 6.3.1.3p3. However, on a two-complement system, an implementation that defines anything other than a reinterpretation of the bits is free come to me, and I will be
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
s/free come/free to come/
Good catch, fixed!
Just realized when looking at this again that there's another typo:
"two-complement" should be "two's-complement".
OK, fixed that as well. ;-)
Thank you for all the reviews and comments!
Thanx, Paul
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
happy to act as a witness for its being committed to an insane asylum.
With the typo above fixed:
Reviewed-by Josh Triplett <[email protected]>
Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume
In reply to Paul E. McKenneyQuoted 7 lines · Paul E. McKenney
From: Borislav Petkov <[email protected]>
CONFIG_RCU_FAST_NO_HZcan increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.
Quoted 5 lines
[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]Signed-off-by Borislav Petkov <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>
This patch still seems like a hack, and there *ought* to be a better general solution to avoid excessive grace-period latency. Nonetheless, in the absence of such a solution,
Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Borislav Petkov <[email protected]>
CONFIG_RCU_FAST_NO_HZcan increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]
Quoted 7 lines
Quoted 2 lines
Signed-off-by Borislav Petkov <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>This patch still seems like a hack, and there *ought* to be a better general solution to avoid excessive grace-period latency. Nonetheless, in the absence of such a solution,
Reviewed-by Josh Triplett <[email protected]>
Yeah, I'm not happy about it either but from quickly skimming over what context we're using rcu_expedited in, the basic requirement for a fix is for the pm core to be able to tell rcu not to stretch grace periods.
So simply setting a variable is much simpler than switching to calling all those *_expedited() rcu flavors from the pm notifier when going down.
Unless Paul has a better idea, of course.
The basic problem here is, however, that you need to temporarily reconfigure the inner workings of a subsystem because hardware is performing a power state transition. Unless someone teaches rcu about power state transitions... :-)
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume
Quoted 28 lines · Borislav Petkov
Quoted 11 lines · Josh Triplett
Quoted 10 lines · Paul E. McKenney
From: Borislav Petkov <[email protected]>
CONFIG_RCU_FAST_NO_HZcan increase grace-period durations by up to a factor of four, which can result in long suspend and resume times. Thus, this commit temporarily switches to expedited grace periods when suspending the box and return to normal settings when resuming.[ paulmck: This also papers over an audio/irq bug, but hopefully that will
be fixed soon. ]Quoted 7 lines
Quoted 2 lines
Signed-off-by Borislav Petkov <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>This patch still seems like a hack, and there *ought* to be a better general solution to avoid excessive grace-period latency. Nonetheless, in the absence of such a solution,
Reviewed-by Josh Triplett <[email protected]>Yeah, I'm not happy about it either but from quickly skimming over what context we're using
rcu_expeditedin, the basic requirement for a fix is for the pm core to be able to tell rcu not to stretch grace periods.So simply setting a variable is much simpler than switching to calling all those
*_expedited()rcu flavors from the pm notifier when going down.
Quoted 6 lines
Unless Paul has a better idea, of course.
The basic problem here is, however, that you need to temporarily reconfigure the inner workings of a subsystem because hardware is performing a power state transition. Unless someone teaches rcu about power state transitions... :-)
I would guess that once we have a few more subsystems that want RCU to temporarily expedite its grace periods, we will have enough information and experience to do something a bit more general. In the meantime, I am opting for something simple. ;-)
Thanx, Paul
Re: [PATCH tip/core/rcu 01/11] rcu: Expedite grace periods during suspend/resume
Quoted 4 lines · Paul E. McKenney
I would guess that once we have a few more subsystems that want RCU to temporarily expedite its grace periods, we will have enough information and experience to do something a bit more general. In the meantime, I am opting for something simple. ;-)
Aah, the empirical approach! Very wise, master Jedi :-)
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--Quoted 24 lines · Paul E. McKenney
Hello!
This series contains a number of miscellaneous fixes:
1. Switch to expedited grace periods during hibernation operations
(courtesy of Borislav Petkov).2. Restrict above expediting to systems of 256 or fewer CPUs.
3. Make RCU debugobjects fixup actions leak callbacks rather than
risking corrupting the callback lists.4. Propagate above callback leaking to debugobjects's callers.
5. Make
call_rcu()leak callbacks when debugobjects indicates an error.6. Switch to expedited grace periods during suspend/resume operations
(Courtesy of Bjørn Mork).7. Avoid needless self-wakeups from the grace-period kthreads.
8. Make
list_first_or_null_rcu()uselist_entry_rcu()(courtesy of
Tejun Heo).
Quoted 6 lines
9. Fix
irq_work_queue()build error forTREE_PREEMPT_RCUfor some
configurations (courtesy of James Hogan).10. Simplify
_rcu_barrier()processing per Linus Torvalds feedback.11. Avoid signed-overflow undefined behavior in
time_after()and friends.
Comments on 2, 6, and 11; for the rest: