Fixes for RCU/scheduler/irq-threads trainwreck

kernel/rcutree_plugin.h +1 -1

                if (rnp->parent == NULL) {
+                       raw_spin_unlock_irqrestore(&rnp->lock, flags);
                        wake_up(&sync_rcu_preempt_exp_wq);
                        break;
                }

@@ -707,7 +709,6 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
                raw_spin_lock(&rnp->lock); /* irqs already disabled */
                rnp->expmask &= ~mask;
        }
-       raw_spin_unlock_irqrestore(&rnp->lock, flags);
 } 

Review static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp) · line 700

Quoted 7 lines · Paul E. McKenney

+++ b/kernel/rcutree_plugin.h

(quoted diff) +1 -0

@@ -696,8 +696,10 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
        raw_spin_lock_irqsave(&rnp->lock, flags);
        for (;;) {
                if (!sync_rcu_preempt_exp_done(rnp))
+                       raw_spin_unlock_irqrestore(&rnp->lock, flags);
                        break;

I bet that'll all work much better if you wrap it in curly braces like:

		if (!sync_rcu_preempt_exp_done(rnp)) {
			raw_spin_unlock_irqrestore(&rnp->lock, flags);
			break;
		}

That might also explain those explosions Ed and Ben have been seeing.

Review static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp) · line 712

Quoted 11 lines

(quoted diff) +1 -1

                if (rnp->parent == NULL) {
+                       raw_spin_unlock_irqrestore(&rnp->lock, flags);
                        wake_up(&sync_rcu_preempt_exp_wq);
                        break;
                }

@@ -707,7 +709,6 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
                raw_spin_lock(&rnp->lock); /* irqs already disabled */
                rnp->expmask &= ~mask;
        }
-       raw_spin_unlock_irqrestore(&rnp->lock, flags);
 } 

Quoted 11 lines · Peter Zijlstra

kernel/rcutree_plugin.h static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp) · line 700

Quoted 7 lines · Paul E. McKenney

+++ b/kernel/rcutree_plugin.h

kernel/rcutree_plugin.h +1 -0

@@ -696,8 +696,10 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
        raw_spin_lock_irqsave(&rnp->lock, flags);
        for (;;) {
                if (!sync_rcu_preempt_exp_done(rnp))
+                       raw_spin_unlock_irqrestore(&rnp->lock, flags);
                        break;

I bet that'll all work much better if you wrap it in curly braces like:

Quoted 6 lines

		if (!sync_rcu_preempt_exp_done(rnp)) {
			raw_spin_unlock_irqrestore(&rnp->lock, flags);
			break;
		}

That might also explain those explosions Ed and Ben have been seeing.

Indeed. Must be the call of the snake. :-(

Thank you for catching this!

Quoted 11 lines

kernel/rcutree_plugin.h static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp) · line 712

Quoted 11 lines

kernel/rcutree_plugin.h +1 -1

                if (rnp->parent == NULL) {
+                       raw_spin_unlock_irqrestore(&rnp->lock, flags);
                        wake_up(&sync_rcu_preempt_exp_wq);
                        break;
                }

@@ -707,7 +709,6 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
                raw_spin_lock(&rnp->lock); /* irqs already disabled */
                rnp->expmask &= ~mask;
        }
-       raw_spin_unlock_irqrestore(&rnp->lock, flags);
 } 

So this time I am testing the exact patch series before resending. In the meantime, here is the updated version of this patch.

Thanx, Paul

------------------------------------------------------------------------

rcu: decrease rcu_report_exp_rnp coupling with scheduler

PREEMPT_RCU read-side critical sections blocking an expedited grace period invoke rcu_report_exp_rnp(). When the last such critical section has completed, rcu_report_exp_rnp() invokes the scheduler to wake up the task that invoked synchronize_rcu_expedited() -- needlessly holding the root rcu_node structure's lock while doing so, thus needlessly providing a way for RCU and the scheduler to deadlock.

This commit therefore releases the root rcu_node structure's lock before calling wake_up().

Reported-by Ed Tomlinson <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

kernel/rcutree_plugin.h +4 -1

index 75113cb..6abef3c 100644
@@ -695,9 +695,12 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
 
 	raw_spin_lock_irqsave(&rnp->lock, flags);
 	for (;;) {
-		if (!sync_rcu_preempt_exp_done(rnp))
+		if (!sync_rcu_preempt_exp_done(rnp)) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			break;
+		}
 		if (rnp->parent == NULL) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			wake_up(&sync_rcu_preempt_exp_wq);
 			break;
 		}

Paul/Peter,

With all 7 fixes applied with the above version of patch 1, I get a warning during boot:

[    3.553004]  [<ffffffff8104b06a>] warn_slowpath_null+0x1a/0x20
[    3.553004]  [<ffffffff810bb479>] __rcu_read_unlock+0xc9/0x120
[    3.553004]  [<ffffffff8103fed8>] cpuacct_charge+0xc8/0xe0
[    3.553004]  [<ffffffff8103fe58>] ? cpuacct_charge+0x48/0xe0
[    3.553004]  [<ffffffff810326b7>] ? task_of+0x97/0xd0
[    3.553004]  [<ffffffff81040ef5>] update_curr+0x1a5/0x210
[    3.553004]  [<ffffffff81576d78>] ? preempt_schedule_irq+0x68/0xa0

[    3.553004]  [<ffffffff810419e0>] put_prev_task_fair+0x110/0x120
[    3.553004]  [<ffffffff81575f3a>] schedule+0x1da/0xc50
[    3.553004]  [<ffffffff81576d72>] ? preempt_schedule_irq+0x62/0xa0
[    3.553004]  [<ffffffff81576d78>] preempt_schedule_irq+0x68/0xa0
[    3.553004]  [<ffffffff8157a316>] retint_kernel+0x26/0x30
[    3.553004]  [<ffffffff810da494>] ? ftrace_likely_update+0x14/0x20
[    3.553004]  [<ffffffff810bb4ab>] __rcu_read_unlock+0xfb/0x120
[    3.553004]  [<ffffffff810f8190>] find_get_page+0x170/0x190 

Quoted 2 lines · On Wednesday 20 July 2011 00:54:15 Paul E. McKenney wrote:

Quoted 1 line · Peter Zijlstra

On Tue, 2011-07-19 at 17:18 -0700, Paul E. McKenney wrote:

Paul/Peter,

With all 7 fixes applied with the above version of patch 1, I get a warning during boot:

[    3.443140] EXT4-fs (sdc3): mounted filesystem with ordered data mode. Opts: (null)
[    3.456097] VFS: Mounted root (ext4 filesystem) readonly on device 8:35.
[    3.469734] devtmpfs: mounted
[    3.478488] Freeing unused kernel memory: 628k freed
[    3.488590] Write protecting the kernel read-only data: 10240k
[    3.500809] Freeing unused kernel memory: 468k freed
[    3.514158] Freeing unused kernel memory: 1240k freed
[    3.552337] ------------[ cut here ]------------
[    3.553004] ------------[ cut here ]------------
[    3.553004] WARNING: at kernel/rcutree_plugin.h:414 __rcu_read_unlock+0xc9/0x120()
[    3.553004] Hardware name: System Product Name
[    3.553004] Modules linked in:
[    3.553004] Pid: 1, comm: init Not tainted 3.0.0-rcr-crc+ #342
[    3.553004] Call Trace:
[    3.553004]  [<ffffffff8104b00f>] warn_slowpath_common+0x7f/0xc0
[    3.553004]  [<ffffffff8104b06a>] warn_slowpath_null+0x1a/0x20
[    3.553004]  [<ffffffff810bb479>] __rcu_read_unlock+0xc9/0x120
[    3.553004]  [<ffffffff8157d6b1>] __atomic_notifier_call_chain+0x91/0xb0
[    3.553004]  [<ffffffff8157d620>] ? notifier_call_chain+0x80/0x80
[    3.553004]  [<ffffffff812c5860>] ? bit_putcs+0x5b0/0x5b0
[    3.553004]  [<ffffffff8157d6e6>] atomic_notifier_call_chain+0x16/0x20
[    3.553004]  [<ffffffff813203c9>] notify_write+0x29/0x30
[    3.553004]  [<ffffffff81322024>] vt_console_print+0x164/0x3b0
[    3.553004]  [<ffffffff8104b29e>] __call_console_drivers+0x8e/0xb0
[    3.553004]  [<ffffffff8104b30a>] _call_console_drivers+0x4a/0x80
[    3.553004]  [<ffffffff8104b9ed>] console_unlock+0xfd/0x210
[    3.553004]  [<ffffffff8104c246>] vprintk+0x3f6/0x530
[    3.553004]  [<ffffffff810bb479>] ? __rcu_read_unlock+0xc9/0x120

[    3.553004]  [<ffffffff8157585c>] printk+0x41/0x45
[    3.553004]  [<ffffffff810bb479>] ? __rcu_read_unlock+0xc9/0x120
[    3.553004]  [<ffffffff8104afcd>] warn_slowpath_common+0x3d/0xc0
[    3.553004]  [<ffffffff8104b06a>] warn_slowpath_null+0x1a/0x20
[    3.553004]  [<ffffffff810bb479>] __rcu_read_unlock+0xc9/0x120
[    3.553004]  [<ffffffff8103fed8>] cpuacct_charge+0xc8/0xe0
[    3.553004]  [<ffffffff8103fe58>] ? cpuacct_charge+0x48/0xe0
[    3.553004]  [<ffffffff810326b7>] ? task_of+0x97/0xd0

kernel/rcutree_plugin.h +2 -2

@@ -391,10 +400,15 @@ void __rcu_read_unlock(void)
        struct task_struct *t = current;
 
        barrier();  /* needed if we ever invoke rcu_read_unlock in rcutree.c */
