lockdep circular locking error (rcu_node_level_0 vs rq->lock)

I was doing an install in a kvm guest, which wedged itself at the end. This was in the host dmesg.

[ INFO: possible circular locking dependency detected ]
3.0.0-rc6+ #91
-------------------------------------------------------
libvirtd/5720 is trying to acquire lock:
(rcu_node_level_0){..-.-.}, at: [<ffffffff814c6c12>] rcu_report_unblock_qs_rnp.part.5+0x3f/0x60

but task is already holding lock:
(&rq->lock){-.-.-.}, at: [<ffffffff8105408e>] sched_ttwu_pending+0x39/0x5b

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (&rq->lock){-.-.-.}:

       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf0ab>] _raw_spin_lock+0x40/0x73
       [<ffffffff8104663a>] __task_rq_lock+0x5e/0x8b
       [<ffffffff8105506d>] wake_up_new_task+0x46/0x10d
       [<ffffffff8105a1c9>] do_fork+0x231/0x331
       [<ffffffff81010c80>] kernel_thread+0x75/0x77
       [<ffffffff814abe82>] rest_init+0x26/0xdc
       [<ffffffff81d3dbc2>] start_kernel+0x401/0x40c
       [<ffffffff81d3d2c4>] x86_64_start_reservations+0xaf/0xb3
       [<ffffffff81d3d3ca>] x86_64_start_kernel+0x102/0x111

-> #2 (&p->pi_lock){-.-.-.}:

       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf238>] _raw_spin_lock_irqsave+0x4f/0x89
       [<ffffffff81054e3d>] try_to_wake_up+0x2e/0x1db
       [<ffffffff81054ffc>] default_wake_function+0x12/0x14
       [<ffffffff81079008>] autoremove_wake_function+0x18/0x3d
       [<ffffffff81045010>] __wake_up_common+0x4d/0x83
       [<ffffffff8104634e>] __wake_up+0x39/0x4d
       [<ffffffff810c3cd6>] rcu_report_exp_rnp+0x52/0x8b
       [<ffffffff810c4f18>] __rcu_read_unlock+0x1d0/0x231
       [<ffffffff8115202a>] rcu_read_unlock+0x26/0x28
       [<ffffffff8115465d>] __d_lookup+0x103/0x115
       [<ffffffff8114b9eb>] walk_component+0x1b1/0x3af
       [<ffffffff8114bd8a>] link_path_walk+0x1a1/0x43b
       [<ffffffff8114c148>] path_lookupat+0x5a/0x2af
       [<ffffffff8114d222>] do_path_lookup+0x28/0x97
       [<ffffffff8114d658>] user_path_at+0x59/0x96
       [<ffffffff81145214>] sys_readlinkat+0x33/0x95
       [<ffffffff81145291>] sys_readlink+0x1b/0x1d
       [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

-> #1 (sync_rcu_preempt_exp_wq.lock){......}:

       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf238>] _raw_spin_lock_irqsave+0x4f/0x89
       [<ffffffff81046337>] __wake_up+0x22/0x4d
       [<ffffffff810c3cd6>] rcu_report_exp_rnp+0x52/0x8b
       [<ffffffff810c4f18>] __rcu_read_unlock+0x1d0/0x231
       [<ffffffff8115202a>] rcu_read_unlock+0x26/0x28
       [<ffffffff8115465d>] __d_lookup+0x103/0x115
       [<ffffffff8114b9eb>] walk_component+0x1b1/0x3af
       [<ffffffff8114bd8a>] link_path_walk+0x1a1/0x43b
       [<ffffffff8114c148>] path_lookupat+0x5a/0x2af
       [<ffffffff8114d222>] do_path_lookup+0x28/0x97
       [<ffffffff8114d658>] user_path_at+0x59/0x96
       [<ffffffff81145214>] sys_readlinkat+0x33/0x95
       [<ffffffff81145291>] sys_readlink+0x1b/0x1d
       [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

-> #0 (rcu_node_level_0){..-.-.}:

       [<ffffffff8108d7e5>] __lock_acquire+0xa2f/0xd0c
       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf0ab>] _raw_spin_lock+0x40/0x73
       [<ffffffff814c6c12>] rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
       [<ffffffff810c4ed6>] __rcu_read_unlock+0x18e/0x231
       [<ffffffff810463f4>] rcu_read_unlock+0x26/0x28
       [<ffffffff8104b6db>] cpuacct_charge+0x58/0x61
       [<ffffffff81052f18>] update_curr+0x107/0x134
       [<ffffffff8105349b>] check_preempt_wakeup+0xc9/0x1d0
       [<ffffffff81049775>] check_preempt_curr+0x2f/0x6e
       [<ffffffff81053f5e>] ttwu_do_wakeup+0x7b/0x111
       [<ffffffff81054050>] ttwu_do_activate.constprop.76+0x5c/0x61
       [<ffffffff8105409e>] sched_ttwu_pending+0x49/0x5b
       [<ffffffff810540be>] scheduler_ipi+0xe/0x10
       [<ffffffff810224f6>] smp_reschedule_interrupt+0x1b/0x1d
       [<ffffffff814d6b33>] reschedule_interrupt+0x13/0x20
       [<ffffffff813fcc18>] rcu_read_unlock+0x26/0x28
       [<ffffffff813fe308>] sock_def_readable+0x88/0x8d
       [<ffffffff81497760>] unix_stream_sendmsg+0x264/0x2ff
       [<ffffffff813f83c4>] sock_aio_write+0x112/0x126
       [<ffffffff8114093b>] do_sync_write+0xbf/0xff
       [<ffffffff81141012>] vfs_write+0xb6/0xf6
       [<ffffffff81141206>] sys_write+0x4d/0x74
       [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

other info that might help us debug this:

Chain exists of:
  rcu_node_level_0 --> &p->pi_lock --> &rq->lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&rq->lock);
                               lock(&p->pi_lock);
                               lock(&rq->lock);

