Re: [PATCH 1/6] locking: add ww_mutex_(un)lock_for_each helpers

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Jun 14, 2019 at 02:41:20PM +0200, Christian König wrote:
> The ww_mutex implementation allows for detection deadlocks when multiple
> threads try to acquire the same set of locks in different order.
> 
> The problem is that handling those deadlocks was the burden of the user of
> the ww_mutex implementation and at least some users didn't got that right
> on the first try.
> 
> I'm not a big fan of macros, but it still better then implementing the same
> logic at least halve a dozen times. So this patch adds two macros to lock
> and unlock multiple ww_mutex instances with the necessary deadlock handling.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  include/linux/ww_mutex.h | 75 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 3af7c0e03be5..a0d893b5b114 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -369,4 +369,79 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_unlock_for_each - cleanup after error or contention
> + * @loop: for loop code fragment iterating over all the locks
> + * @pos: code fragment returning the currently handled lock
> + * @contended: the last contended ww_mutex or NULL or ERR_PTR
> + *
> + * Helper to make cleanup after error or lock contention easier.
> + * First unlock the last contended lock and then all other locked ones.
> + */
> +#define ww_mutex_unlock_for_each(loop, pos, contended)	\
> +	if (!IS_ERR(contended)) {			\
> +		if (contended)				\
> +			ww_mutex_unlock(contended);	\
> +		contended = (pos);			\
> +		loop {					\
> +			if (contended == (pos))		\
> +				break;			\
> +			ww_mutex_unlock(pos);		\
> +		}					\
> +	}
> +
> +/**
> + * ww_mutex_lock_for_each - implement ww_mutex deadlock handling
> + * @loop: for loop code fragment iterating over all the locks
> + * @pos: code fragment returning the currently handled lock
> + * @contended: ww_mutex pointer variable for state handling
> + * @ret: int variable to store the return value of ww_mutex_lock()
> + * @intr: If true ww_mutex_lock_interruptible() is used
> + * @ctx: ww_acquire_ctx pointer for the locking
> + *
> + * This macro implements the necessary logic to lock multiple ww_mutex
> + * instances. Lock contention with backoff and re-locking is handled by the
> + * macro so that the loop body only need to handle other errors and
> + * successfully acquired locks.
> + *
> + * With the @loop and @pos code fragment it is possible to apply this logic
> + * to all kind of containers (array, list, tree, etc...) holding multiple
> + * ww_mutex instances.
> + *
> + * @contended is used to hold the current state we are in. -ENOENT is used to
> + * signal that we are just starting the handling. -EDEADLK means that the
> + * current position is contended and we need to restart the loop after locking
> + * it. NULL means that there is no contention to be handled. Any other value is
> + * the last contended ww_mutex.
> + */
> +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)	\
> +	for (contended = ERR_PTR(-ENOENT); ({				\
> +		__label__ relock, next;					\
> +		ret = -ENOENT;						\
> +		if (contended == ERR_PTR(-ENOENT))			\
> +			contended = NULL;				\
> +		else if (contended == ERR_PTR(-EDEADLK))		\
> +			contended = (pos);				\
> +		else							\
> +			goto next;					\
> +		loop {							\
> +			if (contended == (pos))	{			\
> +				contended = NULL;			\
> +				continue;				\
> +			}						\
> +relock:									\
> +			ret = !(intr) ? ww_mutex_lock(pos, ctx) :	\
> +				ww_mutex_lock_interruptible(pos, ctx);	\
> +			if (ret == -EDEADLK) {				\
> +				ww_mutex_unlock_for_each(loop, pos,	\
> +							 contended);	\
> +				contended = ERR_PTR(-EDEADLK);		\
> +				goto relock;				\
> +			}						\
> +			break;						\
> +next:									\
> +			continue;					\
> +		}							\
> +	}), ret != -ENOENT;)
> +

Yea gawds, that's eyebleeding bad, even for macros :/

It also breaks with pretty much all other *for_each*() macros in
existence by not actually including the loop itself but farming that out
to an argument statement (@loop), suggesting these really should not be
called for_each.