[PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT

[PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT

[Sebastian Andrzej Siewior]

The lkp bot reported that rt_rwlock_is_contended() is not used so I
looked closer.

#1 wires up rwlock_is_contended() with rt_rwlock_is_contended() as it
was probably intended. As noted in the patch description, a writer
will always see true based on the current implementation. This could be
solved by looking at the waiters of the rtmutex underneath as done in #2
for spin_lock (which is missing). The helper is not exported and would
be also needed for rwsem_is_contended() because it is using the same
rw_base_is_contended() implementation.

Maybe it is not worth the trouble given that on PREEMPT_RT the rwlock/
spinlock is preemptible so it might be just best to return false and
wait for the scheduler to do its magic.

Sebastian

[PATCH 1/2] locking: Wire up rwlock_is_contended() on PREEMPT_RT

[Sebastian Andrzej Siewior]

rwlock_is_contended() should return 0 if there are no waiters pending
on that lock on, != 0 otherwise.
PREEMPT_RT's implementation of RW-locks provides
rt_rwlock_is_contended() which returns != 0 if there is a writer about
to acquire the lock. The only downside is that a writter, that is
using that function, will always see that this lock is contended even if
there are no readers or writters pending.

Wire up rwlock_is_contended() with RT's rt_rwlock_is_contended().

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/rwlock_rt.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/rwlock_rt.h b/include/linux/rwlock_rt.h
index 49c1f3842ed5b..e482671ff49ee 100644
--- a/include/linux/rwlock_rt.h
+++ b/include/linux/rwlock_rt.h
@@ -30,6 +30,7 @@ extern void rt_read_unlock(rwlock_t *rwlock);
 extern void rt_write_lock(rwlock_t *rwlock);
 extern int rt_write_trylock(rwlock_t *rwlock);
 extern void rt_write_unlock(rwlock_t *rwlock);
+extern int rt_rwlock_is_contended(rwlock_t *rwlock);
 
 static __always_inline void read_lock(rwlock_t *rwlock)
 {
@@ -135,6 +136,6 @@ static __always_inline void write_unlock_irqrestore(rwlock_t *rwlock,
 	rt_write_unlock(rwlock);
 }
 
-#define rwlock_is_contended(lock)		(((void)(lock), 0))
+#define rwlock_is_contended(lock)		rt_rwlock_is_contended(lock)
 
 #endif /* __LINUX_RWLOCK_RT_H */
-- 
2.33.0

[PATCH 2/2] locking: Provide spin_is_contended() on PREEMPT_RT

[Sebastian Andrzej Siewior]

spin_is_contended() should return != 0 if there is a waiter pending on
the lock. This can be checked on PREEMPT_RT's spin lock implementation
by checking if the lock as any waiters pending.

Provide spin_is_contended() on PREEMPT_RT by checking if there are any
waiters.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/spinlock_rt.h  | 3 ++-
 kernel/locking/spinlock_rt.c | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
index 835aedaf68acd..844bc8f9cf091 100644
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -39,6 +39,7 @@ extern void rt_spin_unlock(spinlock_t *lock);
 extern void rt_spin_lock_unlock(spinlock_t *lock);
 extern int rt_spin_trylock_bh(spinlock_t *lock);
 extern int rt_spin_trylock(spinlock_t *lock);
+extern int rt_spin_is_contended(spinlock_t *lock);
 
 static __always_inline void spin_lock(spinlock_t *lock)
 {
@@ -145,7 +146,7 @@ static __always_inline void spin_unlock_irqrestore(spinlock_t *lock,
 #define spin_trylock_irqsave(lock, flags)		\
 	__cond_lock(lock, __spin_trylock_irqsave(lock, flags))
 
-#define spin_is_contended(lock)		(((void)(lock), 0))
+#define spin_is_contended(lock)		rt_spin_is_contended(lock)
 
 static inline int spin_is_locked(spinlock_t *lock)
 {
diff --git a/kernel/locking/spinlock_rt.c b/kernel/locking/spinlock_rt.c
index d2912e44d61fd..c9acb665277ff 100644
--- a/kernel/locking/spinlock_rt.c
+++ b/kernel/locking/spinlock_rt.c
@@ -118,6 +118,12 @@ int __sched rt_spin_trylock_bh(spinlock_t *lock)
 }
 EXPORT_SYMBOL(rt_spin_trylock_bh);
 
+int __sched rt_spin_is_contended(spinlock_t *lock)
+{
+	return rt_mutex_has_waiters(&lock->lock);
+}
+EXPORT_SYMBOL(rt_spin_is_contended);
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 void __rt_spin_lock_init(spinlock_t *lock, const char *name,
 			 struct lock_class_key *key, bool percpu)
-- 
2.33.0

Re: [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2021-09-06 16:30:02 [+0200], To linux-kernel@vger.kernel.org wrote:
> The lkp bot reported that rt_rwlock_is_contended() is not used so I
> looked closer.
> 
> #1 wires up rwlock_is_contended() with rt_rwlock_is_contended() as it
> was probably intended. As noted in the patch description, a writer
> will always see true based on the current implementation. This could be
> solved by looking at the waiters of the rtmutex underneath as done in #2
> for spin_lock (which is missing). The helper is not exported and would
> be also needed for rwsem_is_contended() because it is using the same
> rw_base_is_contended() implementation.
> 
> Maybe it is not worth the trouble given that on PREEMPT_RT the rwlock/
> spinlock is preemptible so it might be just best to return false and
> wait for the scheduler to do its magic.

I looked at callers and would argue that we could simply return false
here for all is_conded checks (as we do). We don't spin on RT but
schedule out on contention and even if there are waiters, as long as we
are the task with the highest priority, then the lock won't be handed
over to the next task in line. Therefore I suggest to remove the unused
rt_rwlock_is_contended().

The rwsem_is_contended() makes sense since it is only used by readers.
Also it is a sleep-able lock so it has the same semantic as !RT.
However. shrink_slab() will drop the lock _and_ could move on which is
good. The two mmap_lock_is_contended() user will drop the read lock and
acquire it again which will always succeed as long as there is another
reader.
Unlike the !RT implementation of rwsem which would not allow that.

Sebastian

[PATCH] locking: Remove rt_rwlock_is_contended()

[Sebastian Andrzej Siewior]

rt_rwlock_is_contended() has not users. It makes no sense to use it as
rwlock_is_contended() because it is a sleeping lock on RT and preemption
is possible. It reports always != 0 if used by a writer and even if
there is a waiter then the lock might not be handed over if the
current owner has the highest priority.

Remove rt_rwlock_is_contended().

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 8282947f67345 ("locking/rwlock: Provide RT variant")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/locking/spinlock_rt.c |    6 ------
 1 file changed, 6 deletions(-)

--- a/kernel/locking/spinlock_rt.c
+++ b/kernel/locking/spinlock_rt.c
@@ -246,12 +246,6 @@ void __sched rt_write_unlock(rwlock_t *r
 }
 EXPORT_SYMBOL(rt_write_unlock);
 
-int __sched rt_rwlock_is_contended(rwlock_t *rwlock)
-{
-	return rw_base_is_contended(&rwlock->rwbase);
-}
-EXPORT_SYMBOL(rt_rwlock_is_contended);
-
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 void __rt_rwlock_init(rwlock_t *rwlock, const char *name,
 		      struct lock_class_key *key)

Re: [PATCH] locking: Remove rt_rwlock_is_contended()

[Peter Zijlstra]

On Tue, Sep 07, 2021 at 12:34:58PM +0200, Sebastian Andrzej Siewior wrote:
> rt_rwlock_is_contended() has not users. It makes no sense to use it as
> rwlock_is_contended() because it is a sleeping lock on RT and preemption
> is possible. It reports always != 0 if used by a writer and even if
> there is a waiter then the lock might not be handed over if the
> current owner has the highest priority.

I'm confused now... so first you have two patches that wire up
{spin,rwlock}_is_contended() and how you're arguing we shouldn't do
that?

AFAICT the _is_contended() can still use useful even with preemption,
the typicla use case is a long lock-holder deciding to drop the lock in
order to let someone else in. That still works with preemptible locks,
no?

Re: [PATCH] locking: Remove rt_rwlock_is_contended()

[Sebastian Andrzej Siewior]

On 2021-09-10 18:16:14 [+0200], Peter Zijlstra wrote:
> On Tue, Sep 07, 2021 at 12:34:58PM +0200, Sebastian Andrzej Siewior wrote:
> > rt_rwlock_is_contended() has not users. It makes no sense to use it as
> > rwlock_is_contended() because it is a sleeping lock on RT and preemption
> > is possible. It reports always != 0 if used by a writer and even if
> > there is a waiter then the lock might not be handed over if the
> > current owner has the highest priority.
> 
> I'm confused now... so first you have two patches that wire up
> {spin,rwlock}_is_contended() and how you're arguing we shouldn't do
> that?

Yes. I got arguments against it after sleeping :)

> AFAICT the _is_contended() can still use useful even with preemption,
> the typicla use case is a long lock-holder deciding to drop the lock in
> order to let someone else in. That still works with preemptible locks,
> no?

Sure. We can do that. Then we should look into:
- fixing rwsem_is_contended() for the writer. The writer always observes
  true even with no waiter around.

- checking the top waiter list vs priority of the lock owner/current. If
  the current lock owner has the highest priority then the unlock+lock
  is probably pointless as he regains the lock.
  For the spin_lock() case, if the owner is SCHED_OTHER and the waiter
  is SCHED_OTHER then unlock+lock will give the lock to the previous
  owner due to rt_mutex_steal() working in his favour. Unless there is a
  preemption.

- reader checking for contention is probably pointless. It works with a
  pending writer and one reader since a second reader will hold-off the
  writer from acquiring the lock. Also if the reader does unlock+lock
  then writer might not be quick enough.

Sebastian

Re: [PATCH] locking: Remove rt_rwlock_is_contended()

[Peter Zijlstra]

On Fri, Sep 10, 2021 at 06:37:04PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-09-10 18:16:14 [+0200], Peter Zijlstra wrote:
> > On Tue, Sep 07, 2021 at 12:34:58PM +0200, Sebastian Andrzej Siewior wrote:

> Yes. I got arguments against it after sleeping :)

Sleep is magical :-)

> > AFAICT the _is_contended() can still use useful even with preemption,
> > the typicla use case is a long lock-holder deciding to drop the lock in
> > order to let someone else in. That still works with preemptible locks,
> > no?
> 
> Sure. We can do that. Then we should look into:
> - fixing rwsem_is_contended() for the writer. The writer always observes
>   true even with no waiter around.

Right, that function does look somewhat dodgy. I'm thinking the current
function returns true if there's more than a single reader present (or a
writer) present, which is not the same.

I suppose it shoud return something like:

  for a writer: rt_mutex_is_contended(&rwb->rtmutex);
  for a reader: rt_mutex_is_locked(&rwb->rtmutex);

However, given the below arguments,,,

> - checking the top waiter list vs priority of the lock owner/current. If
>   the current lock owner has the highest priority then the unlock+lock
>   is probably pointless as he regains the lock.
>   For the spin_lock() case, if the owner is SCHED_OTHER and the waiter
>   is SCHED_OTHER then unlock+lock will give the lock to the previous
>   owner due to rt_mutex_steal() working in his favour. Unless there is a
>   preemption.

That is a good argument against all this; I had not considered that.

> - reader checking for contention is probably pointless. It works with a
>   pending writer and one reader since a second reader will hold-off the
>   writer from acquiring the lock. Also if the reader does unlock+lock
>   then writer might not be quick enough.

Should be fixable with a handoff, but yeah.

OK, I suppose the safe and easy option is to never report contention as
per your latest patch, and if/when someone complains about it, they can
sort through these issues :-)