lock(rcu_node_level_0);

*** DEADLOCK ***

1 lock held by libvirtd/5720:
#0: (&rq->lock){-.-.-.}, at: [<ffffffff8105408e>] sched_ttwu_pending+0x39/0x5b

stack backtrace:
Pid: 5720, comm: libvirtd Not tainted 3.0.0-rc6+ #91
Call Trace:
 <IRQ>  [<ffffffff814c51cf>] print_circular_bug+0x1f8/0x209
 [<ffffffff8108d7e5>] __lock_acquire+0xa2f/0xd0c
 [<ffffffff8107e905>] ? sched_clock_local+0x12/0x75
 [<ffffffff814c6c12>] ? rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
 [<ffffffff814c6c12>] ? rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff8108adab>] ? lock_release_holdtime.part.10+0x59/0x62
 [<ffffffff814cf0ab>] _raw_spin_lock+0x40/0x73
 [<ffffffff814c6c12>] ? rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff814cf855>] ? _raw_spin_unlock+0x47/0x54
 [<ffffffff814c6c12>] rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff810c4e00>] ? __rcu_read_unlock+0xb8/0x231
 [<ffffffff810c4ed6>] __rcu_read_unlock+0x18e/0x231
 [<ffffffff810463f4>] rcu_read_unlock+0x26/0x28
 [<ffffffff8104b6db>] cpuacct_charge+0x58/0x61
 [<ffffffff81052f18>] update_curr+0x107/0x134
 [<ffffffff8105349b>] check_preempt_wakeup+0xc9/0x1d0
 [<ffffffff81049775>] check_preempt_curr+0x2f/0x6e
 [<ffffffff81053f5e>] ttwu_do_wakeup+0x7b/0x111
 [<ffffffff81054050>] ttwu_do_activate.constprop.76+0x5c/0x61
 [<ffffffff8105409e>] sched_ttwu_pending+0x49/0x5b
 [<ffffffff810540be>] scheduler_ipi+0xe/0x10
 [<ffffffff810224f6>] smp_reschedule_interrupt+0x1b/0x1d
 [<ffffffff814d6b33>] reschedule_interrupt+0x13/0x20
 <EOI>  [<ffffffff8107e905>] ? sched_clock_local+0x12/0x75
 [<ffffffff810c4d91>] ? __rcu_read_unlock+0x49/0x231
 [<ffffffff8108dea5>] ? lock_release+0x1b1/0x1de
 [<ffffffff813fcc18>] rcu_read_unlock+0x26/0x28
 [<ffffffff813fe308>] sock_def_readable+0x88/0x8d
 [<ffffffff81497760>] unix_stream_sendmsg+0x264/0x2ff
 [<ffffffff813f83c4>] sock_aio_write+0x112/0x126
 [<ffffffff8121cd95>] ? inode_has_perm+0x6a/0x77

 [<ffffffff8114093b>] do_sync_write+0xbf/0xff
 [<ffffffff81219562>] ? security_file_permission+0x2e/0x33
 [<ffffffff81140d71>] ? rw_verify_area+0xb6/0xd3
 [<ffffffff81141012>] vfs_write+0xb6/0xf6
 [<ffffffff811426a0>] ? fget_light+0x97/0xa2
 [<ffffffff81141206>] sys_write+0x4d/0x74
 [<ffffffff81078f85>] ? remove_wait_queue+0x1a/0x3a
 [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

Quoted 148 lines · Dave Jones

I was doing an install in a kvm guest, which wedged itself at the end. This was in the host dmesg.

[ INFO: possible circular locking dependency detected ]
3.0.0-rc6+ #91
-------------------------------------------------------
libvirtd/5720 is trying to acquire lock:
(rcu_node_level_0){..-.-.}, at: [<ffffffff814c6c12>] rcu_report_unblock_qs_rnp.part.5+0x3f/0x60

but task is already holding lock:
(&rq->lock){-.-.-.}, at: [<ffffffff8105408e>] sched_ttwu_pending+0x39/0x5b

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (&rq->lock){-.-.-.}:

       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf0ab>] _raw_spin_lock+0x40/0x73
       [<ffffffff8104663a>] __task_rq_lock+0x5e/0x8b
       [<ffffffff8105506d>] wake_up_new_task+0x46/0x10d
       [<ffffffff8105a1c9>] do_fork+0x231/0x331
       [<ffffffff81010c80>] kernel_thread+0x75/0x77
       [<ffffffff814abe82>] rest_init+0x26/0xdc
       [<ffffffff81d3dbc2>] start_kernel+0x401/0x40c
       [<ffffffff81d3d2c4>] x86_64_start_reservations+0xaf/0xb3
       [<ffffffff81d3d3ca>] x86_64_start_kernel+0x102/0x111