-       if (--t->rcu_read_lock_nesting == 0) {
-               barrier();  /* decr before ->rcu_read_unlock_special load */
+       if (t->rcu_read_lock_nesting != 1)
+               --t->rcu_read_lock_nesting;

Quoted 12 lines · Paul E. McKenney

kernel/rcutree_plugin.h +5 -2

@@ -391,10 +400,15 @@ void __rcu_read_unlock(void)
        struct task_struct *t = current;
 
        barrier();  /* needed if we ever invoke rcu_read_unlock in rcutree.c */
-       if (--t->rcu_read_lock_nesting == 0) {
-               barrier();  /* decr before ->rcu_read_unlock_special load */
+       if (t->rcu_read_lock_nesting != 1)
+               --t->rcu_read_lock_nesting;
+       else {
+               t->rcu_read_lock_nesting = INT_MIN;
+               barrier();  /* assign before ->rcu_read_unlock_special load */
                if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))

kernel/rcutree_plugin.h void __rcu_read_unlock(void) · line 414

Quoted 6 lines

kernel/rcutree_plugin.h +2 -0

                        rcu_read_unlock_special(t);
+               barrier();  /* ->rcu_read_unlock_special load before assign */
+               t->rcu_read_lock_nesting = 0;
        }
 #ifdef CONFIG_PROVE_LOCKING
        WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0); 

But won't the above change make that WARN_ON_ONCE() invalid?

Hello!

This patch set contains fixes for a trainwreck involving RCU, the scheduler, and threaded interrupts. This trainwreck involved RCU failing to properly protect one of its bit fields, use of RCU by the scheduler from portions of irq_exit() where in_irq() returns false, uses of the scheduler by RCU colliding with uses of RCU by the scheduler, threaded interrupts exercising the problematic portions of irq_exit() more heavily, and so on. The patches are as follows:

1. Properly protect current->rcu_read_unlock_special().
Lack of protection was causing RCU to recurse on itself, which in turn resulted in deadlocks involving RCU and the scheduler. This affects only RCU_BOOST=y configurations.
2. Streamline code produced by __rcu_read_unlock(). This one is
an innocent bystander that is being carried due to conflicts with other patches. (A later version will likely merge it with #3 below.)
3. Make __rcu_read_unlock() delay counting the per-task
->rcu_read_lock_nesting variable to zero until all cleanup for the just-ended RCU read-side critical section has completed. This prevents a number of other cases that could result in deadlock
due to self recursion. This affects only TREE_PREEMPT_RCU=y
configurations.
4. Make scheduler_ipi() correctly identify itself as being
in_irq() when it needs to do anything that might involve RCU, thus enabling RCU to avoid yet another class of potential
self-recursions and deadlocks. This affects PREEMPT_RCU=y
configurations.
5. Make irq_exit() inform RCU when it is invoking the scheduler
in situations where in_irq() would return false, thus allowing RCU to correctly avoid self-recursion. This affects TREE_PREEMPT_RCU=y configurations.
6. Make __lock_task_sighand() execute the entire RCU read-side
critical section with irqs disabled. (An experimental patch at
http://marc.info/?l=linux-kernel&m=131110647222185 might possibly make it legal to have an RCU read-side critical section where the rcu_read_unlock() is executed with interrupts disabled, but where some protion of the RCU read-side critical section was preemptible.) This affects TREE_PREEMPT_RCU=y configurations.

TINY_PREEMPT_RCU will also need a few of these changes, but in the meantime this patch stack helps organize things better for testing. These are also available from the following subject-to-rebase git branch:

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-2.6-rcu.git rcu/urgent

This patch set contains fixes for a trainwreck involving RCU, the scheduler, and threaded interrupts. This trainwreck involved RCU failing to properly protect one of its bit fields, use of RCU by the scheduler from portions of irq_exit() where in_irq() returns false, uses of the scheduler by RCU colliding with uses of RCU by the scheduler, threaded interrupts exercising the problematic portions of irq_exit() more heavily, and so on. The patches are as follows:

1. Properly protect current->rcu_read_unlock_special().
Lack of protection was causing RCU to recurse on itself, which in turn resulted in deadlocks involving RCU and the scheduler. This affects only RCU_BOOST=y configurations.
2. Streamline code produced by __rcu_read_unlock(). This one is
an innocent bystander that is being carried due to conflicts with other patches. (A later version will likely merge it with #3 below.)
3. Make __rcu_read_unlock() delay counting the per-task
->rcu_read_lock_nesting variable to zero until all cleanup for the just-ended RCU read-side critical section has completed. This prevents a number of other cases that could result in deadlock
due to self recursion. This affects only TREE_PREEMPT_RCU=y
configurations.
4. Make scheduler_ipi() correctly identify itself as being
in_irq() when it needs to do anything that might involve RCU, thus enabling RCU to avoid yet another class of potential
self-recursions and deadlocks. This affects PREEMPT_RCU=y
configurations.
5. Make irq_exit() inform RCU when it is invoking the scheduler
in situations where in_irq() would return false, thus allowing RCU to correctly avoid self-recursion. This affects TREE_PREEMPT_RCU=y configurations.
6. Make __lock_task_sighand() execute the entire RCU read-side
critical section with irqs disabled. (An experimental patch at
http://marc.info/?l=linux-kernel&m=131110647222185 might possibly make it legal to have an RCU read-side critical section where the rcu_read_unlock() is executed with interrupts disabled, but where some protion of the RCU read-side critical section was preemptible.) This affects TREE_PREEMPT_RCU=y configurations.

TINY_PREEMPT_RCU will also need a few of these changes, but in the meantime this patch stack helps organize things better for testing. These are also available from the following subject-to-rebase git branch:

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-2.6-rcu.git rcu/urgent

Thanx, Paul

------------------------------------------------------------------------

 b/include/linux/sched.h   |    3 +++
 b/kernel/rcutree_plugin.h |    3 ++-
 b/kernel/sched.c          |   44 ++++++++++++++++++++++++++++++++++++++------
 b/kernel/signal.c         |   16 +++++++++++-----
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   45 ++++++++++++++++++++++++++++++++-------------
 6 files changed, 96 insertions(+), 27 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Pulling this on top of master and rebuilding with boost enabled and booting with threadirqs does not boot correctly here.

* Starting APC UPS daemon ...

[   46.007217]
[   46.007221] =================================
[   46.008013] [ INFO: inconsistent lock state ]
[   46.008013] 3.0.0-rc7-crc+ #340
[   46.008013] ---------------------------------
[   46.008013] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[   46.008013] ip/2982 [HC0[0]:SC0[0]:HE1:SE1] takes:
[   46.008013]  (rcu_node_level_0){+.?...}, at: [<ffffffff810ba279>] rcu_report_exp_rnp+0x19/0x50
[   46.008013] {IN-SOFTIRQ-W} state was registered at:
[   46.008013]   [<ffffffff8108a1e5>] __lock_acquire+0x5a5/0x16a0
[   46.008013]   [<ffffffff8108b8d5>] lock_acquire+0x95/0x140
[   46.008013]   [<ffffffff815793a6>] _raw_spin_lock_irqsave+0x46/0x60
[   46.008013]   [<ffffffff810bbf80>] __rcu_process_callbacks+0x190/0x2a0
[   46.008013]   [<ffffffff810bc0ed>] rcu_process_callbacks+0x5d/0x60
[   46.008013]   [<ffffffff81052e14>] __do_softirq+0x194/0x300
[   46.008013]   [<ffffffff8105311d>] run_ksoftirqd+0x19d/0x240
[   46.008013]   [<ffffffff81071296>] kthread+0xb6/0xc0
[   46.008013]   [<ffffffff81582294>] kernel_thread_helper+0x4/0x10
[   46.008013] irq event stamp: 6802
[   46.008013] hardirqs last  enabled at (6801): [<ffffffff815776bd>] __mutex_unlock_slowpath+0x10d/0x1c0
[   46.008013] hardirqs last disabled at (6802): [<ffffffff8157937c>] _raw_spin_lock_irqsave+0x1c/0x60
[   46.008013] softirqs last  enabled at (6698): [<ffffffff814c274a>] sk_common_release+0x6a/0x120
[   46.008013] softirqs last disabled at (6696): [<ffffffff81579476>] _raw_write_lock_bh+0x16/0x50
[   46.008013]
[   46.008013] other info that might help us debug this:
[   46.008013]  Possible unsafe locking scenario:
[   46.008013]
[   46.008013]        CPU0
[   46.008013]        ----
[   46.008013]   lock(rcu_node_level_0);
[   46.008013]   <Interrupt>
[   46.008013]     lock(rcu_node_level_0);
[   46.008013]
[   46.008013]  *** DEADLOCK ***
[   46.008013]
[   46.008013] 3 locks held by ip/2982:
[   46.008013]  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff814e4fd7>] rtnl_lock+0x17/0x20
[   46.008013]  #1:  (sync_rcu_preempt_exp_mutex){+.+...}, at: [<ffffffff810bc9e7>] synchronize_rcu_expedited+0x37/0x210
[   46.008013]  #2:  (rcu_node_level_0){+.?...}, at: [<ffffffff810ba279>] rcu_report_exp_rnp+0x19/0x50
[   46.008013]
[   46.008013] stack backtrace:
[   46.008013] Pid: 2982, comm: ip Tainted: G        W   3.0.0-rc7-crc+ #340
[   46.008013] Call Trace:
[   46.008013]  [<ffffffff81088c33>] print_usage_bug+0x223/0x270
[   46.008013]  [<ffffffff81089558>] mark_lock+0x328/0x400
[   46.008013]  [<ffffffff8108969e>] mark_held_locks+0x6e/0xa0
[   46.008013]  [<ffffffff81579a5d>] ? _raw_spin_unlock_irqrestore+0x7d/0x90
[   46.008013]  [<ffffffff8108999d>] trace_hardirqs_on_caller+0x14d/0x190
[   46.008013]  [<ffffffff810899ed>] trace_hardirqs_on+0xd/0x10
[   46.008013]  [<ffffffff81579a5d>] _raw_spin_unlock_irqrestore+0x7d/0x90
[   46.008013]  [<ffffffff810bcb18>] synchronize_rcu_expedited+0x168/0x210
[   46.008013]  [<ffffffff8111a073>] ? might_fault+0x53/0xb0
[   46.008013]  [<ffffffff8125200a>] ? security_capable+0x2a/0x30
[   46.008013]  [<ffffffff814d0985>] synchronize_net+0x45/0x50
[   46.008013]  [<ffffffffa0359613>] ipip6_tunnel_ioctl+0x5f3/0x800 [sit]
[   46.008013]  [<ffffffffa0359020>] ? ipip6_tunnel_locate+0x2e0/0x2e0 [sit] 
[   46.008013]  [<ffffffff814d43ba>] dev_ifsioc+0x11a/0x2c0
[   46.008013]  [<ffffffff814d678a>] dev_ioctl+0x35a/0x810
[   46.008013]  [<ffffffff81089a8d>] ? debug_check_no_locks_freed+0x9d/0x150
[   46.008013]  [<ffffffff8108999d>] ? trace_hardirqs_on_caller+0x14d/0x190
[   46.008013]  [<ffffffff810899ed>] ? trace_hardirqs_on+0xd/0x10
[   46.008013]  [<ffffffff814bb83a>] sock_ioctl+0xea/0x2b0
[   46.008013]  [<ffffffff81162474>] do_vfs_ioctl+0xa4/0x5a0
[   46.008013]  [<ffffffff815799cc>] ? _raw_spin_unlock+0x5c/0x70
[   46.008013]  [<ffffffff8114c1cc>] ? fd_install+0x7c/0x90
[   46.008013]  [<ffffffff8158149c>] ? sysret_check+0x27/0x62
[   46.008013]  [<ffffffff81162a09>] sys_ioctl+0x99/0xa0
[   46.008013]  [<ffffffff8158146b>] system_call_fastpath+0x16/0x1b
[   46.786123] BUG: sleeping function called from invalid context at net/ipv4/route.c:757
[   46.799548] in_atomic(): 1, irqs_disabled(): 0, pid: 2982, name: ip
[   46.799553] INFO: lockdep is turned off.
[   46.799561] Pid: 2982, comm: ip Tainted: G        W   3.0.0-rc7-crc+ #340
[   46.799565] Call Trace:
[   46.799576]  [<ffffffff81036b39>] __might_sleep+0xf9/0x120
[   46.799586]  [<ffffffff814fea38>] rt_do_flush+0x178/0x1b0
[   46.799594]  [<ffffffff814feafc>] rt_cache_flush+0x5c/0x70
[   46.799606]  [<ffffffff81542d92>] fib_netdev_event+0x72/0xf0
[   46.799615]  [<ffffffff8157d576>] notifier_call_chain+0x56/0x80
[   46.799625]  [<ffffffff81077ff6>] raw_notifier_call_chain+0x16/0x20
[   46.799633]  [<ffffffff814d1cd4>] call_netdevice_notifiers+0x74/0x90
[   46.799642]  [<ffffffff814d2c37>] netdev_state_change+0x27/0x40
[   46.799653]  [<ffffffffa0359686>] ipip6_tunnel_ioctl+0x666/0x800 [sit]
[   46.799663]  [<ffffffffa0359020>] ? ipip6_tunnel_locate+0x2e0/0x2e0 [sit]
[   46.799673]  [<ffffffff814d43ba>] dev_ifsioc+0x11a/0x2c0
[   46.799682]  [<ffffffff814d678a>] dev_ioctl+0x35a/0x810
[   46.799690]  [<ffffffff81089a8d>] ? debug_check_no_locks_freed+0x9d/0x150
[   46.799700]  [<ffffffff8108999d>] ? trace_hardirqs_on_caller+0x14d/0x190
[   46.799707]  [<ffffffff810899ed>] ? trace_hardirqs_on+0xd/0x10
[   46.799718]  [<ffffffff814bb83a>] sock_ioctl+0xea/0x2b0
[   46.799726]  [<ffffffff81162474>] do_vfs_ioctl+0xa4/0x5a0
[   46.799735]  [<ffffffff815799cc>] ? _raw_spin_unlock+0x5c/0x70
[   46.799744]  [<ffffffff8114c1cc>] ? fd_install+0x7c/0x90
[   46.799752]  [<ffffffff8158149c>] ? sysret_check+0x27/0x62
[   46.799760]  [<ffffffff81162a09>] sys_ioctl+0x99/0xa0
[   46.799769]  [<ffffffff8158146b>] system_call_fastpath+0x16/0x1b

Followed by more BUGs and yielding a box that needed in interrupt button pressed to force a reboot.

I've previously tested with patches 5 and 6 from Peter Z with good results with threadirqs with boost disabled.

Ed

Quoted 56 lines

This patch set contains fixes for a trainwreck involving RCU, the scheduler, and threaded interrupts. This trainwreck involved RCU failing to properly protect one of its bit fields, use of RCU by the scheduler from portions of irq_exit() where in_irq() returns false, uses of the scheduler by RCU colliding with uses of RCU by the scheduler, threaded interrupts exercising the problematic portions of irq_exit() more heavily, and so on. The patches are as follows:

1. Properly protect current->rcu_read_unlock_special().
Lack of protection was causing RCU to recurse on itself, which in turn resulted in deadlocks involving RCU and the scheduler. This affects only RCU_BOOST=y configurations.
2. Streamline code produced by __rcu_read_unlock(). This one is
an innocent bystander that is being carried due to conflicts with other patches. (A later version will likely merge it with #3 below.)
3. Make __rcu_read_unlock() delay counting the per-task
->rcu_read_lock_nesting variable to zero until all cleanup for the just-ended RCU read-side critical section has completed. This prevents a number of other cases that could result in deadlock
due to self recursion. This affects only TREE_PREEMPT_RCU=y
configurations.
4. Make scheduler_ipi() correctly identify itself as being
in_irq() when it needs to do anything that might involve RCU, thus enabling RCU to avoid yet another class of potential
self-recursions and deadlocks. This affects PREEMPT_RCU=y
configurations.
5. Make irq_exit() inform RCU when it is invoking the scheduler
in situations where in_irq() would return false, thus allowing RCU to correctly avoid self-recursion. This affects TREE_PREEMPT_RCU=y configurations.
6. Make __lock_task_sighand() execute the entire RCU read-side
critical section with irqs disabled. (An experimental patch at
http://marc.info/?l=linux-kernel&m=131110647222185 might possibly make it legal to have an RCU read-side critical section where the rcu_read_unlock() is executed with interrupts disabled, but where some protion of the RCU read-side critical section was preemptible.) This affects TREE_PREEMPT_RCU=y configurations.

TINY_PREEMPT_RCU will also need a few of these changes, but in the meantime this patch stack helps organize things better for testing. These are also available from the following subject-to-rebase git branch:

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-2.6-rcu.git rcu/urgent

Thanx, Paul

------------------------------------------------------------------------

 b/include/linux/sched.h   |    3 +++
 b/kernel/rcutree_plugin.h |    3 ++-
 b/kernel/sched.c          |   44 ++++++++++++++++++++++++++++++++++++++------
 b/kernel/signal.c         |   16 +++++++++++-----
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   45 ++++++++++++++++++++++++++++++++-------------
 6 files changed, 96 insertions(+), 27 deletions(-)

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 1 line · On Tuesday 19 July 2011 21:30:00 Ed Tomlinson wrote:

On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Paul,

Two things to add. Its possible an eariler error is missing from the log below. My serial console was missing entries from before '0 and 40ish' and I did think I saw a bunch of FFFFFFFs scroll by eariler...

Second, booting with threadirqs and boost enabled in .config with patches 2,5,6 is ok so far.

Thanks,
Ed

Quoted 178 lines

Pulling this on top of master and rebuilding with boost enabled and booting with threadirqs does not boot correctly here.

* Starting APC UPS daemon ...

[   46.007217]
[   46.007221] =================================
[   46.008013] [ INFO: inconsistent lock state ]
[   46.008013] 3.0.0-rc7-crc+ #340
[   46.008013] ---------------------------------
[   46.008013] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[   46.008013] ip/2982 [HC0[0]:SC0[0]:HE1:SE1] takes:
[   46.008013]  (rcu_node_level_0){+.?...}, at: [<ffffffff810ba279>] rcu_report_exp_rnp+0x19/0x50
[   46.008013] {IN-SOFTIRQ-W} state was registered at:
[   46.008013]   [<ffffffff8108a1e5>] __lock_acquire+0x5a5/0x16a0
[   46.008013]   [<ffffffff8108b8d5>] lock_acquire+0x95/0x140
[   46.008013]   [<ffffffff815793a6>] _raw_spin_lock_irqsave+0x46/0x60
[   46.008013]   [<ffffffff810bbf80>] __rcu_process_callbacks+0x190/0x2a0
[   46.008013]   [<ffffffff810bc0ed>] rcu_process_callbacks+0x5d/0x60
[   46.008013]   [<ffffffff81052e14>] __do_softirq+0x194/0x300
[   46.008013]   [<ffffffff8105311d>] run_ksoftirqd+0x19d/0x240
[   46.008013]   [<ffffffff81071296>] kthread+0xb6/0xc0
[   46.008013]   [<ffffffff81582294>] kernel_thread_helper+0x4/0x10
[   46.008013] irq event stamp: 6802
[   46.008013] hardirqs last  enabled at (6801): [<ffffffff815776bd>] __mutex_unlock_slowpath+0x10d/0x1c0
[   46.008013] hardirqs last disabled at (6802): [<ffffffff8157937c>] _raw_spin_lock_irqsave+0x1c/0x60
[   46.008013] softirqs last  enabled at (6698): [<ffffffff814c274a>] sk_common_release+0x6a/0x120
[   46.008013] softirqs last disabled at (6696): [<ffffffff81579476>] _raw_write_lock_bh+0x16/0x50
[   46.008013]
[   46.008013] other info that might help us debug this:
[   46.008013]  Possible unsafe locking scenario:
[   46.008013]
[   46.008013]        CPU0
[   46.008013]        ----
[   46.008013]   lock(rcu_node_level_0);
[   46.008013]   <Interrupt>
[   46.008013]     lock(rcu_node_level_0);
[   46.008013]
[   46.008013]  *** DEADLOCK ***
[   46.008013]
[   46.008013] 3 locks held by ip/2982:
[   46.008013]  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff814e4fd7>] rtnl_lock+0x17/0x20
[   46.008013]  #1:  (sync_rcu_preempt_exp_mutex){+.+...}, at: [<ffffffff810bc9e7>] synchronize_rcu_expedited+0x37/0x210
[   46.008013]  #2:  (rcu_node_level_0){+.?...}, at: [<ffffffff810ba279>] rcu_report_exp_rnp+0x19/0x50
[   46.008013]
[   46.008013] stack backtrace:
[   46.008013] Pid: 2982, comm: ip Tainted: G        W   3.0.0-rc7-crc+ #340
[   46.008013] Call Trace:
[   46.008013]  [<ffffffff81088c33>] print_usage_bug+0x223/0x270
[   46.008013]  [<ffffffff81089558>] mark_lock+0x328/0x400
[   46.008013]  [<ffffffff8108969e>] mark_held_locks+0x6e/0xa0
[   46.008013]  [<ffffffff81579a5d>] ? _raw_spin_unlock_irqrestore+0x7d/0x90
[   46.008013]  [<ffffffff8108999d>] trace_hardirqs_on_caller+0x14d/0x190
[   46.008013]  [<ffffffff810899ed>] trace_hardirqs_on+0xd/0x10
[   46.008013]  [<ffffffff81579a5d>] _raw_spin_unlock_irqrestore+0x7d/0x90
[   46.008013]  [<ffffffff810bcb18>] synchronize_rcu_expedited+0x168/0x210
[   46.008013]  [<ffffffff8111a073>] ? might_fault+0x53/0xb0
[   46.008013]  [<ffffffff8125200a>] ? security_capable+0x2a/0x30
[   46.008013]  [<ffffffff814d0985>] synchronize_net+0x45/0x50
[   46.008013]  [<ffffffffa0359613>] ipip6_tunnel_ioctl+0x5f3/0x800 [sit]
[   46.008013]  [<ffffffffa0359020>] ? ipip6_tunnel_locate+0x2e0/0x2e0 [sit] 
[   46.008013]  [<ffffffff814d43ba>] dev_ifsioc+0x11a/0x2c0
[   46.008013]  [<ffffffff814d678a>] dev_ioctl+0x35a/0x810
[   46.008013]  [<ffffffff81089a8d>] ? debug_check_no_locks_freed+0x9d/0x150
[   46.008013]  [<ffffffff8108999d>] ? trace_hardirqs_on_caller+0x14d/0x190
[   46.008013]  [<ffffffff810899ed>] ? trace_hardirqs_on+0xd/0x10
[   46.008013]  [<ffffffff814bb83a>] sock_ioctl+0xea/0x2b0
[   46.008013]  [<ffffffff81162474>] do_vfs_ioctl+0xa4/0x5a0
[   46.008013]  [<ffffffff815799cc>] ? _raw_spin_unlock+0x5c/0x70
[   46.008013]  [<ffffffff8114c1cc>] ? fd_install+0x7c/0x90
[   46.008013]  [<ffffffff8158149c>] ? sysret_check+0x27/0x62
[   46.008013]  [<ffffffff81162a09>] sys_ioctl+0x99/0xa0
[   46.008013]  [<ffffffff8158146b>] system_call_fastpath+0x16/0x1b
[   46.786123] BUG: sleeping function called from invalid context at net/ipv4/route.c:757
[   46.799548] in_atomic(): 1, irqs_disabled(): 0, pid: 2982, name: ip
[   46.799553] INFO: lockdep is turned off.
[   46.799561] Pid: 2982, comm: ip Tainted: G        W   3.0.0-rc7-crc+ #340
[   46.799565] Call Trace:
[   46.799576]  [<ffffffff81036b39>] __might_sleep+0xf9/0x120
[   46.799586]  [<ffffffff814fea38>] rt_do_flush+0x178/0x1b0
[   46.799594]  [<ffffffff814feafc>] rt_cache_flush+0x5c/0x70
[   46.799606]  [<ffffffff81542d92>] fib_netdev_event+0x72/0xf0
[   46.799615]  [<ffffffff8157d576>] notifier_call_chain+0x56/0x80
[   46.799625]  [<ffffffff81077ff6>] raw_notifier_call_chain+0x16/0x20
[   46.799633]  [<ffffffff814d1cd4>] call_netdevice_notifiers+0x74/0x90
[   46.799642]  [<ffffffff814d2c37>] netdev_state_change+0x27/0x40
[   46.799653]  [<ffffffffa0359686>] ipip6_tunnel_ioctl+0x666/0x800 [sit]
[   46.799663]  [<ffffffffa0359020>] ? ipip6_tunnel_locate+0x2e0/0x2e0 [sit]
[   46.799673]  [<ffffffff814d43ba>] dev_ifsioc+0x11a/0x2c0
[   46.799682]  [<ffffffff814d678a>] dev_ioctl+0x35a/0x810
[   46.799690]  [<ffffffff81089a8d>] ? debug_check_no_locks_freed+0x9d/0x150
[   46.799700]  [<ffffffff8108999d>] ? trace_hardirqs_on_caller+0x14d/0x190
[   46.799707]  [<ffffffff810899ed>] ? trace_hardirqs_on+0xd/0x10
[   46.799718]  [<ffffffff814bb83a>] sock_ioctl+0xea/0x2b0
[   46.799726]  [<ffffffff81162474>] do_vfs_ioctl+0xa4/0x5a0
[   46.799735]  [<ffffffff815799cc>] ? _raw_spin_unlock+0x5c/0x70
[   46.799744]  [<ffffffff8114c1cc>] ? fd_install+0x7c/0x90
[   46.799752]  [<ffffffff8158149c>] ? sysret_check+0x27/0x62
[   46.799760]  [<ffffffff81162a09>] sys_ioctl+0x99/0xa0
[   46.799769]  [<ffffffff8158146b>] system_call_fastpath+0x16/0x1b

Followed by more BUGs and yielding a box that needed in interrupt button pressed to force a reboot.

I've previously tested with patches 5 and 6 from Peter Z with good results with threadirqs with boost disabled.

Ed

Quoted 56 lines

This patch set contains fixes for a trainwreck involving RCU, the scheduler, and threaded interrupts. This trainwreck involved RCU failing to properly protect one of its bit fields, use of RCU by the scheduler from portions of irq_exit() where in_irq() returns false, uses of the scheduler by RCU colliding with uses of RCU by the scheduler, threaded interrupts exercising the problematic portions of irq_exit() more heavily, and so on. The patches are as follows:

1. Properly protect current->rcu_read_unlock_special().
Lack of protection was causing RCU to recurse on itself, which in turn resulted in deadlocks involving RCU and the scheduler. This affects only RCU_BOOST=y configurations.
2. Streamline code produced by __rcu_read_unlock(). This one is
an innocent bystander that is being carried due to conflicts with other patches. (A later version will likely merge it with #3 below.)
3. Make __rcu_read_unlock() delay counting the per-task
->rcu_read_lock_nesting variable to zero until all cleanup for the just-ended RCU read-side critical section has completed. This prevents a number of other cases that could result in deadlock
due to self recursion. This affects only TREE_PREEMPT_RCU=y
configurations.
4. Make scheduler_ipi() correctly identify itself as being
in_irq() when it needs to do anything that might involve RCU, thus enabling RCU to avoid yet another class of potential
self-recursions and deadlocks. This affects PREEMPT_RCU=y
configurations.
5. Make irq_exit() inform RCU when it is invoking the scheduler
in situations where in_irq() would return false, thus allowing RCU to correctly avoid self-recursion. This affects TREE_PREEMPT_RCU=y configurations.
6. Make __lock_task_sighand() execute the entire RCU read-side
critical section with irqs disabled. (An experimental patch at
http://marc.info/?l=linux-kernel&m=131110647222185 might possibly make it legal to have an RCU read-side critical section where the rcu_read_unlock() is executed with interrupts disabled, but where some protion of the RCU read-side critical section was preemptible.) This affects TREE_PREEMPT_RCU=y configurations.

TINY_PREEMPT_RCU will also need a few of these changes, but in the meantime this patch stack helps organize things better for testing. These are also available from the following subject-to-rebase git branch:

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-2.6-rcu.git rcu/urgent

Thanx, Paul

------------------------------------------------------------------------

 b/include/linux/sched.h   |    3 +++
 b/kernel/rcutree_plugin.h |    3 ++-
 b/kernel/sched.c          |   44 ++++++++++++++++++++++++++++++++++++++------
 b/kernel/signal.c         |   16 +++++++++++-----
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   45 ++++++++++++++++++++++++++++++++-------------
 6 files changed, 96 insertions(+), 27 deletions(-)

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines · Ed Tomlinson

Quoted 1 line · On Tuesday 19 July 2011 21:30:00 Ed Tomlinson wrote:

On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Quoted 6 lines

Paul,

Two things to add.  Its possible an eariler error is missing from the log below.  My serial console was missing entries from before '0 and 40ish' and I did think I saw a bunch of FFFFFFFs scroll by eariler...

Second, booting with threadirqs and boost enabled in .config with patches 2,5,6 is ok so far.

Patches 3 and 4 resulted in failures?  (Patch 1 certainly did.)

Quoted 6 lines · Paul E. McKenney

Quoted 5 lines · Ed Tomlinson

Quoted 1 line · On Tuesday 19 July 2011 21:30:00 Ed Tomlinson wrote:

On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Paul,

Quoted 6 lines

Quoted 4 lines

Two things to add.  Its possible an eariler error is missing from the log below.  My serial console was missing entries from before '0 and 40ish' and I did think I saw a bunch of FFFFFFFs scroll by eariler...

Second, booting with threadirqs and boost enabled in .config with patches 2,5,6 is ok so far.

Patches 3 and 4 resulted in failures?  (Patch 1 certainly did.)

Gaah. Any possibility of getting this resolved soon?

I've merged the pathname lookup fix. I was hoping all the RCU issues were behind us..

Quoted 11 lines · Linus Torvalds

Quoted 9 lines · Paul E. McKenney

Quoted 8 lines · Ed Tomlinson

Quoted 1 line · On Tuesday 19 July 2011 21:30:00 Ed Tomlinson wrote:

On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Paul,

Two things to add. Its possible an eariler error is missing from the log below. My serial console was missing entries from before '0 and 40ish' and I did think I saw a bunch of FFFFFFFs scroll by eariler...

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Second, booting with threadirqs and boost enabled in .config with patches 2,5,6 is ok so far.

Patches 3 and 4 resulted in failures? (Patch 1 certainly did.)

Gaah. Any possibility of getting this resolved soon?

I believe that we are very close. These failures all involve unusual configurations, which is why they showed up so late.

Quoted 16 lines · Paul E. McKenney

Quoted 15 lines · Linus Torvalds

Quoted 12 lines · Paul E. McKenney

Quoted 9 lines · Ed Tomlinson

Quoted 1 line · On Tuesday 19 July 2011 21:30:00 Ed Tomlinson wrote:

On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Paul,

Two things to add. Its possible an eariler error is missing from the log below. My serial console was missing entries from before '0 and 40ish' and I did think I saw a bunch of FFFFFFFs scroll by eariler...

Second, booting with threadirqs and boost enabled in .config with patches 2,5,6 is ok so far.

Patches 3 and 4 resulted in failures? (Patch 1 certainly did.)

Quoted 4 lines

Quoted 1 line

Gaah. Any possibility of getting this resolved soon?

I believe that we are very close. These failures all involve unusual configurations, which is why they showed up so late.

What is strange about my configuration? Just that I enabled the RCU boost and other new RCU features?

Surely it's not that I'm just doing lots of mounts and unmounts and file-io with lots of processes....

And I was posting lockdep warnings on this since -rc1 or so, though granted I was running a few extra patches and having other bugs at the same time....

RCU boost is new and counts as strange for another few days at least.

Quoted 1 line · Paul E. McKenney

RCU boost is new and counts as strange for another few days at least.

I'd rather prefer a oneliner patch that turns it off in the Kconfig for now.

We are awfully late in the -rc cycle and this diffstat:

 b/include/linux/sched.h   |    3 ++
 b/kernel/rcutree_plugin.h |    6 +++--
 b/kernel/sched.c          |   44 ++++++++++++++++++++++++++++++++++-----
 b/kernel/signal.c         |   19 +++++++++++------
 b/kernel/softirq.c        |   12 +++++++++-
 kernel/rcutree_plugin.h   |   51 +++++++++++++++++++++++++++++++++-------------
 6 files changed, 105 insertions(+), 30 deletions(-)

looks very scary to me.

Quoted 17 lines · Ingo Molnar

Quoted 1 line · Paul E. McKenney

RCU boost is new and counts as strange for another few days at least.

I'd rather prefer a oneliner patch that turns it off in the Kconfig for now.

We are awfully late in the -rc cycle and this diffstat:

 b/include/linux/sched.h   |    3 ++
 b/kernel/rcutree_plugin.h |    6 +++--
 b/kernel/sched.c          |   44 ++++++++++++++++++++++++++++++++++-----
 b/kernel/signal.c         |   19 +++++++++++------
 b/kernel/softirq.c        |   12 +++++++++-
 kernel/rcutree_plugin.h   |   51 +++++++++++++++++++++++++++++++++-------------
 6 files changed, 105 insertions(+), 30 deletions(-)

Quoted 1 line

looks very scary to me.

A lot of that is also relevant to !BOOST.

This can interact badly with the recently
added RCU read-side critical sections in the scheduler that have either the runqueue or the priority-inheritance locks held, especially when interrupts occur towards the end of __rcu_read_unlock().

Quoted 4 lines · Paul E. McKenney

This can interact badly with the recently
added RCU read-side critical sections in the scheduler that have either the runqueue or the priority-inheritance locks held, especially when interrupts occur towards the end of __rcu_read_unlock().

Right, so while I recently added a lot more, there have been rcu usage sites under rq->lock for a long while, see commits

a18b83b7ef ("cpuacct: make cpuacct hierarchy walk in cpuacct_charge() safe when rcupreempt is used -v2") -- March 2009.

f3b577dec1 ("rcu: apply RCU protection to wake_affine()") -- Jun 2010

b0a0f667 ("sched: suppress RCU lockdep splat in task_fork_fair") -- Oct 2010

So I'm not quite seeing how the problems we're hitting now are new.

Quoted 3 lines

 6 files changed, 105 insertions(+), 30 deletions(-)

looks very scary to me.

A lot of that is also relevant to !BOOST.

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines

 6 files changed, 105 insertions(+), 30 deletions(-)

looks very scary to me.

A lot of that is also relevant to !BOOST.

Can we limit this somehow? Or split it up? Which part of this is "fix new BOOST features, not ever even executed without BOOST", and which part of this is "fixes core stuff"?

I *really* hate the timing of this. The code that is only impacted by BOOST I cannot find it in myself to care about, and I'd be willing to consider it basically EXPERIMENTAL and just pulling it.

IOW, is the core non-boost fix just a few obvious oneliners?

The "it all broke completely" in previous version of this also doesn't make me get all the warm fuzzies. Which all makes me go "what is minimal and really really SAFE?"

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

 b/kernel/rcutree_plugin.h |   12 ++++++------
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

 b/kernel/rcutree_plugin.h |   12 ++++++------
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Quoted 14 lines · Ingo Molnar

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

 b/kernel/rcutree_plugin.h |   12 ++++++------
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

Quoted 7 lines

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Ok, i looked some more at the background and the symptoms that people are seeing: kernel crashes and lockups. I think we want these problems fixed in v3.0, even if it was the recent introduction of RCU_BOOST that made it really prominent.

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Linus, would that approach be fine with you? I'll send an RFC pull request for the 6 patches as a reply to this mail, in a couple of minutes.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 30 lines · Ingo Molnar

Quoted 14 lines · Ingo Molnar

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

  b/kernel/rcutree_plugin.h |   12 ++++++------
  b/kernel/softirq.c        |   12 ++++++++++--
  kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
  3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

Quoted 7 lines

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Ok, i looked some more at the background and the symptoms that people are seeing: kernel crashes and lockups. I think we want these problems fixed in v3.0, even if it was the recent introduction of RCU_BOOST that made it really prominent.

Quoted 4 lines

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

RCU_BOOST is currently default=n. Is that sufficient? If not, one

Quoted 36 lines · Paul E. McKenney

Quoted 34 lines · Ingo Molnar

Quoted 14 lines · Ingo Molnar

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

 b/kernel/rcutree_plugin.h |   12 ++++++------
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

Quoted 7 lines

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Ok, i looked some more at the background and the symptoms that people are seeing: kernel crashes and lockups. I think we want these problems fixed in v3.0, even if it was the recent introduction of RCU_BOOST that made it really prominent.

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Quoted 1 line

RCU_BOOST is currently default=n. Is that sufficient? If not, one

Not if it remains broken I think..unless you put it under CONFIG_BROKEN or something. Otherwise, folks are liable to turn it on and not realize it's the cause of subtle bugs.

For what it's worth, my tests have been running clean for around 2 hours, so the full set of fixes with RCU_BOOST appears good, so far. I'll let it continue to run
at least overnight to make sure I'm not just getting lucky...

Quoted 37 lines · Ben Greear

Quoted 36 lines · Paul E. McKenney

Quoted 34 lines · Ingo Molnar

Quoted 14 lines · Ingo Molnar

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

  b/kernel/rcutree_plugin.h |   12 ++++++------
  b/kernel/softirq.c        |   12 ++++++++++--
  kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
  3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

Quoted 7 lines

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Ok, i looked some more at the background and the symptoms that people are seeing: kernel crashes and lockups. I think we want these problems fixed in v3.0, even if it was the recent introduction of RCU_BOOST that made it really prominent.

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Quoted 5 lines

Quoted 1 line

RCU_BOOST is currently default=n. Is that sufficient? If not, one

Not if it remains broken I think..unless you put it under CONFIG_BROKEN or something. Otherwise, folks are liable to turn it on and not realize it's the cause of subtle bugs.

Good point, I could easily add "depends on BROKEN".

Quoted 3 lines

For what it's worth, my tests have been running clean for around 2 hours, so the full set of fixes with RCU_BOOST appears good, so far. I'll let it continue to run
at least overnight to make sure I'm not just getting lucky...

Continuing to think good thoughts... ;-)

Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 46 lines · Paul E. McKenney

Quoted 37 lines · Ben Greear

Quoted 36 lines · Paul E. McKenney

Quoted 34 lines · Ingo Molnar

Quoted 14 lines · Ingo Molnar

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

 b/kernel/rcutree_plugin.h |   12 ++++++------
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

Quoted 7 lines

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Ok, i looked some more at the background and the symptoms that people are seeing: kernel crashes and lockups. I think we want these problems fixed in v3.0, even if it was the recent introduction of RCU_BOOST that made it really prominent.

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Quoted 5 lines

Quoted 1 line

RCU_BOOST is currently default=n. Is that sufficient? If not, one

Not if it remains broken I think..unless you put it under CONFIG_BROKEN or something. Otherwise, folks are liable to turn it on and not realize it's the cause of subtle bugs.

Good point, I could easily add "depends on BROKEN".

Quoted 5 lines

Quoted 3 lines

For what it's worth, my tests have been running clean for around 2 hours, so the full set of fixes with RCU_BOOST appears good, so far. I'll let it continue to run
at least overnight to make sure I'm not just getting lucky...

Continuing to think good thoughts... ;-)

My test is still going strong with no splats or errors, so I think that nailed the problems I was seeing...

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Linus, would that approach be fine with you? I'll send an RFC pull request for the 6 patches as a reply to this mail, in a couple of minutes.

Quoted 8 lines · Ingo Molnar

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Linus, would that approach be fine with you? I'll send an RFC pull request for the 6 patches as a reply to this mail, in a couple of minutes.

here it is:

Linus,

Please pull the latest core-urgent-for-linus git tree from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core-urgent-for-linus

In addition to the testing Paul & co has performed i have boot tested x86 defconfig/allno/allyes/allmod and a fair number of randconfigs, and i build tested a dozen architecture defconfigs, amongst them the key ones.

( We could also mark RCU_BOOST in init/Kconfig as EXPERIMENTAL, to
further de-risk it - but that change is not part of this pull request. )

Thanks,

Ingo

------------------>
Paul E. McKenney (5):
rcu: decrease rcu_report_exp_rnp coupling with scheduler
rcu: Fix RCU_BOOST race handling current->rcu_read_unlock_special
rcu: Streamline code produced by __rcu_read_unlock()
rcu: protect __rcu_read_unlock() against scheduler-using irq handlers
signal: align __lock_task_sighand() irq disabling and RCU

Peter Zijlstra (2):
sched: Add irq_{enter,exit}() to scheduler_ipi() softirq,rcu: Inform RCU of irq_exit() activity

 include/linux/sched.h   |    3 ++
 kernel/rcutree_plugin.h |   53 ++++++++++++++++++++++++++++++++++------------
 kernel/sched.c          |   44 +++++++++++++++++++++++++++++++++-----
 kernel/signal.c         |   19 +++++++++++-----
 kernel/softirq.c        |   12 ++++++++-
 5 files changed, 103 insertions(+), 28 deletions(-)

include/linux/sched.h +3 -0

index 496770a..76676a4 100644
@@ -1254,6 +1254,9 @@ struct task_struct {
 #ifdef CONFIG_PREEMPT_RCU
 	int rcu_read_lock_nesting;
 	char rcu_read_unlock_special;
+#if defined(CONFIG_RCU_BOOST) && defined(CONFIG_TREE_PREEMPT_RCU)
+	int rcu_boosted;
+#endif /* #if defined(CONFIG_RCU_BOOST) && defined(CONFIG_TREE_PREEMPT_RCU) */
 	struct list_head rcu_node_entry;
 #endif /* #ifdef CONFIG_PREEMPT_RCU */
 #ifdef CONFIG_TREE_PREEMPT_RCU

kernel/rcutree_plugin.h +39 -14

index 75113cb..8aafbb8 100644
@@ -68,6 +68,7 @@ struct rcu_state rcu_preempt_state = RCU_STATE_INITIALIZER(rcu_preempt_state);
 DEFINE_PER_CPU(struct rcu_data, rcu_preempt_data);
 static struct rcu_state *rcu_state = &rcu_preempt_state;
 
+static void rcu_read_unlock_special(struct task_struct *t);
 static int rcu_preempted_readers_exp(struct rcu_node *rnp);
 
 /*

@@ -147,7 +148,7 @@ static void rcu_preempt_note_context_switch(int cpu)
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
 
-	if (t->rcu_read_lock_nesting &&
+	if (t->rcu_read_lock_nesting > 0 &&
 	    (t->rcu_read_unlock_special & RCU_READ_UNLOCK_BLOCKED) == 0) {
 
 		/* Possibly blocking in an RCU read-side critical section. */

@@ -190,6 +191,14 @@ static void rcu_preempt_note_context_switch(int cpu)
 				rnp->gp_tasks = &t->rcu_node_entry;
 		}
 		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+	} else if (t->rcu_read_lock_nesting < 0 &&
+		   t->rcu_read_unlock_special) {
+
+		/*
+		 * Complete exit from RCU read-side critical section on
+		 * behalf of preempted instance of __rcu_read_unlock().
+		 */
+		rcu_read_unlock_special(t);
 	}
 
 	/*

@@ -284,7 +293,7 @@ static struct list_head *rcu_next_node_entry(struct task_struct *t,
  * notify RCU core processing or task having blocked during the RCU
  * read-side critical section.
  */
-static void rcu_read_unlock_special(struct task_struct *t)
+static noinline void rcu_read_unlock_special(struct task_struct *t)
 {
 	int empty;
 	int empty_exp;

@@ -309,7 +318,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
 	}
 
 	/* Hardware IRQ handlers cannot block. */
-	if (in_irq()) {
+	if (in_irq() || in_serving_softirq()) {
 		local_irq_restore(flags);
 		return;
 	}

@@ -342,6 +351,11 @@ static void rcu_read_unlock_special(struct task_struct *t)
 #ifdef CONFIG_RCU_BOOST
 		if (&t->rcu_node_entry == rnp->boost_tasks)
 			rnp->boost_tasks = np;
+		/* Snapshot and clear ->rcu_boosted with rcu_node lock held. */
+		if (t->rcu_boosted) {
+			special |= RCU_READ_UNLOCK_BOOSTED;
+			t->rcu_boosted = 0;
+		}
 #endif /* #ifdef CONFIG_RCU_BOOST */
 		t->rcu_blocked_node = NULL;
 

@@ -358,7 +372,6 @@ static void rcu_read_unlock_special(struct task_struct *t)
 #ifdef CONFIG_RCU_BOOST
 		/* Unboost if we were boosted. */
 		if (special & RCU_READ_UNLOCK_BOOSTED) {
-			t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BOOSTED;
 			rt_mutex_unlock(t->rcu_boost_mutex);
 			t->rcu_boost_mutex = NULL;
 		}

@@ -387,13 +400,22 @@ void __rcu_read_unlock(void)
 	struct task_struct *t = current;
 
 	barrier();  /* needed if we ever invoke rcu_read_unlock in rcutree.c */
-	--t->rcu_read_lock_nesting;
-	barrier();  /* decrement before load of ->rcu_read_unlock_special */
-	if (t->rcu_read_lock_nesting == 0 &&
-	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
-		rcu_read_unlock_special(t);
+	if (t->rcu_read_lock_nesting != 1)
+		--t->rcu_read_lock_nesting;
+	else {
+		t->rcu_read_lock_nesting = INT_MIN;
+		barrier();  /* assign before ->rcu_read_unlock_special load */
+		if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
+			rcu_read_unlock_special(t);
+		barrier();  /* ->rcu_read_unlock_special load before assign */
+		t->rcu_read_lock_nesting = 0;
+	}
 #ifdef CONFIG_PROVE_LOCKING
-	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
+	{
+		int rrln = ACCESS_ONCE(t->rcu_read_lock_nesting);
+
+		WARN_ON_ONCE(rrln < 0 && rrln > INT_MIN / 2);
+	}
 #endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);

@@ -589,7 +611,8 @@ static void rcu_preempt_check_callbacks(int cpu)
 		rcu_preempt_qs(cpu);
 		return;
 	}
-	if (per_cpu(rcu_preempt_data, cpu).qs_pending)
+	if (t->rcu_read_lock_nesting > 0 &&
+	    per_cpu(rcu_preempt_data, cpu).qs_pending)
 		t->rcu_read_unlock_special |= RCU_READ_UNLOCK_NEED_QS;
 }
 

@@ -695,9 +718,12 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
 
 	raw_spin_lock_irqsave(&rnp->lock, flags);
 	for (;;) {
-		if (!sync_rcu_preempt_exp_done(rnp))
+		if (!sync_rcu_preempt_exp_done(rnp)) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			break;
+		}
 		if (rnp->parent == NULL) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			wake_up(&sync_rcu_preempt_exp_wq);
 			break;
 		}

@@ -707,7 +733,6 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
 		raw_spin_lock(&rnp->lock); /* irqs already disabled */
 		rnp->expmask &= ~mask;
 	}
-	raw_spin_unlock_irqrestore(&rnp->lock, flags);
 }
 
 /*

@@ -1174,7 +1199,7 @@ static int rcu_boost(struct rcu_node *rnp)
 	t = container_of(tb, struct task_struct, rcu_node_entry);
 	rt_mutex_init_proxy_locked(&mtx, t);
 	t->rcu_boost_mutex = &mtx;
-	t->rcu_read_unlock_special |= RCU_READ_UNLOCK_BOOSTED;
+	t->rcu_boosted = 1;
 	raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	rt_mutex_lock(&mtx);  /* Side effect: boosts task t's priority. */
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */

kernel/sched.c +38 -6

index 3dc716f..31e92ae 100644
@@ -2544,13 +2544,9 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_do_pending(struct task_struct *list)
 {
 	struct rq *rq = this_rq();
-	struct task_struct *list = xchg(&rq->wake_list, NULL);
-
-	if (!list)
-		return;
 
 	raw_spin_lock(&rq->lock);
 

@@ -2563,9 +2559,45 @@ static void sched_ttwu_pending(void)
 	raw_spin_unlock(&rq->lock);
 }
 
+#ifdef CONFIG_HOTPLUG_CPU
+
+static void sched_ttwu_pending(void)
+{
+	struct rq *rq = this_rq();
+	struct task_struct *list = xchg(&rq->wake_list, NULL);
+
+	if (!list)
+		return;
+
+	sched_ttwu_do_pending(list);
+}
+
+#endif /* CONFIG_HOTPLUG_CPU */
+
 void scheduler_ipi(void)
 {
-	sched_ttwu_pending();
+	struct rq *rq = this_rq();
+	struct task_struct *list = xchg(&rq->wake_list, NULL);
+
+	if (!list)
+		return;
+
+	/*
+	 * Not all reschedule IPI handlers call irq_enter/irq_exit, since
+	 * traditionally all their work was done from the interrupt return
+	 * path. Now that we actually do some work, we need to make sure
+	 * we do call them.
+	 *
+	 * Some archs already do call them, luckily irq_enter/exit nest
+	 * properly.
+	 *
+	 * Arguably we should visit all archs and update all handlers,
+	 * however a fair share of IPIs are still resched only so this would
+	 * somewhat pessimize the simple resched case.
+	 */
+	irq_enter();
+	sched_ttwu_do_pending(list);
+	irq_exit();
 }
 
 static void ttwu_queue_remote(struct task_struct *p, int cpu)

