[PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Sebastian Andrzej Siewior]

On !RT a locked spinlock_t and rwlock_t disables preemption which
implies a RCU read section. There is code that relies on that behaviour.

Add an explicit RCU read section on RT while a sleeping lock (a lock
which would disables preemption on !RT) acquired.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/locking/rtmutex.c   | 6 ++++++
 kernel/locking/rwlock-rt.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index cf09f8e7ed0f4..602eb7821a1b1 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1141,6 +1141,7 @@ void __sched rt_spin_lock_slowunlock(struct rt_mutex *lock)
 void __lockfunc rt_spin_lock(spinlock_t *lock)
 {
 	sleeping_lock_inc();
+	rcu_read_lock();
 	migrate_disable();
 	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
 	rt_spin_lock_fastlock(&lock->lock, rt_spin_lock_slowlock);
@@ -1156,6 +1157,7 @@ void __lockfunc __rt_spin_lock(struct rt_mutex *lock)
 void __lockfunc rt_spin_lock_nested(spinlock_t *lock, int subclass)
 {
 	sleeping_lock_inc();
+	rcu_read_lock();
 	migrate_disable();
 	spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
 	rt_spin_lock_fastlock(&lock->lock, rt_spin_lock_slowlock);
@@ -1169,6 +1171,7 @@ void __lockfunc rt_spin_unlock(spinlock_t *lock)
 	spin_release(&lock->dep_map, 1, _RET_IP_);
 	rt_spin_lock_fastunlock(&lock->lock, rt_spin_lock_slowunlock);
 	migrate_enable();
+	rcu_read_unlock();
 	sleeping_lock_dec();
 }
 EXPORT_SYMBOL(rt_spin_unlock);
@@ -1200,6 +1203,7 @@ int __lockfunc rt_spin_trylock(spinlock_t *lock)
 	ret = __rt_mutex_trylock(&lock->lock);
 	if (ret) {
 		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
+		rcu_read_lock();
 	} else {
 		migrate_enable();
 		sleeping_lock_dec();
@@ -1216,6 +1220,7 @@ int __lockfunc rt_spin_trylock_bh(spinlock_t *lock)
 	ret = __rt_mutex_trylock(&lock->lock);
 	if (ret) {
 		sleeping_lock_inc();
+		rcu_read_lock();
 		migrate_disable();
 		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
 	} else
@@ -1232,6 +1237,7 @@ int __lockfunc rt_spin_trylock_irqsave(spinlock_t *lock, unsigned long *flags)
 	ret = __rt_mutex_trylock(&lock->lock);
 	if (ret) {
 		sleeping_lock_inc();
+		rcu_read_lock();
 		migrate_disable();
 		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
 	}
diff --git a/kernel/locking/rwlock-rt.c b/kernel/locking/rwlock-rt.c
index c3b91205161cc..0ae8c62ea8320 100644
--- a/kernel/locking/rwlock-rt.c
+++ b/kernel/locking/rwlock-rt.c
@@ -310,6 +310,7 @@ int __lockfunc rt_read_trylock(rwlock_t *rwlock)
 	ret = do_read_rt_trylock(rwlock);
 	if (ret) {
 		rwlock_acquire_read(&rwlock->dep_map, 0, 1, _RET_IP_);
+		rcu_read_lock();
 	} else {
 		migrate_enable();
 		sleeping_lock_dec();
@@ -327,6 +328,7 @@ int __lockfunc rt_write_trylock(rwlock_t *rwlock)
 	ret = do_write_rt_trylock(rwlock);
 	if (ret) {
 		rwlock_acquire(&rwlock->dep_map, 0, 1, _RET_IP_);
+		rcu_read_lock();
 	} else {
 		migrate_enable();
 		sleeping_lock_dec();
@@ -338,6 +340,7 @@ EXPORT_SYMBOL(rt_write_trylock);
 void __lockfunc rt_read_lock(rwlock_t *rwlock)
 {
 	sleeping_lock_inc();
+	rcu_read_lock();
 	migrate_disable();
 	rwlock_acquire_read(&rwlock->dep_map, 0, 0, _RET_IP_);
 	do_read_rt_lock(rwlock);
@@ -347,6 +350,7 @@ EXPORT_SYMBOL(rt_read_lock);
 void __lockfunc rt_write_lock(rwlock_t *rwlock)
 {
 	sleeping_lock_inc();
+	rcu_read_lock();
 	migrate_disable();
 	rwlock_acquire(&rwlock->dep_map, 0, 0, _RET_IP_);
 	do_write_rt_lock(rwlock);
@@ -358,6 +362,7 @@ void __lockfunc rt_read_unlock(rwlock_t *rwlock)
 	rwlock_release(&rwlock->dep_map, 1, _RET_IP_);
 	do_read_rt_unlock(rwlock);
 	migrate_enable();
+	rcu_read_unlock();
 	sleeping_lock_dec();
 }
 EXPORT_SYMBOL(rt_read_unlock);
@@ -367,6 +372,7 @@ void __lockfunc rt_write_unlock(rwlock_t *rwlock)
 	rwlock_release(&rwlock->dep_map, 1, _RET_IP_);
 	do_write_rt_unlock(rwlock);
 	migrate_enable();
+	rcu_read_unlock();
 	sleeping_lock_dec();
 }
 EXPORT_SYMBOL(rt_write_unlock);
-- 
2.24.0

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Steven Rostedt]

On Tue, 19 Nov 2019 09:46:40 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On !RT a locked spinlock_t and rwlock_t disables preemption which
> implies a RCU read section. There is code that relies on that behaviour.
> 
> Add an explicit RCU read section on RT while a sleeping lock (a lock
> which would disables preemption on !RT) acquired.

I know that there was some work to merge the RCU flavors of
rcu_read_lock and rcu_read_lock_sched, I'm assuming this depends on
that behavior. That is, a synchronize_rcu() will wait for all CPUs to
schedule and all grace periods to finish, which means that those using
rcu_read_lock() and those using all CPUs to schedule can be
interchangeable. That is, on !RT, it's likely that rcu_read_lock()
waiters will end up waiting for all CPUs to schedule, and on RT, this
makes it where those waiting for all CPUs to schedule, will also wait
for all rcu_read_lock()s grace periods to finish. If that's the case,
then this change is fine. But it depends on that being the case, which
it wasn't in older kernels, and we need to be careful about backporting
this.

-- Steve


> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Paul E. McKenney]

On Tue, Nov 19, 2019 at 09:21:49AM -0500, Steven Rostedt wrote:
> On Tue, 19 Nov 2019 09:46:40 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > On !RT a locked spinlock_t and rwlock_t disables preemption which
> > implies a RCU read section. There is code that relies on that behaviour.
> > 
> > Add an explicit RCU read section on RT while a sleeping lock (a lock
> > which would disables preemption on !RT) acquired.
> 
> I know that there was some work to merge the RCU flavors of
> rcu_read_lock and rcu_read_lock_sched, I'm assuming this depends on
> that behavior. That is, a synchronize_rcu() will wait for all CPUs to
> schedule and all grace periods to finish, which means that those using

s/grace periods/RCU readers/, but yes.

> rcu_read_lock() and those using all CPUs to schedule can be
> interchangeable. That is, on !RT, it's likely that rcu_read_lock()
> waiters will end up waiting for all CPUs to schedule, and on RT, this
> makes it where those waiting for all CPUs to schedule, will also wait
> for all rcu_read_lock()s grace periods to finish. If that's the case,
> then this change is fine. But it depends on that being the case, which
> it wasn't in older kernels, and we need to be careful about backporting
> this.

Right in one.  Backporting across the RCU flavor consolidation change
must be done with care.

							Thanx, Paul

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Sebastian Andrzej Siewior]

On 2019-11-19 09:21:49 [-0500], Steven Rostedt wrote:
> On Tue, 19 Nov 2019 09:46:40 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > On !RT a locked spinlock_t and rwlock_t disables preemption which
> > implies a RCU read section. There is code that relies on that behaviour.
> > 
> > Add an explicit RCU read section on RT while a sleeping lock (a lock
> > which would disables preemption on !RT) acquired.
> 
> I know that there was some work to merge the RCU flavors of
> rcu_read_lock and rcu_read_lock_sched, I'm assuming this depends on
> that behavior. That is, a synchronize_rcu() will wait for all CPUs to
> schedule and all grace periods to finish, which means that those using
> rcu_read_lock() and those using all CPUs to schedule can be
> interchangeable. That is, on !RT, it's likely that rcu_read_lock()
> waiters will end up waiting for all CPUs to schedule, and on RT, this
> makes it where those waiting for all CPUs to schedule, will also wait
> for all rcu_read_lock()s grace periods to finish. If that's the case,
> then this change is fine. But it depends on that being the case, which
> it wasn't in older kernels, and we need to be careful about backporting
> this.

Let me give you an example how I got into this:

do_sigaction() acquires p->sighand->siglock and then iterates over list
via for_each_thread() which is a list_for_each_entry_rcu(). No RCU lock
is held, just the siglock.
On removal side, __unhash_process() removes a task from the list but
while doing so it holds the siglock and tasklist_lock. So it is
perfectly fine.
Later, we have:
|do_exit()
| -> exit_notify()
|   -> write_lock_irq(&tasklist_lock);
|   -> forget_original_parent()
|      -> find_child_reaper()
|        -> find_alive_thread()
|           -> for_each_thread()

find_alive_thread() does the for_each_thread() and checks PF_EXITING.
it might be enough for not operating on "removed" task_struct. It
dereferences task_struct->flags while looking for PF_EXITING. At this
point only tasklist_lock is acquired.
I have *no* idea if the whole synchronisation based on siglock/
PF_EXITING/ tasklist_lock is enough and RCU simply doesn't matter. It
seems so.

I am a little worried if this construct here (or somewhere else) assumes
that holding one of those locks, which disable preemption, is the same
as rcu_read_lock() (or rcu_read_lock_sched()).

> -- Steve

Sebastian

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Steven Rostedt]