-> #2 (&p->pi_lock){-.-.-.}:

       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf238>] _raw_spin_lock_irqsave+0x4f/0x89
       [<ffffffff81054e3d>] try_to_wake_up+0x2e/0x1db
       [<ffffffff81054ffc>] default_wake_function+0x12/0x14
       [<ffffffff81079008>] autoremove_wake_function+0x18/0x3d
       [<ffffffff81045010>] __wake_up_common+0x4d/0x83
       [<ffffffff8104634e>] __wake_up+0x39/0x4d
       [<ffffffff810c3cd6>] rcu_report_exp_rnp+0x52/0x8b
       [<ffffffff810c4f18>] __rcu_read_unlock+0x1d0/0x231
       [<ffffffff8115202a>] rcu_read_unlock+0x26/0x28
       [<ffffffff8115465d>] __d_lookup+0x103/0x115
       [<ffffffff8114b9eb>] walk_component+0x1b1/0x3af
       [<ffffffff8114bd8a>] link_path_walk+0x1a1/0x43b
       [<ffffffff8114c148>] path_lookupat+0x5a/0x2af
       [<ffffffff8114d222>] do_path_lookup+0x28/0x97
       [<ffffffff8114d658>] user_path_at+0x59/0x96
       [<ffffffff81145214>] sys_readlinkat+0x33/0x95
       [<ffffffff81145291>] sys_readlink+0x1b/0x1d
       [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

-> #1 (sync_rcu_preempt_exp_wq.lock){......}:

       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf238>] _raw_spin_lock_irqsave+0x4f/0x89
       [<ffffffff81046337>] __wake_up+0x22/0x4d
       [<ffffffff810c3cd6>] rcu_report_exp_rnp+0x52/0x8b
       [<ffffffff810c4f18>] __rcu_read_unlock+0x1d0/0x231
       [<ffffffff8115202a>] rcu_read_unlock+0x26/0x28
       [<ffffffff8115465d>] __d_lookup+0x103/0x115
       [<ffffffff8114b9eb>] walk_component+0x1b1/0x3af
       [<ffffffff8114bd8a>] link_path_walk+0x1a1/0x43b
       [<ffffffff8114c148>] path_lookupat+0x5a/0x2af
       [<ffffffff8114d222>] do_path_lookup+0x28/0x97
       [<ffffffff8114d658>] user_path_at+0x59/0x96
       [<ffffffff81145214>] sys_readlinkat+0x33/0x95
       [<ffffffff81145291>] sys_readlink+0x1b/0x1d
       [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

-> #0 (rcu_node_level_0){..-.-.}:

       [<ffffffff8108d7e5>] __lock_acquire+0xa2f/0xd0c
       [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
       [<ffffffff814cf0ab>] _raw_spin_lock+0x40/0x73
       [<ffffffff814c6c12>] rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
       [<ffffffff810c4ed6>] __rcu_read_unlock+0x18e/0x231
       [<ffffffff810463f4>] rcu_read_unlock+0x26/0x28
       [<ffffffff8104b6db>] cpuacct_charge+0x58/0x61
       [<ffffffff81052f18>] update_curr+0x107/0x134
       [<ffffffff8105349b>] check_preempt_wakeup+0xc9/0x1d0
       [<ffffffff81049775>] check_preempt_curr+0x2f/0x6e
       [<ffffffff81053f5e>] ttwu_do_wakeup+0x7b/0x111
       [<ffffffff81054050>] ttwu_do_activate.constprop.76+0x5c/0x61
       [<ffffffff8105409e>] sched_ttwu_pending+0x49/0x5b
       [<ffffffff810540be>] scheduler_ipi+0xe/0x10
       [<ffffffff810224f6>] smp_reschedule_interrupt+0x1b/0x1d
       [<ffffffff814d6b33>] reschedule_interrupt+0x13/0x20
       [<ffffffff813fcc18>] rcu_read_unlock+0x26/0x28
       [<ffffffff813fe308>] sock_def_readable+0x88/0x8d
       [<ffffffff81497760>] unix_stream_sendmsg+0x264/0x2ff
       [<ffffffff813f83c4>] sock_aio_write+0x112/0x126
       [<ffffffff8114093b>] do_sync_write+0xbf/0xff
       [<ffffffff81141012>] vfs_write+0xb6/0xf6
       [<ffffffff81141206>] sys_write+0x4d/0x74
       [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

other info that might help us debug this:

Chain exists of:
  rcu_node_level_0 --> &p->pi_lock --> &rq->lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&rq->lock);
                               lock(&p->pi_lock);
                               lock(&rq->lock);

lock(rcu_node_level_0);

*** DEADLOCK ***

1 lock held by libvirtd/5720:
#0: (&rq->lock){-.-.-.}, at: [<ffffffff8105408e>] sched_ttwu_pending+0x39/0x5b

stack backtrace:
Pid: 5720, comm: libvirtd Not tainted 3.0.0-rc6+ #91
Call Trace:
 <IRQ>  [<ffffffff814c51cf>] print_circular_bug+0x1f8/0x209
 [<ffffffff8108d7e5>] __lock_acquire+0xa2f/0xd0c
 [<ffffffff8107e905>] ? sched_clock_local+0x12/0x75
 [<ffffffff814c6c12>] ? rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff8108dfc5>] lock_acquire+0xf3/0x13e
 [<ffffffff814c6c12>] ? rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff8108adab>] ? lock_release_holdtime.part.10+0x59/0x62
 [<ffffffff814cf0ab>] _raw_spin_lock+0x40/0x73
 [<ffffffff814c6c12>] ? rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff814cf855>] ? _raw_spin_unlock+0x47/0x54
 [<ffffffff814c6c12>] rcu_report_unblock_qs_rnp.part.5+0x3f/0x60
 [<ffffffff810c4e00>] ? __rcu_read_unlock+0xb8/0x231
 [<ffffffff810c4ed6>] __rcu_read_unlock+0x18e/0x231
 [<ffffffff810463f4>] rcu_read_unlock+0x26/0x28
 [<ffffffff8104b6db>] cpuacct_charge+0x58/0x61
 [<ffffffff81052f18>] update_curr+0x107/0x134
 [<ffffffff8105349b>] check_preempt_wakeup+0xc9/0x1d0
 [<ffffffff81049775>] check_preempt_curr+0x2f/0x6e
 [<ffffffff81053f5e>] ttwu_do_wakeup+0x7b/0x111
 [<ffffffff81054050>] ttwu_do_activate.constprop.76+0x5c/0x61
 [<ffffffff8105409e>] sched_ttwu_pending+0x49/0x5b
 [<ffffffff810540be>] scheduler_ipi+0xe/0x10
 [<ffffffff810224f6>] smp_reschedule_interrupt+0x1b/0x1d
 [<ffffffff814d6b33>] reschedule_interrupt+0x13/0x20
 <EOI>  [<ffffffff8107e905>] ? sched_clock_local+0x12/0x75
 [<ffffffff810c4d91>] ? __rcu_read_unlock+0x49/0x231
 [<ffffffff8108dea5>] ? lock_release+0x1b1/0x1de
 [<ffffffff813fcc18>] rcu_read_unlock+0x26/0x28
 [<ffffffff813fe308>] sock_def_readable+0x88/0x8d
 [<ffffffff81497760>] unix_stream_sendmsg+0x264/0x2ff
 [<ffffffff813f83c4>] sock_aio_write+0x112/0x126
 [<ffffffff8121cd95>] ? inode_has_perm+0x6a/0x77