kernel/signal.c +13 -6

index ff76786..415d85d 100644
@@ -1178,18 +1178,25 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 {
 	struct sighand_struct *sighand;
 
-	rcu_read_lock();
 	for (;;) {
+		local_irq_save(*flags);
+		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);
-		if (unlikely(sighand == NULL))
+		if (unlikely(sighand == NULL)) {
+			rcu_read_unlock();
+			local_irq_restore(*flags);
 			break;
+		}
 
-		spin_lock_irqsave(&sighand->siglock, *flags);
-		if (likely(sighand == tsk->sighand))
+		spin_lock(&sighand->siglock);
+		if (likely(sighand == tsk->sighand)) {
+			rcu_read_unlock();
 			break;
-		spin_unlock_irqrestore(&sighand->siglock, *flags);
+		}
+		spin_unlock(&sighand->siglock);
+		rcu_read_unlock();
+		local_irq_restore(*flags);
 	}
-	rcu_read_unlock();
 
 	return sighand;
 }

kernel/softirq.c +10 -3

index 40cf63d..fca82c3 100644
@@ -315,16 +315,24 @@ static inline void invoke_softirq(void)
 {
 	if (!force_irqthreads)
 		__do_softirq();
-	else
+	else {
+		__local_bh_disable((unsigned long)__builtin_return_address(0),
+				SOFTIRQ_OFFSET);
 		wakeup_softirqd();
+		__local_bh_enable(SOFTIRQ_OFFSET);
+	}
 }
 #else
 static inline void invoke_softirq(void)
 {
 	if (!force_irqthreads)
 		do_softirq();
-	else
+	else {
+		__local_bh_disable((unsigned long)__builtin_return_address(0),
+				SOFTIRQ_OFFSET);
 		wakeup_softirqd();
+		__local_bh_enable(SOFTIRQ_OFFSET);
+	}
 }
 #endif
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 18 lines · On Wednesday 20 July 2011 17:04:26 Ingo Molnar wrote:

Quoted 8 lines · Ingo Molnar

Having put some testing into your rcu/urgent branch today i'd feel more comfortable with taking this plus perhaps an RCU_BOOST disabling patch. That makes it all fundamentally tested by a number of people (including those who reported/reproduced the problems).

Linus, would that approach be fine with you? I'll send an RFC pull request for the 6 patches as a reply to this mail, in a couple of minutes.

here it is:

Linus,

Please pull the latest core-urgent-for-linus git tree from:

Quoted 1 line

git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core-urgent-for-linus

I've been running on the previous version of this tree along with a fix for the first patch. An additional patch was added this morning to prevent an invalid warning from triggering. Aside from that warning the system was stable with no other unexpected output in dmesg.

Resetting to master and pulling the above tree gives me a clean boot. On atleast one box that was seeing problems this seems too be making things better (boost and threadirqs enabled).

Thanks
Ed

Quoted 331 lines

In addition to the testing Paul & co has performed i have boot tested x86 defconfig/allno/allyes/allmod and a fair number of randconfigs, and i build tested a dozen architecture defconfigs, amongst them the key ones.

( We could also mark RCU_BOOST in init/Kconfig as EXPERIMENTAL, to
further de-risk it - but that change is not part of this pull request. )

Thanks,

Ingo

------------------>
Paul E. McKenney (5):
rcu: decrease rcu_report_exp_rnp coupling with scheduler
rcu: Fix RCU_BOOST race handling current->rcu_read_unlock_special
rcu: Streamline code produced by __rcu_read_unlock()
rcu: protect __rcu_read_unlock() against scheduler-using irq handlers
signal: align __lock_task_sighand() irq disabling and RCU