On Fri, 22 Nov 2019 19:01:40 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> Let me give you an example how I got into this:
> 
> do_sigaction() acquires p->sighand->siglock and then iterates over list
> via for_each_thread() which is a list_for_each_entry_rcu(). No RCU lock
> is held, just the siglock.
> On removal side, __unhash_process() removes a task from the list but
> while doing so it holds the siglock and tasklist_lock. So it is
> perfectly fine.
> Later, we have:
> |do_exit()
> | -> exit_notify()
> |   -> write_lock_irq(&tasklist_lock);
> |   -> forget_original_parent()
> |      -> find_child_reaper()
> |        -> find_alive_thread()
> |           -> for_each_thread()
> 
> find_alive_thread() does the for_each_thread() and checks PF_EXITING.
> it might be enough for not operating on "removed" task_struct. It
> dereferences task_struct->flags while looking for PF_EXITING. At this
> point only tasklist_lock is acquired.
> I have *no* idea if the whole synchronisation based on siglock/
> PF_EXITING/ tasklist_lock is enough and RCU simply doesn't matter. It
> seems so.
> 
> I am a little worried if this construct here (or somewhere else) assumes
> that holding one of those locks, which disable preemption, is the same
> as rcu_read_lock() (or rcu_read_lock_sched()).

I'm wondering if instead, we should start throwing in rcu_read_lock()
and explicitly have the preempt disabled rcu use that as well, since
today it's basically one and the same.

