Re: [PATCH v2] locking/ww_mutex: Initialize waiter.ww_ctx properly

Re: [PATCH v2] locking/ww_mutex: Initialize waiter.ww_ctx properly

[Guenter Roeck]

On Thu, Aug 19, 2021 at 09:30:30PM +0200, Sebastian Andrzej Siewior wrote:
> The gathering of the debug code for the ww-mutex initialized moved the
> POISON initialiation into one spot and only set waiter.ww_ctx if the
> ww_ctx was non-NULL thus keeping the POISON value in ww-mutex case.
> 
> For ww-mutex without a context it is expected to set the context to
> NULL, the poison value was intended only for the regular mutex.
> 
> Always initialized waiter.ww_ctx to ww_ctx in the ww-mutex case.
> 
> Fixes: c0afb0ffc06e6 ("locking/ww_mutex: Gather mutex_waiter initialization")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
> v1…v2: Use PeterZ' approach.
> 
>  kernel/locking/mutex.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index 05b68931622d1..2c70213934cd4 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -614,7 +614,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
>  
>  	debug_mutex_lock_common(lock, &waiter);
>  	waiter.task = current;
> -	if (ww_ctx)
> +	if (use_ww_ctx)
>  		waiter.ww_ctx = ww_ctx;
>  
>  	lock_contended(&lock->dep_map, ip);
> -- 
> 2.33.0
> 

[patch V5 00/72] locking, sched: The PREEMPT-RT locking infrastructure

[Thomas Gleixner]

Folks,

the following series is an update to V4 which can be found here:

  https://lore.kernel.org/r/20210811120348.855823694@linutronix.de

It contains the bulk of the PREEMPT-RT locking infrastructure. In
PREEMPT-RT enabled kernels the following locking primitives are substituted
by RT-Mutex based variants:

  mutex, ww_mutex, rw_semaphore, spinlock, rwlock

semaphores are not substituted because they do not provide strict owner
semantics.

Of course raw_spinlocks are not touched either as they protect low level
operations in the scheduler, timers and hardware access.

The most interesting parts of the series which need a lot of eyeballs
are:

  - the scheduler bits which provide the infrastructure for spinlock and
    rwlock substitution to ensure that the task state is preserved when
    blocking on such a lock and a regular wakeup is handled correctly and
    not lost

  - the rtmutex core implementation to handle lock contention on spinlocks
    and rwlocks correctly vs. the task state

  - the rw_semaphore/rwlock substitutions which utilize the same
    implementation vs. the reader/writer handling

  - The new rtmutex based ww_mutex implementation.

  - the PI futex related bits to handle the interaction between blocking
    on the underlying rtmutex and contention on the hash bucket lock which
    is converted to a 'sleeping spinlock'.

The rest surely needs a thorough review as well, but those parts are pretty
straight forward: quite some code restructuring and the actual wrapper
functions to replace the existing !RT implementations.

The series survived internal testing in RT kernels and is part of the upcoming
v5.14-rc6-rt9 release.

For !RT kernels there is no functional change.

The series is also available from git:

  git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git rtmutex

Changes vs. V4

  - A few lockdep wait_type fixes in static and runtime initializers.
    Sebastian noticed while working on getting the lockdep selftests
    reenabled on RT.

  - Missing might_sleep() invocations in RT spin/rwlocks (Sebastian)

  - Add explicit owner init for local locks when lockdep is enabled instead
    of relying on zero initialized memory.

  - Add the RT variants for local locks, which is the last lock type
    getting special treatment on RT.

The lockdep selftest changes are not yet ready and will be posted in a
separate submission.


Thanks,

	tglx
