Preemptible-RCU updates for 3.20
Subsystems (2)
cover Preemptible-RCU updates for 3.20
Hello!
This series fixes some problems in preemptible RCU.
1. Protect rcu_boost() lockless accesses with ACCESS_ONCE().
2-11. Fix a low-probability long-standing bug in preemptible RCU that
could occur in systems with more than 16 CPUs, but only if a full bank of 16 CPUs (as in 0-15, 16-31, 32-47, ...) was taken offline. See http://paulmck.livejournal.com/37782.html for more details.
12. Revert old commit that worked around an rt_mutex bug. This old
commit could cause priority inversion, and the rt_mutex bug has long since been fixed. Courtesy of Lai Jiangshan.
13. Simplify grace-period code, enabled by 2-11 above.
14. Remove redundant RCU callback list initialization.
Thanx, Paul
------------------------------------------------------------------------
b/kernel/rcu/tree.c | 115 +++++++++++++------------- b/kernel/rcu/tree.h | 24 ----- b/kernel/rcu/tree_plugin.h | 196 ++++++++++----------------------------------- 3 files changed, 102 insertions(+), 233 deletions(-)
PATCH 1/14 rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
From: "Paul E. McKenney" <[email protected]>
This commit prevents random compiler optimizations by applying ACCESS_ONCE() to lockless accesses.
--- kernel/rcu/tree_plugin.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/rcu/tree_plugin.h +2 -1
@@ -1127,7 +1127,8 @@ static int rcu_boost(struct rcu_node *rnp)struct task_struct *t;struct list_head *tb; -if (rnp->exp_tasks ==NULL && rnp->boost_tasks ==NULL ) +if (ACCESS_ONCE (rnp->exp_tasks) ==NULL && +ACCESS_ONCE (rnp->boost_tasks) ==NULL )return 0 ;/* Nothing left to boost. */ raw_spin_lock_irqsave (&rnp->lock, flags);
Signature · 1 line
1.8.1.5
PATCH 6/14 rcu: Shorten irq-disable region in rcu_cleanup_dead_cpu()
From: "Paul E. McKenney" <[email protected]>
Now that we are not migrating callbacks, there is no need to hold the ->orphan_lock across the the ->qsmaskinit bit-clearing process. This commit therefore releases ->orphan_lock immediately after adopting the orphaned RCU callbacks.
--- kernel/rcu/tree.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/rcu/tree.c +2 -2
@@ -2289,14 +2289,14 @@ static void rcu_cleanup_dead_cpu(int cpu, struct rcu_state *rsp)/* Orphan the dead CPU's callbacks, and adopt them if appropriate. */ rcu_send_cbs_to_orphanage (cpu, rsp, rnp, rdp);rcu_adopt_orphan_cbs (rsp, flags); +raw_spin_unlock_irqrestore (&rsp->orphan_lock, flags);/* Remove outgoing CPU from mask in the leaf rcu_node structure. */ -raw_spin_lock (&rnp->lock);/* irqs already disabled. */ +raw_spin_lock_irqsave (&rnp->lock, flags);smp_mb__after_unlock_lock ();/* Enforce GP memory-order guarantee. */ rnp->qsmaskinit &= ~rdp->grpmask;if (rnp->qsmaskinit ==0 && !rcu_preempt_has_tasks (rnp))rcu_cleanup_dead_rnp (rnp); -raw_spin_unlock (&rsp->orphan_lock);/* irqs remain disabled. */ raw_spin_unlock_irqrestore (&rnp->lock, flags); WARN_ONCE(rdp->qlen != 0 || rdp->nxtlist != NULL, "rcu_cleanup_dead_cpu: Callbacks on offline CPU %d: qlen=%lu, nxtlist=%p\n",
Signature · 1 line
1.8.1.5
PATCH 13/14 rcu: Don't scan root rcu_node structure for stalled tasks
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
Now that blocked tasks are no longer migrated to the root rcu_node structure, there is no need to scan the root rcu_node structure for blocked tasks stalling the current grace period. This commit therefore removes this scan.
--- kernel/rcu/tree.c | 9 --------- 1 file changed, 9 deletions(-)
kernel/rcu/tree.c +0 -9
@@ -1107,15 +1107,6 @@ static void print_other_cpu_stall(struct rcu_state *rsp)raw_spin_unlock_irqrestore (&rnp->lock, flags); } -/* -* Now rat on any tasks that got kicked up to the root rcu_node -* due to CPU offlining. -*/ - rnp =rcu_get_root (rsp); -raw_spin_lock_irqsave (&rnp->lock, flags); - ndetected +=rcu_print_task_stall (rnp); -raw_spin_unlock_irqrestore (&rnp->lock, flags); -print_cpu_stall_info_end ();for_each_possible_cpu (cpu) totqlen +=per_cpu_ptr (rsp->rda, cpu)->qlen;
Signature · 1 line
1.8.1.5
PATCH 8/14 rcu: Don't spawn rcub kthreads on root rcu_node structure
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
Now that offlining CPUs no longer moves leaf rcu_node structures' ->blkd_tasks lists to the root, there is no way for the root rcu_node structure's ->blkd_task list to be nonempty, unless the root node is also the sole leaf node. This commit therefore refrains from creating an rcub kthread for the root rcu_node structure unless it is also the sole leaf.
--- kernel/rcu/tree_plugin.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
kernel/rcu/tree_plugin.h +2 -6
@@ -1352,12 +1352,8 @@ static void __init rcu_spawn_boost_kthreads(void)for_each_possible_cpu (cpu)per_cpu (rcu_cpu_has_work, cpu) =0 ;BUG_ON (smpboot_register_percpu_thread (&rcu_cpu_thread_spec)); - rnp =rcu_get_root (rcu_state_p); - (void )rcu_spawn_one_boost_kthread (rcu_state_p, rnp); -if (NUM_RCU_NODES >1 ) { -rcu_for_each_leaf_node (rcu_state_p, rnp) - (void )rcu_spawn_one_boost_kthread (rcu_state_p, rnp); - } +rcu_for_each_leaf_node (rcu_state_p, rnp) + (void )rcu_spawn_one_boost_kthread (rcu_state_p, rnp); }static void rcu_prepare_kthreads (int cpu)
Signature · 1 line
1.8.1.5
PATCH 11/14 rcu: Note quiescent state when CPU goes offline
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
The rcu_cleanup_dead_cpu() function (called after a CPU has gone completely offline) has not reported a quiescent state because there was probably at least one synchronize_rcu() between the time the CPU went offline and the CPU_DEAD notifier, and this would have detected the CPU's offline state via quiescent-state forcing. However, the plan is for CPUs to take themselves offline, at which point it makes sense for them to report their own quiescent state. This commit makes this change in preparation for the new CPU-hotplug setup.
--- kernel/rcu/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/rcu/tree.c +1 -1
@@ -2297,7 +2297,7 @@ static void rcu_cleanup_dead_cpu(int cpu, struct rcu_state *rsp) rnp->qsmaskinit &= ~rdp->grpmask;if (rnp->qsmaskinit ==0 && !rcu_preempt_has_tasks (rnp))rcu_cleanup_dead_rnp (rnp); -raw_spin_unlock_irqrestore (&rnp->lock, flags); +rcu_report_qs_rnp (rdp->grpmask, rsp, rnp, flags);/* Rlses rnp->lock. */ WARN_ONCE (rdp->qlen !=0 || rdp->nxtlist !=NULL ,"rcu_cleanup_dead_cpu: Callbacks on offline CPU %d: qlen=%lu, nxtlist=%p\n" , cpu, rdp->qlen, rdp->nxtlist);
Signature · 1 line
1.8.1.5
PATCH 10/14 rcu: Don't bother affinitying rcub kthreads away from offline CPUs
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
When rcu_boost_kthread_setaffinity() sees that all CPUs for a given rcu_node structure are now offline, it affinities the corresponding RCU-boost ("rcub") kthread away from those CPUs. This is pointless because the kthread cannot run on those offline CPUs in any case. This commit therefore removes this unneeded code.
--- kernel/rcu/tree_plugin.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
kernel/rcu/tree_plugin.h +1 -5
@@ -1322,12 +1322,8 @@ static void rcu_boost_kthread_setaffinity(struct rcu_node *rnp, int outgoingcpu)for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask >>=1 )if ((mask &0x1 ) && cpu != outgoingcpu)cpumask_set_cpu (cpu, cm); -if (cpumask_weight (cm) ==0 ) { +if (cpumask_weight (cm) ==0 )cpumask_setall (cm); -for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++) -cpumask_clear_cpu (cpu, cm); -WARN_ON_ONCE (cpumask_weight (cm) ==0 ); - }set_cpus_allowed_ptr (t, cm);free_cpumask_var (cm); }
Signature · 1 line
1.8.1.5
PATCH 7/14 rcu: Make use of rcu_preempt_has_tasks()
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
Given that there is now arcu_preempt_has_tasks() function that checks to see if the ->blkd_tasks list is non-empty, this commit makes use of it.
--- kernel/rcu/tree_plugin.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/rcu/tree_plugin.h +3 -3
@@ -540,7 +540,7 @@ static int rcu_print_task_stall(struct rcu_node *rnp)static void rcu_preempt_check_blocked_tasks (struct rcu_node *rnp) {WARN_ON_ONCE (rcu_preempt_blocked_readers_cgp (rnp)); -if (!list_empty (&rnp->blkd_tasks)) +if (rcu_preempt_has_tasks (rnp)) rnp->gp_tasks = rnp->blkd_tasks.next;WARN_ON_ONCE (rnp->qsmask); }
@@ -706,7 +706,7 @@ sync_rcu_preempt_exp_init(struct rcu_state *rsp, struct rcu_node *rnp)raw_spin_lock_irqsave (&rnp->lock, flags);smp_mb__after_unlock_lock (); -if (list_empty (&rnp->blkd_tasks)) { +if (!rcu_preempt_has_tasks (rnp)) {raw_spin_unlock_irqrestore (&rnp->lock, flags); }else { rnp->exp_tasks = rnp->blkd_tasks.next;
@@ -985,7 +985,7 @@ void exit_rcu(void)static void rcu_initiate_boost_trace (struct rcu_node *rnp) { -if (list_empty (&rnp->blkd_tasks)) +if (!rcu_preempt_has_tasks (rnp)) rnp->n_balk_blkd_tasks++;else if (rnp->exp_tasks ==NULL && rnp->gp_tasks ==NULL ) rnp->n_balk_exp_gp_tasks++;
Signature · 1 line
1.8.1.5
PATCH 12/14 rcu: Revert "Allow post-unlock reference for rt_mutex" to avoid priority-inversion
In reply to Paul E. McKenneyFrom: Lai Jiangshan <[email protected]>
The patch dfeb9765ce3c ("Allow post-unlock reference for rt_mutex") ensured rcu-boost safe even the rt_mutex has post-unlock reference.
But rt_mutex allowing post-unlock reference is definitely a bug and it was fixed by the commit 27e35715df54 ("rtmutex: Plug slow unlock race"). This fix made the previous patch (dfeb9765ce3c) useless.
And even worse, the priority-inversion introduced by the the previous patch still exists.
rcu_read_unlock_special() {
rt_mutex_unlock(&rnp->boost_mtx);
/* Priority-Inversion:
* the current task had been deboosted and preempted as a low
* priority task immediately, it could wait long before reschedule in,
* and the rcu-booster also waits on this low priority task and sleeps.
* This priority-inversion makes rcu-booster can't work
* as expected.
*/
complete(&rnp->boost_completion);
}
Just revert the patch to avoid it.
--- kernel/rcu/tree.h | 5 ----- kernel/rcu/tree_plugin.h | 8 +------- 2 files changed, 1 insertion(+), 12 deletions(-)
kernel/rcu/tree.h +0 -5
@@ -172,11 +172,6 @@ struct rcu_node {/* queued on this rcu_node structure that */ /* are blocking the current grace period, */ /* there can be no such task. */ -struct completion boost_completion; -/* Used to ensure that the rt_mutex used */ -/* to carry out the boosting is fully */ -/* released with no future boostee accesses */ -/* before that rt_mutex is re-initialized. */ struct rt_mutex boost_mtx;/* Used only for the priority-boosting */ /* side effect, not as a lock. */
kernel/rcu/tree_plugin.h +1 -7
@@ -429,10 +429,8 @@ void rcu_read_unlock_special(struct task_struct *t)#ifdef CONFIG_RCU_BOOST /* Unboost if we were boosted. */ -if (drop_boost_mutex) { +if (drop_boost_mutex)rt_mutex_unlock (&rnp->boost_mtx); -complete (&rnp->boost_completion); - }#endif /* #ifdef CONFIG_RCU_BOOST */ /*
@@ -1081,15 +1079,11 @@ static int rcu_boost(struct rcu_node *rnp)*/ t = container_of(tb, struct task_struct, rcu_node_entry);rt_mutex_init_proxy_locked (&rnp->boost_mtx, t); -init_completion (&rnp->boost_completion);raw_spin_unlock_irqrestore (&rnp->lock, flags);/* Lock only for side effect: boosts task t's priority. */ rt_mutex_lock (&rnp->boost_mtx);rt_mutex_unlock (&rnp->boost_mtx);/* Then keep lockdep happy. */ -/* Wait for boostee to be done w/boost_mtx before reinitializing. */ -wait_for_completion (&rnp->boost_completion); -return ACCESS_ONCE (rnp->exp_tasks) !=NULL ||ACCESS_ONCE (rnp->boost_tasks) !=NULL ; }
Signature · 1 line
1.8.1.5
PATCH 14/14 rcu: Remove redundant callback-list initialization
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
The RCU callback lists are initialized in both rcu_boot_init_percpu_data() and rcu_init_percpu_data(). The former is intended for initializing immutable data, so this commit removes the initialization from rcu_boot_init_percpu_data() and leaves it in rcu_init_percpu_data(). This change prepares for permitting callbacks to be queued very early in boot.
--- kernel/rcu/tree.c | 3 --- 1 file changed, 3 deletions(-)
kernel/rcu/tree.c +0 -3
@@ -3419,9 +3419,6 @@ rcu_boot_init_percpu_data(int cpu, struct rcu_state *rsp)/* Set up local state, ensuring consistent view of global state. */ raw_spin_lock_irqsave (&rnp->lock, flags); rdp->grpmask =1UL << (cpu - rdp->mynode->grplo); -init_callback_list (rdp); - rdp->qlen_lazy =0 ; -ACCESS_ONCE (rdp->qlen) =0 ; rdp->dynticks = &per_cpu (rcu_dynticks, cpu);WARN_ON_ONCE (rdp->dynticks->dynticks_nesting !=DYNTICK_TASK_EXIT_IDLE );WARN_ON_ONCE (atomic_read (&rdp->dynticks->dynticks) !=1 );
Signature · 1 line
1.8.1.5
PATCH 9/14 rcu: Don't initiate RCU priority boosting on root rcu_node
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
Because there is no longer any preempted tasks on the root rcu_node, and because there is no longer ever an rcub kthread for the root rcu_node, this commit drops the code in force_qs_rnp() that attempts to awaken the non-existent root rcub kthread. This is strictly a performance enhancement, removing a root rcu_node ->lock acquisition and release along with some tests in rcu_initiate_boost(), ending with the test that notes that there is no rcub kthread.
--- kernel/rcu/tree.c | 6 ------ 1 file changed, 6 deletions(-)
kernel/rcu/tree.c +0 -6
@@ -2513,12 +2513,6 @@ static void force_qs_rnp(struct rcu_state *rsp, }raw_spin_unlock_irqrestore (&rnp->lock, flags); } - rnp =rcu_get_root (rsp); -if (rnp->qsmask ==0 ) { -raw_spin_lock_irqsave (&rnp->lock, flags); -smp_mb__after_unlock_lock (); -rcu_initiate_boost (rnp, flags);/* releases rnp->lock. */ - } }/*
Signature · 1 line
1.8.1.5
PATCH 3/14 rcu: Abstract rcu_cleanup_dead_rnp() from rcu_cleanup_dead_cpu()
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
This commit abstracts rcu_cleanup_dead_rnp() from rcu_cleanup_dead_cpu() in preparation for the rework of RCU priority boosting. This new function will be invoked from rcu_read_unlock_special() in the reworked scheme, which is why rcu_cleanup_dead_rnp() assumes that the leaf rcu_node structure's ->qsmaskinit field has already been updated.
--- kernel/rcu/tree.c | 67 ++++++++++++++++++++++++++++++++++-------------- kernel/rcu/tree.h | 1 + kernel/rcu/tree_plugin.h | 17 ++++++++++++ 3 files changed, 66 insertions(+), 19 deletions(-)
kernel/rcu/tree.c +48 -19
@@ -2227,6 +2227,46 @@ static void rcu_cleanup_dying_cpu(struct rcu_state *rsp) }/* +* All CPUs for the specified rcu_node structure have gone offline, +* and all tasks that were preempted within an RCU read-side critical +* section while running on one of those CPUs have since exited their RCU +* read-side critical section. Some other CPU is reporting this fact with +* the specified rcu_node structure's ->lock held and interrupts disabled. +* This function therefore goes up the tree of rcu_node structures, +* clearing the corresponding bits in the ->qsmaskinit fields. Note that +* the leaf rcu_node structure's ->qsmaskinit field has already been +* updated +* +* This function does check that the specified rcu_node structure has +* all CPUs offline and no blocked tasks, so it is OK to invoke it +* prematurely. That said, invoking it after the fact will cost you +* a needless lock acquisition. So once it has done its work, don't +* invoke it again. +*/ +static void rcu_cleanup_dead_rnp (struct rcu_node *rnp_leaf) +{ +long mask; +struct rcu_node *rnp = rnp_leaf; + +if (rnp->qsmaskinit ||rcu_preempt_has_tasks (rnp)) +return ; +for (;;) { + mask = rnp->grpmask; + rnp = rnp->parent; +if (!rnp) +break ; +raw_spin_lock (&rnp->lock);/* irqs already disabled. */ +smp_mb__after_unlock_lock ();/* GP memory ordering. */ + rnp->qsmaskinit &= ~mask; +if (rnp->qsmaskinit) { +raw_spin_unlock (&rnp->lock);/* irqs remain disabled. */ +return ; + } +raw_spin_unlock (&rnp->lock);/* irqs remain disabled. */ + } +} + +/* * The CPU has been completely removed, and some other CPU is reporting * this fact from process context. Do the remainder of the cleanup, * including orphaning the outgoing CPU's RCU callbacks, and also
@@ -2236,7 +2276,6 @@ static void rcu_cleanup_dying_cpu(struct rcu_state *rsp)static void rcu_cleanup_dead_cpu (int cpu,struct rcu_state *rsp) {unsigned long flags; -unsigned long mask;int need_report =0 ;struct rcu_data *rdp =per_cpu_ptr (rsp->rda, cpu);struct rcu_node *rnp = rdp->mynode;/* Outgoing CPU's rdp & rnp. */
@@ -2252,24 +2291,14 @@ static void rcu_cleanup_dead_cpu(int cpu, struct rcu_state *rsp)rcu_send_cbs_to_orphanage (cpu, rsp, rnp, rdp);rcu_adopt_orphan_cbs (rsp, flags); -/* Remove the outgoing CPU from the masks in the rcu_node hierarchy. */ - mask = rdp->grpmask;/* rnp->grplo is constant. */ -do { -raw_spin_lock (&rnp->lock);/* irqs already disabled. */ -smp_mb__after_unlock_lock (); - rnp->qsmaskinit &= ~mask; -if (rnp->qsmaskinit !=0 ) { -if (rnp != rdp->mynode) -raw_spin_unlock (&rnp->lock);/* irqs remain disabled. */ -break ; - } -if (rnp == rdp->mynode) - need_report =rcu_preempt_offline_tasks (rsp, rnp, rdp); -else -raw_spin_unlock (&rnp->lock);/* irqs remain disabled. */ - mask = rnp->grpmask; - rnp = rnp->parent; - }while (rnp !=NULL ); +/* Remove outgoing CPU from mask in the leaf rcu_node structure. */ +raw_spin_lock (&rnp->lock);/* irqs already disabled. */ +smp_mb__after_unlock_lock ();/* Enforce GP memory-order guarantee. */ + rnp->qsmaskinit &= ~rdp->grpmask; +if (rnp->qsmaskinit ==0 ) { + need_report =rcu_preempt_offline_tasks (rsp, rnp, rdp); +rcu_cleanup_dead_rnp (rnp); + }/* * We still hold the leaf rcu_node structure lock here, and
kernel/rcu/tree.h +1 -0
@@ -552,6 +552,7 @@ static int rcu_preempt_blocked_readers_cgp(struct rcu_node *rnp);#ifdef CONFIG_HOTPLUG_CPU static void rcu_report_unblock_qs_rnp (struct rcu_node *rnp,unsigned long flags); +static bool rcu_preempt_has_tasks (struct rcu_node *rnp);#endif /* #ifdef CONFIG_HOTPLUG_CPU */ static void rcu_print_detail_task_stall (struct rcu_state *rsp);static int rcu_print_task_stall (struct rcu_node *rnp);
kernel/rcu/tree_plugin.h +17 -0
@@ -307,6 +307,15 @@ static struct list_head *rcu_next_node_entry(struct task_struct *t, }/* +* Return true if the specified rcu_node structure has tasks that were +* preempted within an RCU read-side critical section. +*/ +static bool rcu_preempt_has_tasks (struct rcu_node *rnp) +{ +return !list_empty (&rnp->blkd_tasks); +} + +/* * Handle special cases during rcu_read_unlock(), such as needing to * notify RCU core processing or task having blocked during the RCU * read-side critical section.
@@ -967,6 +976,14 @@ static void rcu_report_unblock_qs_rnp(struct rcu_node *rnp, unsigned long flags) raw_spin_unlock_irqrestore(&rnp->lock, flags); } +/* +* Because there is no preemptible RCU, there can be no readers blocked. +*/ +static bool rcu_preempt_has_tasks (struct rcu_node *rnp) +{ +return false; +} +#endif /* #ifdef CONFIG_HOTPLUG_CPU */ /*
Signature · 1 line
1.8.1.5
PATCH 2/14 rcu: Rename "empty" to "empty_norm" in preparation for boost rework
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
This commit undertakes a simple variable renaming to make way for some rework of RCU priority boosting.
--- kernel/rcu/tree_plugin.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/rcu/tree_plugin.h +3 -3
@@ -313,8 +313,8 @@ static struct list_head *rcu_next_node_entry(struct task_struct *t,*/ void rcu_read_unlock_special (struct task_struct *t) { -int empty;int empty_exp; +int empty_norm;int empty_exp_now;unsigned long flags;struct list_head *np;
@@ -367,7 +367,7 @@ void rcu_read_unlock_special(struct task_struct *t)break ;raw_spin_unlock (&rnp->lock);/* irqs remain disabled. */ } - empty = !rcu_preempt_blocked_readers_cgp (rnp); + empty_norm = !rcu_preempt_blocked_readers_cgp (rnp); empty_exp = !rcu_preempted_readers_exp (rnp);smp_mb ();/* ensure expedited fastpath sees end of RCU c-s. */ np =rcu_next_node_entry (t, rnp);
@@ -393,7 +393,7 @@ void rcu_read_unlock_special(struct task_struct *t)* so we must take a snapshot of the expedited state. */ empty_exp_now = !rcu_preempted_readers_exp (rnp); -if (!empty && !rcu_preempt_blocked_readers_cgp (rnp)) { +if (!empty_norm && !rcu_preempt_blocked_readers_cgp (rnp)) { trace_rcu_quiescent_state_report(TPS("preempt_rcu"), rnp->gpnum, 0, rnp->qsmask,
Signature · 1 line
1.8.1.5
PATCH 5/14 rcu: Don't migrate blocked tasks even if all corresponding CPUs offline
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
When the last CPU associated with a given leaf rcu_node structure goes offline, something must be done about the tasks queued on that rcu_node structure. Each of these tasks has been preempted on one of the leaf rcu_node structure's CPUs while in an RCU read-side critical section that it have not yet exited. Handling these tasks is the job of rcu_preempt_offline_tasks(), which migrates them from the leaf rcu_node structure to the root rcu_node structure.
Unfortunately, this migration has to be done one task at a time because each tasks allegiance must be shifted from the original leaf rcu_node to the root, so that future attempts to deal with these tasks will acquire the root rcu_node structure's ->lock rather than that of the leaf. Worse yet, this migration must be done with interrupts disabled, which is not so good for realtime response, especially given that there is no bound on the number of tasks on a given rcu_node structure's list. (OK, OK, there is a bound, it is just that it is unreasonably large, especially on 64-bit systems.) This was not considered a problem back when rcu_preempt_offline_tasks() was first written because realtime systems were assumed not to do CPU-hotplug operations while real-time applications were running. This assumption has proved of dubious validity given that people are starting to run multiple realtime applications on a single SMP system and that it is common practice to offline then online a CPU before starting its real-time application in order to clear extraneous processing off of that CPU. So we now need CPU hotplug operations to avoid undue latencies.
This commit therefore avoids migrating these tasks, instead letting them be dequeued one by one from the original leaf rcu_node structure by rcu_read_unlock_special(). This means that the clearing of bits from the upper-level rcu_node structures must be deferred until the last such task has been dequeued, because otherwise subsequent grace periods won't wait on them. This commit has the beneficial side effect of simplifying the CPU-hotplug code for TREE_PREEMPT_RCU, especially in CONFIG_RCU_BOOST builds.
--- kernel/rcu/tree.c | 20 +------- kernel/rcu/tree.h | 18 ------- kernel/rcu/tree_plugin.h | 126 +---------------------------------------------- 3 files changed, 4 insertions(+), 160 deletions(-)
kernel/rcu/tree.c +2 -18
@@ -2276,7 +2276,6 @@ static void rcu_cleanup_dead_rnp(struct rcu_node *rnp_leaf)static void rcu_cleanup_dead_cpu (int cpu,struct rcu_state *rsp) {unsigned long flags; -int need_report =0 ;struct rcu_data *rdp =per_cpu_ptr (rsp->rda, cpu);struct rcu_node *rnp = rdp->mynode;/* Outgoing CPU's rdp & rnp. */
@@ -2295,25 +2294,10 @@ static void rcu_cleanup_dead_cpu(int cpu, struct rcu_state *rsp)raw_spin_lock (&rnp->lock);/* irqs already disabled. */ smp_mb__after_unlock_lock ();/* Enforce GP memory-order guarantee. */ rnp->qsmaskinit &= ~rdp->grpmask; -if (rnp->qsmaskinit ==0 ) { - need_report =rcu_preempt_offline_tasks (rsp, rnp, rdp); +if (rnp->qsmaskinit ==0 && !rcu_preempt_has_tasks (rnp))rcu_cleanup_dead_rnp (rnp); - } - -/* -* We still hold the leaf rcu_node structure lock here, and -* irqs are still disabled. The reason for this subterfuge is -* because invoking rcu_report_unblock_qs_rnp() with ->orphan_lock -* held leads to deadlock. -*/ raw_spin_unlock (&rsp->orphan_lock);/* irqs remain disabled. */ - rnp = rdp->mynode; -if (need_report &RCU_OFL_TASKS_NORM_GP ) -rcu_report_unblock_qs_rnp (rnp, flags); -else -raw_spin_unlock_irqrestore (&rnp->lock, flags); -if (need_report &RCU_OFL_TASKS_EXP_GP ) -rcu_report_exp_rnp (rsp, rnp, true); +raw_spin_unlock_irqrestore (&rnp->lock, flags);WARN_ONCE (rdp->qlen !=0 || rdp->nxtlist !=NULL ,"rcu_cleanup_dead_cpu: Callbacks on offline CPU %d: qlen=%lu, nxtlist=%p\n" , cpu, rdp->qlen, rdp->nxtlist);
kernel/rcu/tree.h +0 -18
@@ -514,13 +514,6 @@ extern struct list_head rcu_struct_flavors;#define for_each_rcu_flavor (rsp) \ list_for_each_entry((rsp), &rcu_struct_flavors, flavors) -/* Return values for rcu_preempt_offline_tasks(). */ - -#define RCU_OFL_TASKS_NORM_GP 0x1/* Tasks blocking normal */ -/* GP were moved to root. */ -#define RCU_OFL_TASKS_EXP_GP 0x2/* Tasks blocking expedited */ -/* GP were moved to root. */ -/* * RCU implementation internal declarations: */
@@ -550,24 +543,13 @@ long rcu_batches_completed(void);static void rcu_preempt_note_context_switch (void );static int rcu_preempt_blocked_readers_cgp (struct rcu_node *rnp);#ifdef CONFIG_HOTPLUG_CPU -static void rcu_report_unblock_qs_rnp (struct rcu_node *rnp, -unsigned long flags);static bool rcu_preempt_has_tasks (struct rcu_node *rnp);#endif /* #ifdef CONFIG_HOTPLUG_CPU */ static void rcu_print_detail_task_stall (struct rcu_state *rsp);static int rcu_print_task_stall (struct rcu_node *rnp);static void rcu_preempt_check_blocked_tasks (struct rcu_node *rnp); -#ifdef CONFIG_HOTPLUG_CPU -static int rcu_preempt_offline_tasks (struct rcu_state *rsp, -struct rcu_node *rnp, -struct rcu_data *rdp); -#endif /* #ifdef CONFIG_HOTPLUG_CPU */ static void rcu_preempt_check_callbacks (void );void call_rcu (struct rcu_head *head,void (*func)(struct rcu_head *rcu)); -#if defined(CONFIG_HOTPLUG_CPU ) || defined(CONFIG_PREEMPT_RCU ) -static void rcu_report_exp_rnp (struct rcu_state *rsp,struct rcu_node *rnp, -bool wake); -#endif /* #if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_PREEMPT_RCU) */ static void __init __rcu_init_preempt (void );static void rcu_initiate_boost (struct rcu_node *rnp,unsigned long flags);static void rcu_preempt_boost_start_gp (struct rcu_node *rnp);
kernel/rcu/tree_plugin.h +2 -124
156 lines not on this page · raw message
Signature · 1 line
1.8.1.5
PATCH 4/14 rcu: Make rcu_read_unlock_special() propagate ->qsmaskinit bit clearing
In reply to Paul E. McKenneyFrom: "Paul E. McKenney" <[email protected]>
This commit causes rcu_read_unlock_special() to propagate ->qsmaskinit bit clearing up the rcu_node tree once a given rcu_node structure's blkd_tasks list becomes empty. This is the final commit in preparation for the rework of RCU priority boosting: It enables preempted tasks to remain queued on their rcu_node structure even after all of that rcu_node structure's CPUs have gone offline.
--- kernel/rcu/tree.c | 4 ++++ kernel/rcu/tree_plugin.h | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)
kernel/rcu/tree.c +4 -0
@@ -2329,6 +2329,10 @@ static void rcu_cleanup_dying_cpu(struct rcu_state *rsp) { } +static void __maybe_unused rcu_cleanup_dead_rnp (struct rcu_node *rnp_leaf) +{ +} +static void rcu_cleanup_dead_cpu (int cpu,struct rcu_state *rsp) { }
kernel/rcu/tree_plugin.h +13 -3
@@ -322,9 +322,10 @@ static bool rcu_preempt_has_tasks(struct rcu_node *rnp)*/ void rcu_read_unlock_special (struct task_struct *t) { -int empty_exp; -int empty_norm; -int empty_exp_now; +bool empty; +bool empty_exp; +bool empty_norm; +bool empty_exp_now;unsigned long flags;struct list_head *np;#ifdef CONFIG_RCU_BOOST
@@ -376,6 +377,7 @@ void rcu_read_unlock_special(struct task_struct *t)break ;raw_spin_unlock (&rnp->lock);/* irqs remain disabled. */ } + empty = !rcu_preempt_has_tasks (rnp); empty_norm = !rcu_preempt_blocked_readers_cgp (rnp); empty_exp = !rcu_preempted_readers_exp (rnp);smp_mb ();/* ensure expedited fastpath sees end of RCU c-s. */
@@ -396,6 +398,14 @@ void rcu_read_unlock_special(struct task_struct *t)#endif /* #ifdef CONFIG_RCU_BOOST */ /* +* If this was the last task on the list, go see if we +* need to propagate ->qsmaskinit bit clearing up the +* rcu_node tree. +*/ +if (!empty && !rcu_preempt_has_tasks (rnp)) +rcu_cleanup_dead_rnp (rnp); + +/* * If this was the last task on the current list, and if * we aren't waiting on any CPUs, report the quiescent state. * Note that rcu_report_unblock_qs_rnp() releases rnp->lock,
Signature · 1 line
1.8.1.5
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
In reply to Paul E. McKenneyQuoted 15 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
This commit prevents random compiler optimizations by applying
ACCESS_ONCE()to lockless accesses.Signed-off-by Paul E. McKenney <[email protected]>--- kernel/rcu/tree_plugin.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/rcu/tree_plugin.h +0
-0
kernel/rcu/tree_plugin.h static int rcu_boost(struct rcu_node *rnp) · line 1132
Quoted 7 lines
kernel/rcu/tree_plugin.h +2
-1@@ -1127,7 +1127,8 @@ static int rcu_boost(struct rcu_node *rnp)struct task_struct *t;struct list_head *tb; -if (rnp->exp_tasks ==NULL && rnp->boost_tasks ==NULL ) +if (ACCESS_ONCE (rnp->exp_tasks) ==NULL && +ACCESS_ONCE (rnp->boost_tasks) ==NULL )return 0 ;/* Nothing left to boost. */
Didn't we just obsolete ACCESS_ONCE with that {READ,WRITE}_ONCE stuff?
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 19 lines · Peter Zijlstra
Quoted 18 lines · Paul E. McKenney
From: "Paul E. McKenney" <[email protected]>
This commit prevents random compiler optimizations by applying
ACCESS_ONCE()to lockless accesses.Signed-off-by Paul E. McKenney <[email protected]>--- kernel/rcu/tree_plugin.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/rcu/tree_plugin.h +0
-0@@ -1127,7 +1127,8 @@ static int rcu_boost(struct rcu_node *rnp)struct task_struct *t;struct list_head *tb;
Quoted 6 lines
kernel/rcu/tree_plugin.h static int rcu_boost(struct rcu_node *rnp) · line 1132
Quoted 4 lines
kernel/rcu/tree_plugin.h +2
-1-if (rnp->exp_tasks ==NULL && rnp->boost_tasks ==NULL ) +if (ACCESS_ONCE (rnp->exp_tasks) ==NULL && +ACCESS_ONCE (rnp->boost_tasks) ==NULL )return 0 ;/* Nothing left to boost. */ Didn't we just obsolete
ACCESS_ONCEwith that {READ,WRITE}_ONCE stuff?
Indeed we did! But that was after I did this commit back on October 29th.
I am planning a bulk change to READ_ONCE() and ASSIGN_ONCE() either as the last patch for 3.20 or as the first one for 3.21. Probably as the first for 3.21 to minimize rebasing hassles with any needed 3.20 fixes.
Thanx, Paul
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 7 lines · Paul E. McKenney
Quoted 1 line
Didn't we just obsolete
ACCESS_ONCEwith that {READ,WRITE}_ONCE stuff?Indeed we did! But that was after I did this commit back on October 29th.
I am planning a bulk change to
READ_ONCE()andASSIGN_ONCE()either as the last patch for 3.20 or as the first one for 3.21. Probably as the first for 3.21 to minimize rebasing hassles with any needed 3.20 fixes.
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
- ACCESS_ONCE(a) = b;
+ ASSIGN_ONCE(b, a);
Thanks,
Davidlohr
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 12 lines · Davidlohr Bueso
Quoted 7 lines · Paul E. McKenney
Quoted 1 line
Didn't we just obsolete
ACCESS_ONCEwith that {READ,WRITE}_ONCE stuff?Indeed we did! But that was after I did this commit back on October 29th.
I am planning a bulk change to
READ_ONCE()andASSIGN_ONCE()either as the last patch for 3.20 or as the first one for 3.21. Probably as the first for 3.21 to minimize rebasing hassles with any needed 3.20 fixes.That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
Review
Quoted 2 lines
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);
I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
Thanx, Paul
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 10 lines · Paul E. McKenney
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance rcu_assign_pointer() takes them the right way around, as does atomic_set().
So yes, I think the ASSIGN_ONCE() thing got the arguments the wrong way around.
We could maybe still change it, before its in too long ?
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Paul E. McKenney
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
Quoted 8 lines
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.
Ah, yes, I had forgotten about this point.
Quoted 1 line
We could maybe still change it, before its in too long ?
I would be in favor. Any objections? ;-)
Thanx, Paul
Mathieu Desnoyers
reviewerRe: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
----- Original Message -----
Quoted 29 lines
From: "Paul E. McKenney" <[email protected]>
To: "Peter Zijlstra" <[email protected]>
Cc: "Davidlohr Bueso" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], "mathieu desnoyers" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "bobby prani" <[email protected]>, [email protected]
Sent: Friday, January 9, 2015 9:07:34 AM
Subject: Re: [PATCH tip/core/rcu 01/14] rcu: Protectrcu_boost()lockless accesses withACCESS_ONCE()Quoted 18 lines · Peter Zijlstra
Quoted 10 lines · Paul E. McKenney
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().
Quoted 8 lines
Quoted 2 lines
So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.Ah, yes, I had forgotten about this point.
Quoted 1 line
We could maybe still change it, before its in too long ?
I would be in favor. Any objections? ;-)
Agreed. The target argument should be the first parameter. Also, do you plan to make ASSIGN_ONCE() evaluate to something or void ? In userland liburcu CMM_STORE_SHARED(), which evaluates to the value used as input, we hit warnings with static checkers due to unused value, which is unfortunate. Not sure if the checkers should be thought better, or if evaluating to void is the right approach there.
Thanks,
Mathieu
Quoted 4 lines
Thanx, Paul
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
In reply to Peter ZijlstraAm 09.01.2015 um 14:56 schrieb Peter Zijlstra:
Quoted 14 lines · Paul E. McKenney
Quoted 10 lines
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Quoted 8 lines
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.We could maybe still change it, before its in too long ?
Linus initial proposal was inspired by put_user model which is (val, ptr) and I took that.
As my focus was on avoiding the volatile bug, all my current conversions are READ_ONCE as no potential ASSIGN_ONCE user was done on a non-scalar type, so I have no first hand experience. I am fine with changing that, though, both ways have pros and cons. Last time I checked in Linus tree there was no ASSIGN_ONCE user.
When we talk about changing the parameters it might make sense to also think about some comments from George Spelvin and consider a rename to WRITE_ONCE or STORE_ONCE (READ_ONCE --> LOAD_ONCE). Unfortunately there doesnt seem to be a variant that is fool proof (in the sense of Rustys guideline that a good interface cannot be used wrong). So any proposal in that regard would be very welcome.
Christian
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 1 line · Christian Borntraeger
When we talk about changing the parameters it might make sense to also think about some comments from George Spelvin and consider a rename to
WRITE_ONCEorSTORE_ONCE(READ_ONCE-->LOAD_ONCE). Unfortunately there doesnt seem to be a variant that is fool proof (in the sense of Rustys guideline that a good interface cannot be used wrong). So any proposal in that regard would be very welcome.
I think any of those two names are a lot better than what we have now. I'm a bit inclined towards LOAD/STORE though. That, and reordering the arguments for stores would make it pretty fool proof imo.
Thanks,
Davidlohr
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
In reply to Christian BorntraegerQuoted 19 lines · Christian Borntraeger
Am 09.01.2015 um 14:56 schrieb Peter Zijlstra:
Quoted 18 lines · Paul E. McKenney
Quoted 10 lines
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().
Quoted 7 lines
Quoted 4 lines
So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.We could maybe still change it, before its in too long ?
Linus initial proposal was inspired by
put_usermodel which is (val, ptr) and I took that.
Yeah, like I said, its a bit of a mixed bag. We've got plenty examples of the wrong way around.
Quoted 4 lines
As my focus was on avoiding the volatile bug,
all my current conversions areREAD_ONCEas no potentialASSIGN_ONCEuser was done on a non-scalar type, so I have no first hand experience.
So the implication there is that we'd preserve ACCESS_ONCE() for use on scalar types. I don't think we should do that, I think we should just en-mass convert to {READ,WRITE}/{LOAD,STORE}_ONCE() and kill off ACCESS_ONCE().
Quoted 3 lines
I am fine with changing that, though, both ways have pros and cons. Last time I checked in Linus tree there was no
ASSIGN_ONCEuser.
Right, so Davidlohr just introduced a few in my tree :-), which is how I came to know we even had this stuff..
Quoted 3 lines
When we talk about changing the parameters it might make sense to also think about some comments from George Spelvin and consider a rename to
WRITE_ONCEorSTORE_ONCE(READ_ONCE-->LOAD_ONCE).
I'd be OK with that.
Quoted 4 lines
Unfortunately
there doesnt seem to be a variant that is fool proof (in the sense of Rustys guideline that a good interface cannot be used wrong). So any proposal in that regard would be very welcome.
If you want fool proof, I think we should discard C ;-) Then again, I've yet to see a programming language that would not let a human make a proper idiot out of himself.
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 36 lines · Peter Zijlstra
Quoted 19 lines · Christian Borntraeger
Am 09.01.2015 um 14:56 schrieb Peter Zijlstra:
Quoted 18 lines · Paul E. McKenney
Quoted 10 lines
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().Quoted 7 lines
Quoted 4 lines
So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.We could maybe still change it, before its in too long ?
Linus initial proposal was inspired by
put_usermodel which is (val, ptr) and I took that.Yeah, like I said, its a bit of a mixed bag. We've got plenty examples of the wrong way around.
Quoted 4 lines
As my focus was on avoiding the volatile bug,
all my current conversions areREAD_ONCEas no potentialASSIGN_ONCEuser was done on a non-scalar type, so I have no first hand experience.
Quoted 4 lines
So the implication there is that we'd preserve
ACCESS_ONCE()for use on scalar types. I don't think we should do that, I think we should just en-mass convert to {READ,WRITE}/{LOAD,STORE}_ONCE() and kill offACCESS_ONCE().
Yep. For one thing, the proposed replacements work much better with C11 than does ACCESS_ONCE().
Quoted 13 lines
Quoted 3 lines
I am fine with changing that, though, both ways have pros and cons. Last time I checked in Linus tree there was no
ASSIGN_ONCEuser.Right, so Davidlohr just introduced a few in my tree :-), which is how I came to know we even had this stuff..
Quoted 3 lines
When we talk about changing the parameters it might make sense to also think about some comments from George Spelvin and consider a rename to
WRITE_ONCEorSTORE_ONCE(READ_ONCE-->LOAD_ONCE).I'd be OK with that.
Quoted 8 lines
Quoted 4 lines
Unfortunately
there doesnt seem to be a variant that is fool proof (in the sense of Rustys guideline that a good interface cannot be used wrong). So any proposal in that regard would be very welcome.If you want fool proof, I think we should discard C ;-) Then again, I've yet to see a programming language that would not let a human make a proper idiot out of himself.
Limit NR_CPUS to zero! It is the only way!!!
Thanx, Paul
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Am 12.01.2015 um 23:12 schrieb Paul E. McKenney:
Quoted 37 lines · Peter Zijlstra
Quoted 36 lines · Christian Borntraeger
Quoted 19 lines
Am 09.01.2015 um 14:56 schrieb Peter Zijlstra:
Quoted 18 lines · Paul E. McKenney
Quoted 10 lines
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().Quoted 7 lines
Quoted 4 lines
So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.We could maybe still change it, before its in too long ?
Linus initial proposal was inspired by
put_usermodel which is (val, ptr) and I took that.Yeah, like I said, its a bit of a mixed bag. We've got plenty examples of the wrong way around.
Quoted 4 lines
As my focus was on avoiding the volatile bug,
all my current conversions areREAD_ONCEas no potentialASSIGN_ONCEuser was done on a non-scalar type, so I have no first hand experience.
Quoted 7 lines
Quoted 4 lines
So the implication there is that we'd preserve
ACCESS_ONCE()for use on scalar types. I don't think we should do that, I think we should just en-mass convert to {READ,WRITE}/{LOAD,STORE}_ONCE() and kill offACCESS_ONCE().Yep. For one thing, the proposed replacements work much better with C11 than does
ACCESS_ONCE().
As we agreed there is no perfect interface regarding val,x vs. x,val.
But it seems that there is some consensus that I should push something like the following (still whitespace damaged) to Linus for 3.19? Peter, Davidlohr, Paul (maybe Linus) can you ACK/NACK?
Subject: Change ASSIGN_ONCE(val, x) to WRITE_ONCE(x, val)
Feedback has shown that WRITE_ONCE(x, val) is easier to use than ASSIGN_ONCE(val,x). There are no in-tree users yet, so lets change it.
include/linux/compiler.h +7 -7
@@ -215,7 +215,7 @@ static __always_inline void __read_once_size(volatile void *p, void *res, int si } } -static __always_inline void __assign_once_size(volatile void *p, void *res, int size) +static __always_inline void __write_once_size(volatile void *p, void *res, int size) {switch (size) {case 1 : *(volatile __u8 *)p = *(__u8 *)res;break ;
@@ -235,15 +235,15 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int/* * Prevent the compiler from merging or refetching reads or writes. The * compiler is also forbidden from reordering successive instances of -* READ_ONCE, ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the +* READ_ONCE, WRITE_ONCE and ACCESS_ONCE (see below), but only when the * compiler is aware of some particular ordering. One way to make the * compiler aware of ordering is to put the two invocations of READ_ONCE, -* ASSIGN_ONCE or ACCESS_ONCE() in different C statements. +* WRITE_ONCE or ACCESS_ONCE() in different C statements. * * In contrast to ACCESS_ONCE these two macros will also work on aggregate * data types like structs or unions. If the size of the accessed data * type exceeds the word size of the machine (e.g., 32 bits or 64 bits) -* READ_ONCE() and ASSIGN_ONCE() will fall back to memcpy and print a +* READ_ONCE() and WRITE_ONCE() will fall back to memcpy and print a * compile-time warning. * * Their two major use cases are: (1) Mediating communication between
@@ -257,8 +257,8 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int#define READ_ONCE (x) \ ({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; }) -#define ASSIGN_ONCE (val, x) \ - ({ typeof(x) __val; __val = val; __assign_once_size(&x, &__val, sizeof(__val)); __val; }) +#define WRITE_ONCE (x, val) \ + ({ typeof(x) __val; __val = val; __write_once_size(&x, &__val, sizeof(__val)); __val; })#endif /* __KERNEL__ */
@@ -458,7 +458,7 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int* with an explicit memory barrier or atomic instruction that provides the * required ordering. * -* If possible use READ_ONCE/ASSIGN_ONCE instead. +* If possible use READ_ONCE/WRITE_ONCE instead. */ #define __ACCESS_ONCE (x) ({ \ __maybe_unused typeof(x) __var = (typeof(x)) 0;
Re: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
Quoted 4 lines · Christian Borntraeger
As we agreed there is no perfect interface regarding val,x vs. x,val.
But it seems that there is some consensus that I should push something like the following (still whitespace damaged) to Linus for 3.19? Peter, Davidlohr, Paul (maybe Linus) can you ACK/NACK?
ACK on this, but I have a git tree with users in, I'll fix it up though.
Thanks!
Quoted 63 lines
Subject: Change
ASSIGN_ONCE(val, x)toWRITE_ONCE(x, val)Feedback has shown that
WRITE_ONCE(x, val)is easier to use thanASSIGN_ONCE(val,x). There are no in-tree users yet, so lets change it.Signed-off-by Christian Borntraeger <[email protected]>include/linux/compiler.h +7
-7@@ -215,7 +215,7 @@ static __always_inline void __read_once_size(volatile void *p, void *res, int si } } -static __always_inline void __assign_once_size(volatile void *p, void *res, int size) +static __always_inline void __write_once_size(volatile void *p, void *res, int size) {switch (size) {case 1 : *(volatile __u8 *)p = *(__u8 *)res;break ;@@ -235,15 +235,15 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int/* * Prevent the compiler from merging or refetching reads or writes. The * compiler is also forbidden from reordering successive instances of -* READ_ONCE, ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the +* READ_ONCE, WRITE_ONCE and ACCESS_ONCE (see below), but only when the * compiler is aware of some particular ordering. One way to make the * compiler aware of ordering is to put the two invocations of READ_ONCE, -* ASSIGN_ONCE or ACCESS_ONCE() in different C statements. +* WRITE_ONCE or ACCESS_ONCE() in different C statements. * * In contrast to ACCESS_ONCE these two macros will also work on aggregate * data types like structs or unions. If the size of the accessed data * type exceeds the word size of the machine (e.g., 32 bits or 64 bits) -* READ_ONCE() and ASSIGN_ONCE() will fall back to memcpy and print a +* READ_ONCE() and WRITE_ONCE() will fall back to memcpy and print a * compile-time warning. * * Their two major use cases are: (1) Mediating communication between @@ -257,8 +257,8 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int#define READ_ONCE (x) \ ({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; }) -#define ASSIGN_ONCE (val, x) \ - ({ typeof(x) __val; __val = val; __assign_once_size(&x, &__val, sizeof(__val)); __val; }) +#define WRITE_ONCE (x, val) \ + ({ typeof(x) __val; __val = val; __write_once_size(&x, &__val, sizeof(__val)); __val; })#endif /* __KERNEL__ */ @@ -458,7 +458,7 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int* with an explicit memory barrier or atomic instruction that provides the * required ordering. * -* If possible use READ_ONCE/ASSIGN_ONCE instead. +* If possible use READ_ONCE/WRITE_ONCE instead. */ #define __ACCESS_ONCE (x) ({ \ __maybe_unused typeof(x) __var = (typeof(x)) 0;
Paul E. McKenney
Acked-byRe: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
In reply to Christian BorntraegerQuoted 51 lines · Christian Borntraeger
Am 12.01.2015 um 23:12 schrieb Paul E. McKenney:
Quoted 37 lines · Peter Zijlstra
Quoted 36 lines · Christian Borntraeger
Quoted 19 lines
Am 09.01.2015 um 14:56 schrieb Peter Zijlstra:
Quoted 18 lines · Paul E. McKenney
Quoted 10 lines
Review
Quoted 5 lines
That reminds me, I think the new conversion for stores will most likely introduce silly arg bugs:
(quoted diff) +1
-1- ACCESS_ONCE(a) = b; + ASSIGN_ONCE(b, a);I was planning to do mine by hand for this sort of reason.
Or are you thinking of something more subtle than the case where "b" is an unparenthesized comma-separated expression?
I think he's revering to the wrong way around-ness of the thing.
Its a bit of a mixed bag on assignments, but for instance
rcu_assign_pointer()takes them the right way around, as doesatomic_set().Quoted 7 lines
Quoted 4 lines
So yes, I think the
ASSIGN_ONCE()thing got the arguments the wrong way around.We could maybe still change it, before its in too long ?
Linus initial proposal was inspired by
put_usermodel which is (val, ptr) and I took that.Yeah, like I said, its a bit of a mixed bag. We've got plenty examples of the wrong way around.
Quoted 4 lines
As my focus was on avoiding the volatile bug,
all my current conversions areREAD_ONCEas no potentialASSIGN_ONCEuser was done on a non-scalar type, so I have no first hand experience.Quoted 7 lines
Quoted 4 lines
So the implication there is that we'd preserve
ACCESS_ONCE()for use on scalar types. I don't think we should do that, I think we should just en-mass convert to {READ,WRITE}/{LOAD,STORE}_ONCE() and kill offACCESS_ONCE().Yep. For one thing, the proposed replacements work much better with C11 than does
ACCESS_ONCE().As we agreed there is no perfect interface regarding val,x vs. x,val.
But it seems that there is some consensus that I should push something like the following (still whitespace damaged) to Linus for 3.19? Peter, Davidlohr, Paul (maybe Linus) can you ACK/NACK?
Quoted 6 lines
Subject: Change
ASSIGN_ONCE(val, x)toWRITE_ONCE(x, val)Feedback has shown that
WRITE_ONCE(x, val)is easier to use thanASSIGN_ONCE(val,x). There are no in-tree users yet, so lets change it.Signed-off-by Christian Borntraeger <[email protected]>
Quoted 55 lines
include/linux/compiler.h +7
-7@@ -215,7 +215,7 @@ static __always_inline void __read_once_size(volatile void *p, void *res, int si } } -static __always_inline void __assign_once_size(volatile void *p, void *res, int size) +static __always_inline void __write_once_size(volatile void *p, void *res, int size) {switch (size) {case 1 : *(volatile __u8 *)p = *(__u8 *)res;break ;@@ -235,15 +235,15 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int/* * Prevent the compiler from merging or refetching reads or writes. The * compiler is also forbidden from reordering successive instances of -* READ_ONCE, ASSIGN_ONCE and ACCESS_ONCE (see below), but only when the +* READ_ONCE, WRITE_ONCE and ACCESS_ONCE (see below), but only when the * compiler is aware of some particular ordering. One way to make the * compiler aware of ordering is to put the two invocations of READ_ONCE, -* ASSIGN_ONCE or ACCESS_ONCE() in different C statements. +* WRITE_ONCE or ACCESS_ONCE() in different C statements. * * In contrast to ACCESS_ONCE these two macros will also work on aggregate * data types like structs or unions. If the size of the accessed data * type exceeds the word size of the machine (e.g., 32 bits or 64 bits) -* READ_ONCE() and ASSIGN_ONCE() will fall back to memcpy and print a +* READ_ONCE() and WRITE_ONCE() will fall back to memcpy and print a * compile-time warning. * * Their two major use cases are: (1) Mediating communication between @@ -257,8 +257,8 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int#define READ_ONCE (x) \ ({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; }) -#define ASSIGN_ONCE (val, x) \ - ({ typeof(x) __val; __val = val; __assign_once_size(&x, &__val, sizeof(__val)); __val; }) +#define WRITE_ONCE (x, val) \ + ({ typeof(x) __val; __val = val; __write_once_size(&x, &__val, sizeof(__val)); __val; })#endif /* __KERNEL__ */ @@ -458,7 +458,7 @@ static __always_inline void __assign_once_size(volatile void *p, void *res, int* with an explicit memory barrier or atomic instruction that provides the * required ordering. * -* If possible use READ_ONCE/ASSIGN_ONCE instead. +* If possible use READ_ONCE/WRITE_ONCE instead. */ #define __ACCESS_ONCE (x) ({ \ __maybe_unused typeof(x) __var = (typeof(x)) 0;
Davidlohr Bueso
Acked-byRe: [PATCH tip/core/rcu 01/14] rcu: Protect rcu_boost() lockless accesses with ACCESS_ONCE()
In reply to Christian BorntraegerQuoted 6 lines · Christian Borntraeger
Subject: Change
ASSIGN_ONCE(val, x)toWRITE_ONCE(x, val)Feedback has shown that
WRITE_ONCE(x, val)is easier to use thanASSIGN_ONCE(val,x). There are no in-tree users yet, so lets change it.Signed-off-by Christian Borntraeger <[email protected]>