Quoted 8 lines

 [<ffffffff8114093b>] do_sync_write+0xbf/0xff
 [<ffffffff81219562>] ? security_file_permission+0x2e/0x33
 [<ffffffff81140d71>] ? rw_verify_area+0xb6/0xd3
 [<ffffffff81141012>] vfs_write+0xb6/0xf6
 [<ffffffff811426a0>] ? fget_light+0x97/0xa2
 [<ffffffff81141206>] sys_write+0x4d/0x74
 [<ffffffff81078f85>] ? remove_wait_queue+0x1a/0x3a
 [<ffffffff814d5c02>] system_call_fastpath+0x16/0x1b

Hi Paul, RCU is doing really bad things here, rcu_read_unlock() -> rcu_read_unlock_special() can do all kinds of nasty, including as per the above call back into the scheduler, which is kinda a death-warrant seeing as the scheduler itself can use RCU.

So I am hoping that your idea of doing rcu_read_lock() before acquiring rq locks (and pi locks) and doing rcu_read_unlock() after releasing them does work out!

Quoted 4 lines · Paul E. McKenney

So I am hoping that your idea of doing rcu_read_lock() before acquiring rq locks (and pi locks) and doing rcu_read_unlock() after releasing them does work out!

it would look something like the below, except that it needs some performance testing, it does add a lot of rcu fiddling to hot paths.