---
 b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c |    4 
 b/include/linux/debug_locks.h                       |    3 
 b/include/linux/local_lock_internal.h               |   90 +
 b/include/linux/mutex.h                             |   92 +
 b/include/linux/preempt.h                           |    4 
 b/include/linux/rbtree.h                            |   30 
 b/include/linux/rbtree_types.h                      |   34 
 b/include/linux/rtmutex.h                           |   63 -
 b/include/linux/rwbase_rt.h                         |   38 
 b/include/linux/rwlock_rt.h                         |  140 ++
 b/include/linux/rwlock_types.h                      |   53 
 b/include/linux/rwsem.h                             |   78 +
 b/include/linux/sched.h                             |  119 +-
 b/include/linux/sched/wake_q.h                      |    8 
 b/include/linux/spinlock.h                          |   15 
 b/include/linux/spinlock_api_smp.h                  |    3 
 b/include/linux/spinlock_rt.h                       |  159 ++
 b/include/linux/spinlock_types.h                    |   89 -
 b/include/linux/spinlock_types_raw.h                |   73 +
 b/include/linux/ww_mutex.h                          |   50 
 b/kernel/Kconfig.locks                              |    2 
 b/kernel/futex.c                                    |  556 ++++++---
 b/kernel/locking/Makefile                           |    3 
 b/kernel/locking/mutex-debug.c                      |    5 
 b/kernel/locking/mutex.c                            |  431 -------
 b/kernel/locking/mutex.h                            |   48 
 b/kernel/locking/rtmutex.c                          | 1134 +++++++++-----------
 b/kernel/locking/rtmutex_api.c                      |  590 ++++++++++
 b/kernel/locking/rtmutex_common.h                   |  122 +-
 b/kernel/locking/rwbase_rt.c                        |  263 ++++
 b/kernel/locking/rwsem.c                            |  109 +
 b/kernel/locking/spinlock.c                         |    7 
 b/kernel/locking/spinlock_debug.c                   |    5 
 b/kernel/locking/spinlock_rt.c                      |  263 ++++
 b/kernel/locking/ww_mutex.h                         |  569 ++++++++++
 b/kernel/locking/ww_rt_mutex.c                      |   76 +
 b/kernel/rcu/tree_plugin.h                          |    6 
 b/kernel/sched/core.c                               |  109 +
 b/lib/Kconfig.debug                                 |   11 
 b/lib/test_lockup.c                                 |    8 
 kernel/locking/mutex-debug.h                        |   29 
 41 files changed, 3940 insertions(+), 1551 deletions(-)

[patch V5 41/72] locking/ww_mutex: Gather mutex_waiter initialization

[Thomas Gleixner]

From: Peter Zijlstra <peterz@infradead.org>

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/locking/mutex-debug.c |    1 +
 kernel/locking/mutex.c       |   12 +++---------
 2 files changed, 4 insertions(+), 9 deletions(-)
---
--- a/kernel/locking/mutex-debug.c
+++ b/kernel/locking/mutex-debug.c
@@ -30,6 +30,7 @@ void debug_mutex_lock_common(struct mute
 	memset(waiter, MUTEX_DEBUG_INIT, sizeof(*waiter));
 	waiter->magic = waiter;
 	INIT_LIST_HEAD(&waiter->list);
+	waiter->ww_ctx = MUTEX_POISON_WW_CTX;
 }
 
 void debug_mutex_wake_waiter(struct mutex *lock, struct mutex_waiter *waiter)
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -980,17 +980,15 @@ static __always_inline int __sched
 	}
 
 	debug_mutex_lock_common(lock, &waiter);
