From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Christian_K=c3=b6nig?= Subject: Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros Date: Fri, 14 Jun 2019 15:06:10 +0200 Message-ID: <6f2084ae-61d5-8cb9-c975-901456eed7e3@gmail.com> References: <20190614124125.124181-1-christian.koenig@amd.com> <20190614124125.124181-4-christian.koenig@amd.com> <20190614125940.GP3436@hirez.programming.kicks-ass.net> Reply-To: christian.koenig@amd.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20190614125940.GP3436@hirez.programming.kicks-ass.net> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Peter Zijlstra Cc: daniel@ffwll.ch, l.stach@pengutronix.de, linux+etnaviv@armlinux.org.uk, christian.gmeiner@gmail.com, yuq825@gmail.com, eric@anholt.net, thellstrom@vmware.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, etnaviv@lists.freedesktop.org, lima@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Am 14.06.19 um 14:59 schrieb Peter Zijlstra: > On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote: >> Use the provided macros instead of implementing deadlock handling on our own. >> >> Signed-off-by: Christian König >> --- >> drivers/gpu/drm/drm_gem.c | 49 ++++++++++----------------------------- >> 1 file changed, 12 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index 50de138c89e0..6e4623d3bee2 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -1307,51 +1307,26 @@ int >> drm_gem_lock_reservations(struct drm_gem_object **objs, int count, >> struct ww_acquire_ctx *acquire_ctx) >> { >> - int contended = -1; >> + struct ww_mutex *contended; >> int i, ret; >> >> ww_acquire_init(acquire_ctx, &reservation_ww_class); >> >> -retry: >> - if (contended != -1) { >> - struct drm_gem_object *obj = objs[contended]; >> - >> - ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock, >> - acquire_ctx); >> - if (ret) { >> - ww_acquire_done(acquire_ctx); >> - return ret; >> - } >> - } >> - >> - for (i = 0; i < count; i++) { >> - if (i == contended) >> - continue; >> - >> - ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock, >> - acquire_ctx); >> - if (ret) { >> - int j; >> - >> - for (j = 0; j < i; j++) >> - ww_mutex_unlock(&objs[j]->resv->lock); >> - >> - if (contended != -1 && contended >= i) >> - ww_mutex_unlock(&objs[contended]->resv->lock); >> - >> - if (ret == -EDEADLK) { >> - contended = i; >> - goto retry; > retry here, starts the whole locking loop. > >> - } >> - >> - ww_acquire_done(acquire_ctx); >> - return ret; >> - } >> - } > +#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); \ > + } \ > + } > + > > +#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; \ > > while relock here continues where it left of and doesn't restart @loop. > Or am I reading this monstrosity the wrong way? contended = ERR_PTR(-EDEADLK) makes sure that the whole loop is restarted after retrying to acquire the lock. See the "if" above "loop". Christian. > > + } \ > + break; \ > +next: \ > + continue; \ > + } \ > + }), ret != -ENOENT;) > + > >> + ww_mutex_lock_for_each(for (i = 0; i < count; i++), >> + &objs[i]->resv->lock, contended, ret, true, >> + acquire_ctx) >> + if (ret) >> + goto error; >> >> ww_acquire_done(acquire_ctx); >> >> return 0; >> + >> +error: >> + ww_mutex_unlock_for_each(for (i = 0; i < count; i++), >> + &objs[i]->resv->lock, contended); >> + ww_acquire_done(acquire_ctx); >> + return ret; >> } >> EXPORT_SYMBOL(drm_gem_lock_reservations);