Need sleep now.. will poke more in the morning.

---
 kernel/sched.c      |   76 ++++++++++++++++++++++++++++++++++++++-------------
 kernel/sched_fair.c |   14 +++++++---
 kernel/sched_rt.c   |    6 ++++
 3 files changed, 73 insertions(+), 23 deletions(-)

kernel/sched.c +0 -0

index 9769c75..4bf0951 100644

kernel/sched.c +2 -0

@@ -3055,10 +3082,12 @@ static inline void post_schedule(struct rq *rq)
 	if (rq->post_schedule) {
 		unsigned long flags;
 
+		rcu_read_lock();
 		raw_spin_lock_irqsave(&rq->lock, flags);
 		if (rq->curr->sched_class->post_schedule)
 			rq->curr->sched_class->post_schedule(rq);
 		raw_spin_unlock_irqrestore(&rq->lock, flags);
+		rcu_read_unlock();
 
 		rq->post_schedule = 0;
 	}

kernel/sched.c static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev) · line 895

Quoted 5 lines

kernel/sched.c +1 -0

@@ -892,6 +892,7 @@ static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev)
       * prev into current:
       */
      spin_acquire(&rq->lock.dep_map, 0, 0, _THIS_IP_);
+     rcu_read_acquire();

Oooh... This is a tricky one. Hmmm...

