All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT
@ 2021-09-06 14:30 Sebastian Andrzej Siewior
  2021-09-06 14:30 ` [PATCH 1/2] locking: Wire up rwlock_is_contended() " Sebastian Andrzej Siewior
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-06 14:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, Thomas Gleixner

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] locking: Wire up rwlock_is_contended() on PREEMPT_RT
  2021-09-06 14:30 [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT Sebastian Andrzej Siewior
@ 2021-09-06 14:30 ` Sebastian Andrzej Siewior
  2021-09-06 14:30 ` [PATCH 2/2] locking: Provide spin_is_contended() " Sebastian Andrzej Siewior
  2021-09-07 10:09 ` [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended " Sebastian Andrzej Siewior
  2 siblings, 0 replies; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-06 14:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, Thomas Gleixner, Sebastian Andrzej Siewior,
	kernel test robot

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] locking: Provide spin_is_contended() on PREEMPT_RT
  2021-09-06 14:30 [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT Sebastian Andrzej Siewior
  2021-09-06 14:30 ` [PATCH 1/2] locking: Wire up rwlock_is_contended() " Sebastian Andrzej Siewior
@ 2021-09-06 14:30 ` Sebastian Andrzej Siewior
  2021-09-07 10:09 ` [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended " Sebastian Andrzej Siewior
  2 siblings, 0 replies; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-06 14:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, Thomas Gleixner, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT
  2021-09-06 14:30 [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT Sebastian Andrzej Siewior
  2021-09-06 14:30 ` [PATCH 1/2] locking: Wire up rwlock_is_contended() " Sebastian Andrzej Siewior
  2021-09-06 14:30 ` [PATCH 2/2] locking: Provide spin_is_contended() " Sebastian Andrzej Siewior
@ 2021-09-07 10:09 ` Sebastian Andrzej Siewior
  2021-09-07 10:34   ` [PATCH] locking: Remove rt_rwlock_is_contended() Sebastian Andrzej Siewior
  2 siblings, 1 reply; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-07 10:09 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] locking: Remove rt_rwlock_is_contended()
  2021-09-07 10:09 ` [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended " Sebastian Andrzej Siewior
@ 2021-09-07 10:34   ` Sebastian Andrzej Siewior
  2021-09-10 16:16     ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-07 10:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long,
	Boqun Feng, Thomas Gleixner

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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking: Remove rt_rwlock_is_contended()
  2021-09-07 10:34   ` [PATCH] locking: Remove rt_rwlock_is_contended() Sebastian Andrzej Siewior
@ 2021-09-10 16:16     ` Peter Zijlstra
  2021-09-10 16:37       ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2021-09-10 16:16 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
	Thomas Gleixner

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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking: Remove rt_rwlock_is_contended()
  2021-09-10 16:16     ` Peter Zijlstra
@ 2021-09-10 16:37       ` Sebastian Andrzej Siewior
  2021-09-13 11:20         ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Andrzej Siewior @ 2021-09-10 16:37 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
	Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] locking: Remove rt_rwlock_is_contended()
  2021-09-10 16:37       ` Sebastian Andrzej Siewior
@ 2021-09-13 11:20         ` Peter Zijlstra
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Zijlstra @ 2021-09-13 11:20 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
	Thomas Gleixner

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 :-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-13 11:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-06 14:30 [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended on PREEMPT_RT Sebastian Andrzej Siewior
2021-09-06 14:30 ` [PATCH 1/2] locking: Wire up rwlock_is_contended() " Sebastian Andrzej Siewior
2021-09-06 14:30 ` [PATCH 2/2] locking: Provide spin_is_contended() " Sebastian Andrzej Siewior
2021-09-07 10:09 ` [PATCH 0/2] locking: Provide {rwlock|spin}_is_contended " Sebastian Andrzej Siewior
2021-09-07 10:34   ` [PATCH] locking: Remove rt_rwlock_is_contended() Sebastian Andrzej Siewior
2021-09-10 16:16     ` Peter Zijlstra
2021-09-10 16:37       ` Sebastian Andrzej Siewior
2021-09-13 11:20         ` Peter Zijlstra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.