Although, we still need to be careful for some special cases like
function tracing, that uses its own kind of RCU. But that should be
fixed when I introduce rcu_read_lock_tasks() :-)

-- Steve

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Sebastian Andrzej Siewior]

On 2019-11-25 12:25:45 [-0500], Steven Rostedt wrote:
> On Fri, 22 Nov 2019 19:01:40 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > Let me give you an example how I got into this:
> > 
> > do_sigaction() acquires p->sighand->siglock and then iterates over list
> > via for_each_thread() which is a list_for_each_entry_rcu(). No RCU lock
> > is held, just the siglock.
> > On removal side, __unhash_process() removes a task from the list but
> > while doing so it holds the siglock and tasklist_lock. So it is
> > perfectly fine.
> > Later, we have:
> > |do_exit()
> > | -> exit_notify()
> > |   -> write_lock_irq(&tasklist_lock);
> > |   -> forget_original_parent()
> > |      -> find_child_reaper()
> > |        -> find_alive_thread()
> > |           -> for_each_thread()
> > 
> > find_alive_thread() does the for_each_thread() and checks PF_EXITING.
> > it might be enough for not operating on "removed" task_struct. It
> > dereferences task_struct->flags while looking for PF_EXITING. At this
> > point only tasklist_lock is acquired.
> > I have *no* idea if the whole synchronisation based on siglock/
> > PF_EXITING/ tasklist_lock is enough and RCU simply doesn't matter. It
> > seems so.
> > 
> > I am a little worried if this construct here (or somewhere else) assumes
> > that holding one of those locks, which disable preemption, is the same
> > as rcu_read_lock() (or rcu_read_lock_sched()).
> 
> I'm wondering if instead, we should start throwing in rcu_read_lock()
> and explicitly have the preempt disabled rcu use that as well, since
> today it's basically one and the same.