Does any of this make sense?

kernel/sched.c context_switch(struct rq *rq, struct task_struct *prev, · line 3173

Quoted 5 lines

kernel/sched.c +1 -0

@@ -3141,6 +3170,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
       */
 #ifndef __ARCH_WANT_UNLOCKED_CTXSW
      spin_release(&rq->lock.dep_map, 1, _THIS_IP_);
+     rcu_read_release();

My guess is that we don't need the rcu_read_release() -- the arch shouldn't care that we have a non-atomic field in task_struct incremented, right?

Or am I confused about what this is doing?

Quoted 7 lines · Paul E. McKenney

Review static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev) · line 895

Quoted 5 lines

(quoted diff) +1 -0

@@ -892,6 +892,7 @@ static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev)
       * prev into current:
       */
      spin_acquire(&rq->lock.dep_map, 0, 0, _THIS_IP_);
+     rcu_read_acquire();

Oooh... This is a tricky one. Hmmm...

<snip>

Quoted 1 line

Does any of this make sense?

No?

Quoted 10 lines

Review context_switch(struct rq *rq, struct task_struct *prev, · line 3173

Quoted 5 lines

(quoted diff) +1 -0

@@ -3141,6 +3170,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
       */
 #ifndef __ARCH_WANT_UNLOCKED_CTXSW
      spin_release(&rq->lock.dep_map, 1, _THIS_IP_);
+     rcu_read_release();

My guess is that we don't need the rcu_read_release() -- the arch shouldn't care that we have a non-atomic field in task_struct incremented, right?

Or am I confused about what this is doing?

Either that or I used the wrong primitives for what I was meaning to do.

So from looking at rcupdate.h rcu_read_{acquire,release} are the lockdep annotations of the rcu_read_lock. The thing we're doing above is context switching and not making lockdep upset.

The problem is that held lock state is tracked per task, and we're well switching tasks, so we need to transfer the held lock state from the old to the new task, since the new task will be unlocking the thing, if at that time lockdep finds we're not actually holding it it screams bloody murder.

So what we do is we release the lock (annotation only, the actual lock says locked) on prev before switching and acquire the lock on next right after switching.

Quoted 38 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Review static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev) · line 895

Quoted 5 lines

(quoted diff) +1 -0

