Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 25 Jun 2019 16:22:14 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Christian =?iso-8859-1?Q?K=F6nig?= 
        <ckoenig.leichtzumerken@gmail.com>
Cc:     daniel@ffwll.ch, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] dma-buf: add reservation_context for deadlock
 handling
Message-ID: <20190625142214.GD3419@hirez.programming.kicks-ass.net>
References: <20190625135507.80548-1-christian.koenig@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190625135507.80548-1-christian.koenig@amd.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 25, 2019 at 03:55:06PM +0200, Christian K?nig wrote:

> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
> index 4d32e2c67862..9e53e42b053a 100644
> --- a/drivers/dma-buf/reservation.c
> +++ b/drivers/dma-buf/reservation.c
> @@ -55,6 +55,88 @@ EXPORT_SYMBOL(reservation_seqcount_class);
>  const char reservation_seqcount_string[] = "reservation_seqcount";
>  EXPORT_SYMBOL(reservation_seqcount_string);
>  
> +/**
> + * reservation_context_init - initialize a reservation context
> + * @ctx: the context to initialize
> + *
> + * Start using this reservation context to lock reservation objects for update.
> + */
> +void reservation_context_init(struct reservation_context *ctx)
> +{
> +	ww_acquire_init(&ctx->ctx, &reservation_ww_class);
> +	init_llist_head(&ctx->locked);

	ctx->locked = NULL;

> +	ctx->contended = NULL;
> +}
> +EXPORT_SYMBOL(reservation_context_init);
> +
> +/**
> + * reservation_context_unlock_all - unlock all reservation objects
> + * @ctx: the context which holds the reservation objects
> + *
> + * Unlocks all reservation objects locked with this context.
> + */
> +void reservation_context_unlock_all(struct reservation_context *ctx)
> +{
> +	struct reservation_object *obj, *next;
> +
> +	if (ctx->contended)
> +		ww_mutex_unlock(&ctx->contended->lock);
> +	ctx->contended = NULL;
> +
> +	llist_for_each_entry_safe(obj, next, ctx->locked.first, locked)
> +		ww_mutex_unlock(&obj->lock);

	for (obj = ctx->locked; obj && (next = obj->locked, true); obj = next)

> +	init_llist_head(&ctx->locked);

	ctx->locked = NULL;
> +}
> +EXPORT_SYMBOL(reservation_context_unlock_all);
> +
> +/**
> + * reservation_context_lock - lock a reservation object with deadlock handling
> + * @ctx: the context which should be used to lock the object
> + * @obj: the object which needs to be locked
> + * @interruptible: if we should wait interruptible or not
> + *
> + * Use @ctx to lock the reservation object. If a deadlock is detected we backoff
> + * by releasing all locked objects and use the slow path to lock the reservation
> + * object. After successfully locking in the slow path -EDEADLK is returned to
> + * signal that all other locks must be re-taken as well.
> + */
> +int reservation_context_lock(struct reservation_context *ctx,
> +			     struct reservation_object *obj,
> +			     bool interruptible)
> +{
> +	int ret = 0;
> +
> +	if (unlikely(ctx->contended == obj))
> +		ctx->contended = NULL;
> +	else if (interruptible)
> +		ret = ww_mutex_lock_interruptible(&obj->lock, &ctx->ctx);
> +	else
> +		ret = ww_mutex_lock(&obj->lock, &ctx->ctx);
> +
> +	if (likely(!ret)) {
> +		/* don't use llist_add here, we have separate locking */
> +		obj->locked.next = ctx->locked.first;
> +		ctx->locked.first = &obj->locked;

Since you're not actually using llist, could you then maybe open-code
the whole thing and not rely on its implementation?

That is, it hardly seems worth the trouble, as shown the bits you do use
are hardly longer than just writing it out. Also, I was confused by the
use of llist, as I wondered where the concurrency came from.

> +		return 0;
> +	}
> +	if (unlikely(ret != -EDEADLK))
> +		return ret;
> +
> +	reservation_context_unlock_all(ctx);
> +
> +	if (interruptible) {
> +		ret = ww_mutex_lock_slow_interruptible(&obj->lock, &ctx->ctx);
> +		if (unlikely(ret))
> +			return ret;
> +	} else {
> +		ww_mutex_lock_slow(&obj->lock, &ctx->ctx);
> +	}
> +
> +	ctx->contended = obj;
> +	return -EDEADLK;
> +}
> +EXPORT_SYMBOL(reservation_context_lock);

> diff --git a/include/linux/reservation.h b/include/linux/reservation.h
> index ee750765cc94..a8a52e5d3e80 100644
> --- a/include/linux/reservation.h
> +++ b/include/linux/reservation.h

> +struct reservation_context {
> +	struct ww_acquire_ctx ctx;
> +	struct llist_head locked;

	struct reservation_object *locked;

> +	struct reservation_object *contended;
> +};

> @@ -71,6 +108,7 @@ struct reservation_object_list {
>   */
>  struct reservation_object {
>  	struct ww_mutex lock;
> +	struct llist_node locked;

	struct reservation_object *locked;

>  	seqcount_t seq;
>  
>  	struct dma_fence __rcu *fence_excl;
> -- 
> 2.17.1
>