Peter Zijlstra (2):
sched: Add irq_{enter,exit}() to scheduler_ipi() softirq,rcu: Inform RCU of irq_exit() activity

 include/linux/sched.h   |    3 ++
 kernel/rcutree_plugin.h |   53 ++++++++++++++++++++++++++++++++++------------
 kernel/sched.c          |   44 +++++++++++++++++++++++++++++++++-----
 kernel/signal.c         |   19 +++++++++++-----
 kernel/softirq.c        |   12 ++++++++-
 5 files changed, 103 insertions(+), 28 deletions(-)

include/linux/sched.h +3 -0

index 496770a..76676a4 100644
@@ -1254,6 +1254,9 @@ struct task_struct {
 #ifdef CONFIG_PREEMPT_RCU
 	int rcu_read_lock_nesting;
 	char rcu_read_unlock_special;
+#if defined(CONFIG_RCU_BOOST) && defined(CONFIG_TREE_PREEMPT_RCU)
+	int rcu_boosted;
+#endif /* #if defined(CONFIG_RCU_BOOST) && defined(CONFIG_TREE_PREEMPT_RCU) */
 	struct list_head rcu_node_entry;
 #endif /* #ifdef CONFIG_PREEMPT_RCU */
 #ifdef CONFIG_TREE_PREEMPT_RCU

kernel/rcutree_plugin.h +39 -14

index 75113cb..8aafbb8 100644
@@ -68,6 +68,7 @@ struct rcu_state rcu_preempt_state = RCU_STATE_INITIALIZER(rcu_preempt_state);
 DEFINE_PER_CPU(struct rcu_data, rcu_preempt_data);
 static struct rcu_state *rcu_state = &rcu_preempt_state;
 
+static void rcu_read_unlock_special(struct task_struct *t);
 static int rcu_preempted_readers_exp(struct rcu_node *rnp);
 
 /*

@@ -147,7 +148,7 @@ static void rcu_preempt_note_context_switch(int cpu)
 	struct rcu_data *rdp;
 	struct rcu_node *rnp;
 
-	if (t->rcu_read_lock_nesting &&
+	if (t->rcu_read_lock_nesting > 0 &&
 	    (t->rcu_read_unlock_special & RCU_READ_UNLOCK_BLOCKED) == 0) {
 
 		/* Possibly blocking in an RCU read-side critical section. */

@@ -190,6 +191,14 @@ static void rcu_preempt_note_context_switch(int cpu)
 				rnp->gp_tasks = &t->rcu_node_entry;
 		}
 		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+	} else if (t->rcu_read_lock_nesting < 0 &&
+		   t->rcu_read_unlock_special) {
+
+		/*
+		 * Complete exit from RCU read-side critical section on
+		 * behalf of preempted instance of __rcu_read_unlock().
+		 */
+		rcu_read_unlock_special(t);
 	}
 
 	/*

@@ -284,7 +293,7 @@ static struct list_head *rcu_next_node_entry(struct task_struct *t,
  * notify RCU core processing or task having blocked during the RCU
  * read-side critical section.
  */
-static void rcu_read_unlock_special(struct task_struct *t)
+static noinline void rcu_read_unlock_special(struct task_struct *t)
 {
 	int empty;
 	int empty_exp;

@@ -309,7 +318,7 @@ static void rcu_read_unlock_special(struct task_struct *t)
 	}
 
 	/* Hardware IRQ handlers cannot block. */
-	if (in_irq()) {
+	if (in_irq() || in_serving_softirq()) {
 		local_irq_restore(flags);
 		return;
 	}

@@ -342,6 +351,11 @@ static void rcu_read_unlock_special(struct task_struct *t)
 #ifdef CONFIG_RCU_BOOST
 		if (&t->rcu_node_entry == rnp->boost_tasks)
 			rnp->boost_tasks = np;
+		/* Snapshot and clear ->rcu_boosted with rcu_node lock held. */
+		if (t->rcu_boosted) {
+			special |= RCU_READ_UNLOCK_BOOSTED;
+			t->rcu_boosted = 0;
+		}
 #endif /* #ifdef CONFIG_RCU_BOOST */
 		t->rcu_blocked_node = NULL;
 

@@ -358,7 +372,6 @@ static void rcu_read_unlock_special(struct task_struct *t)
 #ifdef CONFIG_RCU_BOOST
 		/* Unboost if we were boosted. */
 		if (special & RCU_READ_UNLOCK_BOOSTED) {
-			t->rcu_read_unlock_special &= ~RCU_READ_UNLOCK_BOOSTED;
 			rt_mutex_unlock(t->rcu_boost_mutex);
 			t->rcu_boost_mutex = NULL;
 		}

@@ -387,13 +400,22 @@ void __rcu_read_unlock(void)
 	struct task_struct *t = current;
 
 	barrier();  /* needed if we ever invoke rcu_read_unlock in rcutree.c */
-	--t->rcu_read_lock_nesting;
-	barrier();  /* decrement before load of ->rcu_read_unlock_special */
-	if (t->rcu_read_lock_nesting == 0 &&
-	    unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
-		rcu_read_unlock_special(t);
+	if (t->rcu_read_lock_nesting != 1)
+		--t->rcu_read_lock_nesting;
+	else {
+		t->rcu_read_lock_nesting = INT_MIN;
+		barrier();  /* assign before ->rcu_read_unlock_special load */
+		if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
+			rcu_read_unlock_special(t);
+		barrier();  /* ->rcu_read_unlock_special load before assign */
+		t->rcu_read_lock_nesting = 0;
+	}
 #ifdef CONFIG_PROVE_LOCKING
-	WARN_ON_ONCE(ACCESS_ONCE(t->rcu_read_lock_nesting) < 0);
+	{
+		int rrln = ACCESS_ONCE(t->rcu_read_lock_nesting);
+
+		WARN_ON_ONCE(rrln < 0 && rrln > INT_MIN / 2);
+	}
 #endif /* #ifdef CONFIG_PROVE_LOCKING */
 }
 EXPORT_SYMBOL_GPL(__rcu_read_unlock);

@@ -589,7 +611,8 @@ static void rcu_preempt_check_callbacks(int cpu)
 		rcu_preempt_qs(cpu);
 		return;
 	}