@@ -892,6 +892,7 @@ static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev)
       * prev into current:
       */
      spin_acquire(&rq->lock.dep_map, 0, 0, _THIS_IP_);
+     rcu_read_acquire();

Oooh... This is a tricky one. Hmmm...

<snip>

Quoted 1 line

Does any of this make sense?

No?

Quoted 10 lines

Review context_switch(struct rq *rq, struct task_struct *prev, · line 3173

Quoted 5 lines

(quoted diff) +1 -0

@@ -3141,6 +3170,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
       */
 #ifndef __ARCH_WANT_UNLOCKED_CTXSW
      spin_release(&rq->lock.dep_map, 1, _THIS_IP_);
+     rcu_read_release();

My guess is that we don't need the rcu_read_release() -- the arch shouldn't care that we have a non-atomic field in task_struct incremented, right?

Or am I confused about what this is doing?

Either that or I used the wrong primitives for what I was meaning to do.

So from looking at rcupdate.h rcu_read_{acquire,release} are the lockdep annotations of the rcu_read_lock. The thing we're doing above is context switching and not making lockdep upset.

The problem is that held lock state is tracked per task, and we're well switching tasks, so we need to transfer the held lock state from the old to the new task, since the new task will be unlocking the thing, if at that time lockdep finds we're not actually holding it it screams bloody murder.

Quoted 3 lines

So what we do is we release the lock (annotation only, the actual lock says locked) on prev before switching and acquire the lock on next right after switching.

OK, got it. I think, anyway.

So preemptible RCU and context switches are handled as follows by your patch, correct?

1. The outgoing task is in an RCU read-side critical section
due to the rcu_read_lock() that you added at the beginning of schedule().

2. In context_switch(), the lockdep state is set so that lockdep
believes that the task has exited its RCU read-side critical section (thus suppressing the lockdep splat that would otherwise appear).

3. Once the context switch has happened, RCU no longer pays
attention to the outgoing task. So the outgoing task's current->rcu_read_lock_nesting still indicates that the task is in an RCU read-side critical section, but RCU does not know or care.

4. If the incoming task has done at least one prior context switch,
its current->rcu_read_lock_nesting will indicate that it is already in an RCU read-side critical section. Since there is no opportunity for RCU to report a quiescent state during the context switch itself (because holding the runqueue lock keeps interrupts disabled), RCU sees the whole context switch event as being one big RCU read-side critical section.

If interrupts were enabled at any point, RCU might see this as two back-to-back read-side critical sections. This might be OK for all I know, but it would open the door more widely to heavyweight RCU processing at rcu_read_unlock() time.

5. Yes, context switch is a quiescent state, but that quiescent
state happens when rcu_note_context_switch() is called from schedule, which is before the runqueue lock is acquired, and thus before the beginning of the RCU read-side critical section that covers the context switch.

6. In finish_lock_switch(), the rcu_read_acquire() restores
lockdep state to match that of the incoming task.

But if the task is newly created, then its value of current->rcu_read_lock_nesting will be zero, which will leave the incoming task unprotected by RCU. I believe that you need the (untested) patch below to make this work properly. But this patch needs to be included in your set, as I don't immediately see a reasonable way to make it work independently. (Yes, we could have a special CONFIG var for the transition, but it sounds a lot easier to just have it be part of your patch.)

7. Exit processing is done by the task itself before a last
call to schedule(). From what I can see, it should not matter that the value of current->rcu_read_lock_nesting gets incremented on this last TASK_DEAD call to schedule().

Or am I still confused?

On Wed, Jul 13, 2011 at 09:33:09AM -0700, Paul E. McKenney wrote:

Hmmm... Something is confused. I get boot time hangs with the occasional stack overflow, whether or not I add my patch on top. Left to myself, I would try applying your patch incrementally, and also disabling irqs before rcu_read_lock() and enabling them after rcu_read_unlock(), as this prevents rcu_read_unlock() from ever getting to the rcu_read_unlock_special() slowpath.