+	waiter.task = current;
+	if (ww_ctx)
+		waiter.ww_ctx = ww_ctx;
 
 	lock_contended(&lock->dep_map, ip);
 
 	if (!use_ww_ctx) {
 		/* add waiting tasks to the end of the waitqueue (FIFO): */
 		__mutex_add_waiter(lock, &waiter, &lock->wait_list);
-
-
-#ifdef CONFIG_DEBUG_MUTEXES
-		waiter.ww_ctx = MUTEX_POISON_WW_CTX;
-#endif
 	} else {
 		/*
 		 * Add in stamp order, waking up waiters that must kill
@@ -999,12 +997,8 @@ static __always_inline int __sched
 		ret = __ww_mutex_add_waiter(&waiter, lock, ww_ctx);
 		if (ret)
 			goto err_early_kill;
-
-		waiter.ww_ctx = ww_ctx;
 	}
 
-	waiter.task = current;
-
 	set_current_state(state);
 	for (;;) {
 		/*

Re: [patch V5 41/72] locking/ww_mutex: Gather mutex_waiter initialization

[Sebastian Andrzej Siewior]

On 2021-08-15 23:28:39 [+0200], Thomas Gleixner wrote:
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -30,6 +30,7 @@ void debug_mutex_lock_common(struct mute
>  	memset(waiter, MUTEX_DEBUG_INIT, sizeof(*waiter));
>  	waiter->magic = waiter;
>  	INIT_LIST_HEAD(&waiter->list);
> +	waiter->ww_ctx = MUTEX_POISON_WW_CTX;
>  }
>  
>  void debug_mutex_wake_waiter(struct mutex *lock, struct mutex_waiter *waiter)
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -980,17 +980,15 @@ static __always_inline int __sched
>  	}
>  
>  	debug_mutex_lock_common(lock, &waiter);
> +	waiter.task = current;
> +	if (ww_ctx)
> +		waiter.ww_ctx = ww_ctx;
>  
>  	lock_contended(&lock->dep_map, ip);
>  
>  	if (!use_ww_ctx) {
>  		/* add waiting tasks to the end of the waitqueue (FIFO): */
>  		__mutex_add_waiter(lock, &waiter, &lock->wait_list);
> -
> -
> -#ifdef CONFIG_DEBUG_MUTEXES
> -		waiter.ww_ctx = MUTEX_POISON_WW_CTX;
> -#endif

The crash, Guenter Roeck reported, is because now waiter.ww_ctx is
always initialized to poisen.
Previously in the ww-mutex case without a context it was set to NULL
in the next hunk.
Keeping the ww_ctx assigment in the next hunk seems to work.

>  	} else {
>  		/*
>  		 * Add in stamp order, waking up waiters that must kill
> @@ -999,12 +997,8 @@ static __always_inline int __sched
>  		ret = __ww_mutex_add_waiter(&waiter, lock, ww_ctx);
>  		if (ret)
>  			goto err_early_kill;
> -
> -		waiter.ww_ctx = ww_ctx;
>  	}
>  
> -	waiter.task = current;
> -
>  	set_current_state(state);
>  	for (;;) {
>  		/*

Sebastian

Re: [patch V5 41/72] locking/ww_mutex: Gather mutex_waiter initialization

[Peter Zijlstra]

On Thu, Aug 19, 2021 at 07:51:10PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-08-15 23:28:39 [+0200], Thomas Gleixner wrote:
> > --- a/kernel/locking/mutex-debug.c
> > +++ b/kernel/locking/mutex-debug.c
> > @@ -30,6 +30,7 @@ void debug_mutex_lock_common(struct mute
> >  	memset(waiter, MUTEX_DEBUG_INIT, sizeof(*waiter));
> >  	waiter->magic = waiter;
> >  	INIT_LIST_HEAD(&waiter->list);
> > +	waiter->ww_ctx = MUTEX_POISON_WW_CTX;
> >  }
> >  
> >  void debug_mutex_wake_waiter(struct mutex *lock, struct mutex_waiter *waiter)
> > --- a/kernel/locking/mutex.c
> > +++ b/kernel/locking/mutex.c
> > @@ -980,17 +980,15 @@ static __always_inline int __sched
> >  	}
> >  
> >  	debug_mutex_lock_common(lock, &waiter);
> > +	waiter.task = current;
> > +	if (ww_ctx)
> > +		waiter.ww_ctx = ww_ctx;
> >  
> >  	lock_contended(&lock->dep_map, ip);
> >  
> >  	if (!use_ww_ctx) {
> >  		/* add waiting tasks to the end of the waitqueue (FIFO): */
> >  		__mutex_add_waiter(lock, &waiter, &lock->wait_list);
> > -
> > -
> > -#ifdef CONFIG_DEBUG_MUTEXES
> > -		waiter.ww_ctx = MUTEX_POISON_WW_CTX;
> > -#endif
> 
> The crash, Guenter Roeck reported, is because now waiter.ww_ctx is
> always initialized to poisen.
> Previously in the ww-mutex case without a context it was set to NULL
> in the next hunk.
> Keeping the ww_ctx assigment in the next hunk seems to work.

You mean the 'use_ww_ctx && !ww_ctx' case?

Would not the below also help with that?

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 3a65bf4bacfd..d456579d0952 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -618,7 +618,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
 
 	debug_mutex_lock_common(lock, &waiter);
 	waiter.task = current;
-	if (ww_ctx)
+	if (use_ww_ctx)
 		waiter.ww_ctx = ww_ctx;
 
 	lock_contended(&lock->dep_map, ip);

[PATCH v2] locking/ww_mutex: Initialize waiter.ww_ctx properly

[Sebastian Andrzej Siewior]

The gathering of the debug code for the ww-mutex initialized moved the
POISON initialiation into one spot and only set waiter.ww_ctx if the
ww_ctx was non-NULL thus keeping the POISON value in ww-mutex case.

For ww-mutex without a context it is expected to set the context to
NULL, the poison value was intended only for the regular mutex.

Always initialized waiter.ww_ctx to ww_ctx in the ww-mutex case.

Fixes: c0afb0ffc06e6 ("locking/ww_mutex: Gather mutex_waiter initialization")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: Use PeterZ' approach.

 kernel/locking/mutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 05b68931622d1..2c70213934cd4 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -614,7 +614,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
 
 	debug_mutex_lock_common(lock, &waiter);
 	waiter.task = current;
-	if (ww_ctx)
+	if (use_ww_ctx)
 		waiter.ww_ctx = ww_ctx;
 
 	lock_contended(&lock->dep_map, ip);
-- 
2.33.0