Any comment from the RCU camp on this?
Maybe just adding the missing RCU annotation for the list annotation is
enough (like if lock X or Y is held then everything fine). !RT gets this
implicit via preempt_disable(). I'm just worried if someone expects
this kind of behaviour.
If I remember correctly, Scott added rcu_read_lock() recently to
local_bh_disable() because RCU-torture expected it.

> Although, we still need to be careful for some special cases like
> function tracing, that uses its own kind of RCU. But that should be
> fixed when I introduce rcu_read_lock_tasks() :-)
> 
> -- Steve

Sebastian

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Paul E. McKenney]

On Fri, Nov 29, 2019 at 04:45:35PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-11-25 12:25:45 [-0500], Steven Rostedt wrote:
> > On Fri, 22 Nov 2019 19:01:40 +0100
> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > 
> > > Let me give you an example how I got into this:
> > > 
> > > do_sigaction() acquires p->sighand->siglock and then iterates over list
> > > via for_each_thread() which is a list_for_each_entry_rcu(). No RCU lock
> > > is held, just the siglock.
> > > On removal side, __unhash_process() removes a task from the list but
> > > while doing so it holds the siglock and tasklist_lock. So it is
> > > perfectly fine.
> > > Later, we have:
> > > |do_exit()
> > > | -> exit_notify()
> > > |   -> write_lock_irq(&tasklist_lock);
> > > |   -> forget_original_parent()
> > > |      -> find_child_reaper()
> > > |        -> find_alive_thread()
> > > |           -> for_each_thread()
> > > 
> > > find_alive_thread() does the for_each_thread() and checks PF_EXITING.
> > > it might be enough for not operating on "removed" task_struct. It
> > > dereferences task_struct->flags while looking for PF_EXITING. At this
> > > point only tasklist_lock is acquired.
> > > I have *no* idea if the whole synchronisation based on siglock/
> > > PF_EXITING/ tasklist_lock is enough and RCU simply doesn't matter. It
> > > seems so.
> > > 
> > > I am a little worried if this construct here (or somewhere else) assumes
> > > that holding one of those locks, which disable preemption, is the same
> > > as rcu_read_lock() (or rcu_read_lock_sched()).
> > 
> > I'm wondering if instead, we should start throwing in rcu_read_lock()
> > and explicitly have the preempt disabled rcu use that as well, since
> > today it's basically one and the same.
> 
> Any comment from the RCU camp on this?
> Maybe just adding the missing RCU annotation for the list annotation is
> enough (like if lock X or Y is held then everything fine). !RT gets this
> implicit via preempt_disable(). I'm just worried if someone expects
> this kind of behaviour.
> If I remember correctly, Scott added rcu_read_lock() recently to
> local_bh_disable() because RCU-torture expected it.

Adding an explicit rcu_read_lock()/rcu_read_unlock() pair to the various
spinlock primitives for -rt seems quite sensible to me.  My guess is
that non-rt CONFIG_PREEMPT=y uses in mainline might not like the extra
overhead.

For the trylock primitives, would it make more sense to do the
rcu_read_lock() only after successful acquisition, or to do the
rcu_read_lock() to begin with and then do rcu_read_unlock() upon failure?
I would guess the latter, but don't feel strongly about it.

							Thanx, Paul

Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section on RT

[Paul E. McKenney]

