[PATCH 0/4] locking: Some locking code cleanups

[PATCH 0/4] locking: Some locking code cleanups

[Waiman Long]

This series contains a number of cleanup locking patches.

Waiman Long (4):
  locking/qspinlock: Fix 'wait_early' set but not used warning
  locking/mutex: Clean up mutex.h
  locking/rwsem: Clarify that RWSEM_READER_OWNED is just a hint
  locking/rwsem: Make DEBUG_RWSEMS and PREEMPT_RT mutually exclusive

 include/linux/mutex.h               | 23 +++++++++++------------
 kernel/locking/qspinlock_paravirt.h |  2 +-
 kernel/locking/rwsem.c              |  6 +++---
 lib/Kconfig.debug                   |  4 ++--
 4 files changed, 17 insertions(+), 18 deletions(-)

-- 
2.39.3

[PATCH 1/4] locking/qspinlock: Fix 'wait_early' set but not used warning

[Waiman Long]

When CONFIG_LOCK_EVENT_COUNTS is off, the wait_early variable will be
set but not used. This is expected. Recent compilers will not generate
wait_early code in this case.

Add the __maybe_unused attribute to wait_early for suppressing this
W=1 warning.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312260422.f4pK3f9m-lkp@intel.com/
Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/qspinlock_paravirt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
index 6a0184e9c234..ae2b12f68b90 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -294,8 +294,8 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
 {
 	struct pv_node *pn = (struct pv_node *)node;
 	struct pv_node *pp = (struct pv_node *)prev;
+	bool __maybe_unused wait_early;
 	int loop;
-	bool wait_early;
 
 	for (;;) {
 		for (wait_early = false, loop = SPIN_THRESHOLD; loop; loop--) {
-- 
2.39.3

[PATCH 2/4] locking/mutex: Clean up mutex.h

[Waiman Long]

CONFIG_DEBUG_MUTEXES and CONFIG_PREEMPT_RT are mutually exclusive. They
can't be both set at the same time.  Move down the mutex_destroy()
function declaration to the bottom of mutex.h to eliminate duplicated
mutex_destroy() declaration.

Also remove the duplicated mutex_trylock() function declaration in the
CONFIG_PREEMPT_RT section.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/mutex.h | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 7e208d46ba5b..bad383713db2 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -35,18 +35,10 @@
 #ifndef CONFIG_PREEMPT_RT
 
 #ifdef CONFIG_DEBUG_MUTEXES
-
-#define __DEBUG_MUTEX_INITIALIZER(lockname)				\
+# define __DEBUG_MUTEX_INITIALIZER(lockname)				\
 	, .magic = &lockname
-
-extern void mutex_destroy(struct mutex *lock);
-
 #else
-
 # define __DEBUG_MUTEX_INITIALIZER(lockname)
-
-static inline void mutex_destroy(struct mutex *lock) {}
-
 #endif
 
 /**
@@ -101,9 +93,6 @@ extern bool mutex_is_locked(struct mutex *lock);
 
 extern void __mutex_rt_init(struct mutex *lock, const char *name,
 			    struct lock_class_key *key);
-extern int mutex_trylock(struct mutex *lock);
-
-static inline void mutex_destroy(struct mutex *lock) { }
 
 #define mutex_is_locked(l)	rt_mutex_base_is_locked(&(l)->rtmutex)
 
@@ -170,6 +159,16 @@ extern void mutex_unlock(struct mutex *lock);
 
 extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
+#ifdef CONFIG_DEBUG_MUTEXES
+
+extern void mutex_destroy(struct mutex *lock);
+
+#else
+
+static inline void mutex_destroy(struct mutex *lock) {}
+
+#endif
+
 DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
 DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
 DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
-- 
2.39.3

[PATCH 3/4] locking/rwsem: Clarify that RWSEM_READER_OWNED is just a hint

[Waiman Long]

Clarify in the comments that the RWSEM_READER_OWNED bit in the owner
field is just a hint, not an authoritative state of the rwsem.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/rwsem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 2340b6d90ec6..c6d17aee4209 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -35,7 +35,7 @@
 /*
  * The least significant 2 bits of the owner value has the following
  * meanings when set.
- *  - Bit 0: RWSEM_READER_OWNED - The rwsem is owned by readers
+ *  - Bit 0: RWSEM_READER_OWNED - rwsem may be owned by readers (just a hint)
  *  - Bit 1: RWSEM_NONSPINNABLE - Cannot spin on a reader-owned lock
  *
  * When the rwsem is reader-owned and a spinning writer has timed out,
@@ -1002,8 +1002,8 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 
 	/*
 	 * To prevent a constant stream of readers from starving a sleeping
-	 * waiter, don't attempt optimistic lock stealing if the lock is
-	 * currently owned by readers.
+	 * writer, don't attempt optimistic lock stealing if the lock is
+	 * very likely owned by readers.
 	 */
 	if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
 	    (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
-- 
2.39.3

[PATCH 4/4] locking/rwsem: Make DEBUG_RWSEMS and PREEMPT_RT mutually exclusive

[Waiman Long]

The debugging code enabled by CONFIG_DEBUG_RWSEMS will only be
compiled in when CONFIG_PREEMPT_RT isn't set. There is no point to
allow CONFIG_DEBUG_RWSEMS to be set in a kernel configuration where
CONFIG_PREEMPT_RT is also set. Make them mutually exclusive.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 lib/Kconfig.debug | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 975a07f9f1cc..cb695bc76d30 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1303,7 +1303,7 @@ config PROVE_LOCKING
 	select DEBUG_SPINLOCK
 	select DEBUG_MUTEXES if !PREEMPT_RT
 	select DEBUG_RT_MUTEXES if RT_MUTEXES
-	select DEBUG_RWSEMS
+	select DEBUG_RWSEMS if !PREEMPT_RT
 	select DEBUG_WW_MUTEX_SLOWPATH
 	select DEBUG_LOCK_ALLOC
 	select PREEMPT_COUNT if !ARCH_NO_PREEMPT
@@ -1426,7 +1426,7 @@ config DEBUG_WW_MUTEX_SLOWPATH
 
 config DEBUG_RWSEMS
 	bool "RW Semaphore debugging: basic checks"
-	depends on DEBUG_KERNEL
+	depends on DEBUG_KERNEL && !PREEMPT_RT
 	help
 	  This debugging feature allows mismatched rw semaphore locks
 	  and unlocks to be detected and reported.
-- 
2.39.3

Re: [PATCH 1/4] locking/qspinlock: Fix 'wait_early' set but not used warning

[Boqun Feng]

On Mon, Feb 12, 2024 at 10:16:53PM -0500, Waiman Long wrote:
> When CONFIG_LOCK_EVENT_COUNTS is off, the wait_early variable will be
> set but not used. This is expected. Recent compilers will not generate
> wait_early code in this case.
> 
> Add the __maybe_unused attribute to wait_early for suppressing this
> W=1 warning.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312260422.f4pK3f9m-lkp@intel.com/
> Signed-off-by: Waiman Long <longman@redhat.com>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> ---
>  kernel/locking/qspinlock_paravirt.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
> index 6a0184e9c234..ae2b12f68b90 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -294,8 +294,8 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
>  {
>  	struct pv_node *pn = (struct pv_node *)node;
>  	struct pv_node *pp = (struct pv_node *)prev;
> +	bool __maybe_unused wait_early;
>  	int loop;
> -	bool wait_early;
>  
>  	for (;;) {
>  		for (wait_early = false, loop = SPIN_THRESHOLD; loop; loop--) {
> -- 
> 2.39.3
> 

Re: [PATCH 3/4] locking/rwsem: Clarify that RWSEM_READER_OWNED is just a hint

[Boqun Feng]

On Mon, Feb 12, 2024 at 10:16:55PM -0500, Waiman Long wrote:
> Clarify in the comments that the RWSEM_READER_OWNED bit in the owner
> field is just a hint, not an authoritative state of the rwsem.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> ---
>  kernel/locking/rwsem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 2340b6d90ec6..c6d17aee4209 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -35,7 +35,7 @@
>  /*
>   * The least significant 2 bits of the owner value has the following
>   * meanings when set.
> - *  - Bit 0: RWSEM_READER_OWNED - The rwsem is owned by readers
> + *  - Bit 0: RWSEM_READER_OWNED - rwsem may be owned by readers (just a hint)
>   *  - Bit 1: RWSEM_NONSPINNABLE - Cannot spin on a reader-owned lock
>   *
>   * When the rwsem is reader-owned and a spinning writer has timed out,
> @@ -1002,8 +1002,8 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
>  
>  	/*
>  	 * To prevent a constant stream of readers from starving a sleeping
> -	 * waiter, don't attempt optimistic lock stealing if the lock is
> -	 * currently owned by readers.
> +	 * writer, don't attempt optimistic lock stealing if the lock is
> +	 * very likely owned by readers.
>  	 */
>  	if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
>  	    (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
> -- 
> 2.39.3
> 

Re: [PATCH 2/4] locking/mutex: Clean up mutex.h

[Boqun Feng]

On Mon, Feb 12, 2024 at 10:16:54PM -0500, Waiman Long wrote:
> CONFIG_DEBUG_MUTEXES and CONFIG_PREEMPT_RT are mutually exclusive. They
> can't be both set at the same time.  Move down the mutex_destroy()
> function declaration to the bottom of mutex.h to eliminate duplicated
> mutex_destroy() declaration.
> 
> Also remove the duplicated mutex_trylock() function declaration in the
> CONFIG_PREEMPT_RT section.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Although, can you move the first "#ifdef CONFIG_DEBUG_MUTEXES" out of
the "#ifndef CONFIG_PREEMPT_RT" and combine it with the second one, i.e.

	#ifdef CONFIG_DEBUG_MUTEXES
	# define __DEBUG_MUTEX_INITIALIZER(lockname) ...
	extern void mutex_destroy(struct mutex *lock);
	#else
	# define __DEBUG_MUTEX_INITIALIZER(lockname) ..
	static inline void mutex_destroy(struct mutex *lock) {}
	#endif

	#ifndef CONFIG_PREEMPT_RT
	...

Thoughts?

Regards,
Boqun

> ---
>  include/linux/mutex.h | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 7e208d46ba5b..bad383713db2 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -35,18 +35,10 @@
>  #ifndef CONFIG_PREEMPT_RT
>  
>  #ifdef CONFIG_DEBUG_MUTEXES
> -
> -#define __DEBUG_MUTEX_INITIALIZER(lockname)				\
> +# define __DEBUG_MUTEX_INITIALIZER(lockname)				\
>  	, .magic = &lockname
> -
> -extern void mutex_destroy(struct mutex *lock);
> -
>  #else
> -
>  # define __DEBUG_MUTEX_INITIALIZER(lockname)
> -
> -static inline void mutex_destroy(struct mutex *lock) {}
> -
>  #endif
>  
>  /**
> @@ -101,9 +93,6 @@ extern bool mutex_is_locked(struct mutex *lock);
>  
>  extern void __mutex_rt_init(struct mutex *lock, const char *name,
>  			    struct lock_class_key *key);
> -extern int mutex_trylock(struct mutex *lock);
> -
> -static inline void mutex_destroy(struct mutex *lock) { }
>  
>  #define mutex_is_locked(l)	rt_mutex_base_is_locked(&(l)->rtmutex)
>  
> @@ -170,6 +159,16 @@ extern void mutex_unlock(struct mutex *lock);
>  
>  extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
>  
> +#ifdef CONFIG_DEBUG_MUTEXES
> +
> +extern void mutex_destroy(struct mutex *lock);
> +
> +#else
> +
> +static inline void mutex_destroy(struct mutex *lock) {}
> +
> +#endif
> +
>  DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
>  DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
>  DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
> -- 
> 2.39.3
> 

Re: [PATCH 4/4] locking/rwsem: Make DEBUG_RWSEMS and PREEMPT_RT mutually exclusive

[Boqun Feng]

On Mon, Feb 12, 2024 at 10:16:56PM -0500, Waiman Long wrote:
> The debugging code enabled by CONFIG_DEBUG_RWSEMS will only be
> compiled in when CONFIG_PREEMPT_RT isn't set. There is no point to
> allow CONFIG_DEBUG_RWSEMS to be set in a kernel configuration where
> CONFIG_PREEMPT_RT is also set. Make them mutually exclusive.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> ---
>  lib/Kconfig.debug | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 975a07f9f1cc..cb695bc76d30 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1303,7 +1303,7 @@ config PROVE_LOCKING
>  	select DEBUG_SPINLOCK
>  	select DEBUG_MUTEXES if !PREEMPT_RT
>  	select DEBUG_RT_MUTEXES if RT_MUTEXES
> -	select DEBUG_RWSEMS
> +	select DEBUG_RWSEMS if !PREEMPT_RT
>  	select DEBUG_WW_MUTEX_SLOWPATH
>  	select DEBUG_LOCK_ALLOC
>  	select PREEMPT_COUNT if !ARCH_NO_PREEMPT
> @@ -1426,7 +1426,7 @@ config DEBUG_WW_MUTEX_SLOWPATH
>  
>  config DEBUG_RWSEMS
>  	bool "RW Semaphore debugging: basic checks"
> -	depends on DEBUG_KERNEL
> +	depends on DEBUG_KERNEL && !PREEMPT_RT
>  	help
>  	  This debugging feature allows mismatched rw semaphore locks
>  	  and unlocks to be detected and reported.
> -- 
> 2.39.3
> 

Re: [PATCH 2/4] locking/mutex: Clean up mutex.h

[Waiman Long]

On 2/21/24 23:33, Boqun Feng wrote:
> On Mon, Feb 12, 2024 at 10:16:54PM -0500, Waiman Long wrote:
>> CONFIG_DEBUG_MUTEXES and CONFIG_PREEMPT_RT are mutually exclusive. They
>> can't be both set at the same time.  Move down the mutex_destroy()
>> function declaration to the bottom of mutex.h to eliminate duplicated
>> mutex_destroy() declaration.
>>
>> Also remove the duplicated mutex_trylock() function declaration in the
>> CONFIG_PREEMPT_RT section.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>
> Although, can you move the first "#ifdef CONFIG_DEBUG_MUTEXES" out of
> the "#ifndef CONFIG_PREEMPT_RT" and combine it with the second one, i.e.
>
> 	#ifdef CONFIG_DEBUG_MUTEXES
> 	# define __DEBUG_MUTEX_INITIALIZER(lockname) ...
> 	extern void mutex_destroy(struct mutex *lock);
> 	#else
> 	# define __DEBUG_MUTEX_INITIALIZER(lockname) ..
> 	static inline void mutex_destroy(struct mutex *lock) {}
> 	#endif
>
> 	#ifndef CONFIG_PREEMPT_RT
> 	...
>
> Thoughts?

Sure. I can move it up and combine the two pieces.

Thanks for the review.

Cheers,
Longman

>
> Regards,
> Boqun
>
>> ---
>>   include/linux/mutex.h | 23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>> index 7e208d46ba5b..bad383713db2 100644
>> --- a/include/linux/mutex.h
>> +++ b/include/linux/mutex.h
>> @@ -35,18 +35,10 @@
>>   #ifndef CONFIG_PREEMPT_RT
>>   
>>   #ifdef CONFIG_DEBUG_MUTEXES
>> -
>> -#define __DEBUG_MUTEX_INITIALIZER(lockname)				\
>> +# define __DEBUG_MUTEX_INITIALIZER(lockname)				\
>>   	, .magic = &lockname
>> -
>> -extern void mutex_destroy(struct mutex *lock);
>> -
>>   #else
>> -
>>   # define __DEBUG_MUTEX_INITIALIZER(lockname)
>> -
>> -static inline void mutex_destroy(struct mutex *lock) {}
>> -
>>   #endif
>>   
>>   /**
>> @@ -101,9 +93,6 @@ extern bool mutex_is_locked(struct mutex *lock);
>>   
>>   extern void __mutex_rt_init(struct mutex *lock, const char *name,
>>   			    struct lock_class_key *key);
>> -extern int mutex_trylock(struct mutex *lock);
>> -
>> -static inline void mutex_destroy(struct mutex *lock) { }
>>   
>>   #define mutex_is_locked(l)	rt_mutex_base_is_locked(&(l)->rtmutex)
>>   
>> @@ -170,6 +159,16 @@ extern void mutex_unlock(struct mutex *lock);
>>   
>>   extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
>>   
>> +#ifdef CONFIG_DEBUG_MUTEXES
>> +
>> +extern void mutex_destroy(struct mutex *lock);
>> +
>> +#else
>> +
>> +static inline void mutex_destroy(struct mutex *lock) {}
>> +
>> +#endif
>> +
>>   DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
>>   DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
>>   DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
>> -- 
>> 2.39.3
>>