-	if (per_cpu(rcu_preempt_data, cpu).qs_pending)
+	if (t->rcu_read_lock_nesting > 0 &&
+	    per_cpu(rcu_preempt_data, cpu).qs_pending)
 		t->rcu_read_unlock_special |= RCU_READ_UNLOCK_NEED_QS;
 }
 

@@ -695,9 +718,12 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
 
 	raw_spin_lock_irqsave(&rnp->lock, flags);
 	for (;;) {
-		if (!sync_rcu_preempt_exp_done(rnp))
+		if (!sync_rcu_preempt_exp_done(rnp)) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			break;
+		}
 		if (rnp->parent == NULL) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			wake_up(&sync_rcu_preempt_exp_wq);
 			break;
 		}

@@ -707,7 +733,6 @@ static void rcu_report_exp_rnp(struct rcu_state *rsp, struct rcu_node *rnp)
 		raw_spin_lock(&rnp->lock); /* irqs already disabled */
 		rnp->expmask &= ~mask;
 	}
-	raw_spin_unlock_irqrestore(&rnp->lock, flags);
 }
 
 /*

@@ -1174,7 +1199,7 @@ static int rcu_boost(struct rcu_node *rnp)
 	t = container_of(tb, struct task_struct, rcu_node_entry);
 	rt_mutex_init_proxy_locked(&mtx, t);
 	t->rcu_boost_mutex = &mtx;
-	t->rcu_read_unlock_special |= RCU_READ_UNLOCK_BOOSTED;
+	t->rcu_boosted = 1;
 	raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	rt_mutex_lock(&mtx);  /* Side effect: boosts task t's priority. */
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */

kernel/sched.c +38 -6

index 3dc716f..31e92ae 100644
@@ -2544,13 +2544,9 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_do_pending(struct task_struct *list)
 {
 	struct rq *rq = this_rq();
-	struct task_struct *list = xchg(&rq->wake_list, NULL);
-
-	if (!list)
-		return;
 
 	raw_spin_lock(&rq->lock);
 

@@ -2563,9 +2559,45 @@ static void sched_ttwu_pending(void)
 	raw_spin_unlock(&rq->lock);
 }
 
+#ifdef CONFIG_HOTPLUG_CPU
+
+static void sched_ttwu_pending(void)
+{
+	struct rq *rq = this_rq();
+	struct task_struct *list = xchg(&rq->wake_list, NULL);
+
+	if (!list)
+		return;
+
+	sched_ttwu_do_pending(list);
+}
+
+#endif /* CONFIG_HOTPLUG_CPU */
+
 void scheduler_ipi(void)
 {
-	sched_ttwu_pending();
+	struct rq *rq = this_rq();
+	struct task_struct *list = xchg(&rq->wake_list, NULL);
+
+	if (!list)
+		return;
+
+	/*
+	 * Not all reschedule IPI handlers call irq_enter/irq_exit, since
+	 * traditionally all their work was done from the interrupt return
+	 * path. Now that we actually do some work, we need to make sure
+	 * we do call them.
+	 *
+	 * Some archs already do call them, luckily irq_enter/exit nest
+	 * properly.
+	 *
+	 * Arguably we should visit all archs and update all handlers,
+	 * however a fair share of IPIs are still resched only so this would
+	 * somewhat pessimize the simple resched case.
+	 */
+	irq_enter();
+	sched_ttwu_do_pending(list);
+	irq_exit();
 }
 
 static void ttwu_queue_remote(struct task_struct *p, int cpu)

kernel/signal.c +13 -6

index ff76786..415d85d 100644
@@ -1178,18 +1178,25 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
 {
 	struct sighand_struct *sighand;
 
-	rcu_read_lock();
 	for (;;) {
+		local_irq_save(*flags);
+		rcu_read_lock();
 		sighand = rcu_dereference(tsk->sighand);
-		if (unlikely(sighand == NULL))
+		if (unlikely(sighand == NULL)) {
+			rcu_read_unlock();
+			local_irq_restore(*flags);
 			break;
+		}
 
-		spin_lock_irqsave(&sighand->siglock, *flags);
-		if (likely(sighand == tsk->sighand))
+		spin_lock(&sighand->siglock);
+		if (likely(sighand == tsk->sighand)) {
+			rcu_read_unlock();
 			break;
-		spin_unlock_irqrestore(&sighand->siglock, *flags);
+		}
+		spin_unlock(&sighand->siglock);
+		rcu_read_unlock();
+		local_irq_restore(*flags);
 	}
-	rcu_read_unlock();
 
 	return sighand;
 }

kernel/softirq.c +10 -3

index 40cf63d..fca82c3 100644
@@ -315,16 +315,24 @@ static inline void invoke_softirq(void)
 {
 	if (!force_irqthreads)
 		__do_softirq();
-	else
+	else {
+		__local_bh_disable((unsigned long)__builtin_return_address(0),
+				SOFTIRQ_OFFSET);
 		wakeup_softirqd();
+		__local_bh_enable(SOFTIRQ_OFFSET);
+	}
 }
 #else
 static inline void invoke_softirq(void)
 {
 	if (!force_irqthreads)
 		do_softirq();
-	else
+	else {
+		__local_bh_disable((unsigned long)__builtin_return_address(0),
+				SOFTIRQ_OFFSET);
 		wakeup_softirqd();
+		__local_bh_enable(SOFTIRQ_OFFSET);
+	}
 }
 #endif
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 8 lines · Paul E. McKenney

If my guess is correct, then the minimal non-RCU_BOOST fix is #4 (which drags along #3) and #6. Which are not one-liners, but somewhat smaller:

 b/kernel/rcutree_plugin.h |   12 ++++++------
 b/kernel/softirq.c        |   12 ++++++++++--
 kernel/rcutree_plugin.h   |   31 +++++++++++++++++++++++++------
 3 files changed, 41 insertions(+), 14 deletions(-)

That's half the patch size and half the patch count.

PeterZ's question is relevant: since we apparently had similar bugs in v2.6.39 as well, what changed in v3.0 that makes them so urgent to fix?

If it's just better instrumentation that proves them better then i'd suggest fixing this in v3.1 and not risking v3.0 with an unintended side effect.

Peter, does #4 (protect __rcu_read_unlock() against scheduler-using irq handlers) remove the need for #5 (Add irq_{enter,exit}() to scheduler_ipi()) and #6 (Inform RCU of irq_exit() activity)? My guess is "no" for #5 and "yes" for #6.

Quoted 4 lines · Paul E. McKenney

Peter, does #4 (protect __rcu_read_unlock() against scheduler-using irq handlers) remove the need for #5 (Add irq_{enter,exit}() to scheduler_ipi()) and #6 (Inform RCU of irq_exit() activity)? My guess is "no" for #5 and "yes" for #6.

More or less, we want to keep #5 for it does more than just fix RCU, but yeah, I _think_ #4 obsoletes the direct need for #6.

Quoted 7 lines · On Tuesday 19 July 2011 20:17:38 Paul E. McKenney wrote:

Paul,

Two things to add. Its possible an eariler error is missing from the log below. My serial console was missing entries from before '0 and 40ish' and I did think I saw a bunch of FFFFFFFs scroll by eariler...

Second, booting with threadirqs and boost enabled in .config with patches 2,5,6 is ok so far.

Patches 3 and 4 resulted in failures? (Patch 1 certainly did.)

Given some common flag combinations, particularly -Os, gcc will inline rcu_read_unlock_special() despite its being in an unlikely() clause. Use noinline to prohibit this misoptimization.

Quoted 3 lines · Paul E. McKenney

Given some common flag combinations, particularly -Os, gcc will inline rcu_read_unlock_special() despite its being in an unlikely() clause. Use noinline to prohibit this misoptimization.

Btw, I suspect that we should at least look at what it would mean if we make the rcu_read_lock_nesting and the preempt counters both be per-cpu variables instead of making them per-thread/process counters.

Then, when we switch threads, we'd just save/restore them from the process register save area.

There's a lot of critical code sequences (spin-lock/unlock, rcu read-lock/unlock) that currently fetches the thread/process pointer only to then offset it and increment the count. I get the strong feeling that code generation could be improved and we could avoid one level of indirection by just making it a per-thread counter.

For example, instead of __rcu_read_lock: looking like this (and being an external function, partly because of header file dependencies on the data structures involved):

  push   %rbp
  mov    %rsp,%rbp
  mov    %gs:0xb580,%rax
  incl   0x100(%rax)

leaveq
retq

it should inline to just something like

incl %gs:0x100

instead. Same for the preempt counter.

Of course, it would need to involve making sure that we pick a good cacheline etc that is already always dirty. But other than that, is there any real downside?