On Tue, Nov 19, 2019 at 09:46:40AM +0100, Sebastian Andrzej Siewior wrote:
> On !RT a locked spinlock_t and rwlock_t disables preemption which
> implies a RCU read section. There is code that relies on that behaviour.
> 
> Add an explicit RCU read section on RT while a sleeping lock (a lock
> which would disables preemption on !RT) acquired.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  kernel/locking/rtmutex.c   | 6 ++++++
>  kernel/locking/rwlock-rt.c | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index cf09f8e7ed0f4..602eb7821a1b1 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -1141,6 +1141,7 @@ void __sched rt_spin_lock_slowunlock(struct rt_mutex *lock)
>  void __lockfunc rt_spin_lock(spinlock_t *lock)
>  {
>  	sleeping_lock_inc();
> +	rcu_read_lock();
>  	migrate_disable();
>  	spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
>  	rt_spin_lock_fastlock(&lock->lock, rt_spin_lock_slowlock);
> @@ -1156,6 +1157,7 @@ void __lockfunc __rt_spin_lock(struct rt_mutex *lock)
>  void __lockfunc rt_spin_lock_nested(spinlock_t *lock, int subclass)
>  {
>  	sleeping_lock_inc();
> +	rcu_read_lock();
>  	migrate_disable();
>  	spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
>  	rt_spin_lock_fastlock(&lock->lock, rt_spin_lock_slowlock);
> @@ -1169,6 +1171,7 @@ void __lockfunc rt_spin_unlock(spinlock_t *lock)
>  	spin_release(&lock->dep_map, 1, _RET_IP_);
>  	rt_spin_lock_fastunlock(&lock->lock, rt_spin_lock_slowunlock);
>  	migrate_enable();
> +	rcu_read_unlock();
>  	sleeping_lock_dec();
>  }
>  EXPORT_SYMBOL(rt_spin_unlock);
> @@ -1200,6 +1203,7 @@ int __lockfunc rt_spin_trylock(spinlock_t *lock)
>  	ret = __rt_mutex_trylock(&lock->lock);
>  	if (ret) {
>  		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
> +		rcu_read_lock();
>  	} else {
>  		migrate_enable();
>  		sleeping_lock_dec();
> @@ -1216,6 +1220,7 @@ int __lockfunc rt_spin_trylock_bh(spinlock_t *lock)
>  	ret = __rt_mutex_trylock(&lock->lock);
>  	if (ret) {
>  		sleeping_lock_inc();
> +		rcu_read_lock();
>  		migrate_disable();
>  		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
>  	} else
> @@ -1232,6 +1237,7 @@ int __lockfunc rt_spin_trylock_irqsave(spinlock_t *lock, unsigned long *flags)
>  	ret = __rt_mutex_trylock(&lock->lock);
>  	if (ret) {
>  		sleeping_lock_inc();
> +		rcu_read_lock();
>  		migrate_disable();
>  		spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
>  	}
> diff --git a/kernel/locking/rwlock-rt.c b/kernel/locking/rwlock-rt.c
> index c3b91205161cc..0ae8c62ea8320 100644
> --- a/kernel/locking/rwlock-rt.c
> +++ b/kernel/locking/rwlock-rt.c
> @@ -310,6 +310,7 @@ int __lockfunc rt_read_trylock(rwlock_t *rwlock)
>  	ret = do_read_rt_trylock(rwlock);
>  	if (ret) {
>  		rwlock_acquire_read(&rwlock->dep_map, 0, 1, _RET_IP_);
> +		rcu_read_lock();
>  	} else {
>  		migrate_enable();
>  		sleeping_lock_dec();
> @@ -327,6 +328,7 @@ int __lockfunc rt_write_trylock(rwlock_t *rwlock)
>  	ret = do_write_rt_trylock(rwlock);
>  	if (ret) {
>  		rwlock_acquire(&rwlock->dep_map, 0, 1, _RET_IP_);
> +		rcu_read_lock();
>  	} else {
>  		migrate_enable();
>  		sleeping_lock_dec();
> @@ -338,6 +340,7 @@ EXPORT_SYMBOL(rt_write_trylock);
>  void __lockfunc rt_read_lock(rwlock_t *rwlock)
>  {
>  	sleeping_lock_inc();
> +	rcu_read_lock();
>  	migrate_disable();
>  	rwlock_acquire_read(&rwlock->dep_map, 0, 0, _RET_IP_);
>  	do_read_rt_lock(rwlock);
> @@ -347,6 +350,7 @@ EXPORT_SYMBOL(rt_read_lock);
>  void __lockfunc rt_write_lock(rwlock_t *rwlock)
>  {
>  	sleeping_lock_inc();
> +	rcu_read_lock();
>  	migrate_disable();
>  	rwlock_acquire(&rwlock->dep_map, 0, 0, _RET_IP_);
>  	do_write_rt_lock(rwlock);
> @@ -358,6 +362,7 @@ void __lockfunc rt_read_unlock(rwlock_t *rwlock)
>  	rwlock_release(&rwlock->dep_map, 1, _RET_IP_);
>  	do_read_rt_unlock(rwlock);
>  	migrate_enable();
> +	rcu_read_unlock();
>  	sleeping_lock_dec();
>  }
>  EXPORT_SYMBOL(rt_read_unlock);
> @@ -367,6 +372,7 @@ void __lockfunc rt_write_unlock(rwlock_t *rwlock)
>  	rwlock_release(&rwlock->dep_map, 1, _RET_IP_);
>  	do_write_rt_unlock(rwlock);
>  	migrate_enable();
> +	rcu_read_unlock();
>  	sleeping_lock_dec();
>  }
>  EXPORT_SYMBOL(rt_write_unlock);
> -- 
> 2.24.0
>