Message-ID: <YUCpfrbfPSZvD3Xl@phenom.ffwll.local>
Date:   Tue, 14 Sep 2021 15:54:06 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH v2] kernel/locking: Add context to ww_mutex_trylock.

On Tue, Sep 14, 2021 at 02:43:02PM +0200, Maarten Lankhorst wrote:
> Op 14-09-2021 om 08:50 schreef Peter Zijlstra:
> > On Mon, Sep 13, 2021 at 10:42:36AM +0200, Maarten Lankhorst wrote:
> >
> >>> +/**
> >>> + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context
> >>> + * @ww: mutex to lock
> >>> + * @ww_ctx: optional w/w acquire context
> >>> + *
> >>> + * Trylocks a mutex with the optional acquire context; no deadlock detection is
> >>> + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise.
> >>> + *
> >>> + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is
> >>> + * specified, -EALREADY handling may happen in calls to ww_mutex_trylock.
> >>> + *
> >>> + * A mutex acquired with this function must be released with ww_mutex_unlock.
> >>> + */
> >>> +int ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx)
> >>> +{
> >>> +	if (!ww_ctx)
> >>> +		return mutex_trylock(&ww->base);
> >>> +
> >>> +	MUTEX_WARN_ON(ww->base.magic != &ww->base);
> >>> +
> >>> +	if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
> >>> +		return -EALREADY;
> >> I'm not 100% sure this is a good idea, because it would make the
> >> trylock weird.  For i915 I checked manually, because I didn't want to
> >> change the function signature. This is probably the other extreme.
> >>
> >> "if (ww_mutex_trylock())" would look correct, but actually be wrong
> >> and lead to double unlock without adjustments.  Maybe we could make a
> >> ww_mutex_trylock_ctx_err, which would return -EALREADY or -EBUSY on
> >> failure, and 0 on success?  We could keep ww_mutex_trylock without
> >> ctx, probably just #define as (!ww_mutex_trylock_ctx_err(lock, NULL))
> > Urgh, yeah. Also, I suppose that if we already own it, we'll just fail
> > the trylock anyway. Let me take this out.
> >
> >>> +	/*
> >>> +	 * Reset the wounded flag after a kill. No other process can
> >>> +	 * race and wound us here, since they can't have a valid owner
> >>> +	 * pointer if we don't have any locks held.
> >>> +	 */
> >>> +	if (ww_ctx->acquired == 0)
> >>> +		ww_ctx->wounded = 0;
> >> Yeah I guess this needs fixing too. Not completely sure since trylock
> >> wouldn't do the whole ww dance, but since it's our first lock,
> >> probably best to do so regardless so other users don't trip over it.
> > This is actually critical, because if this trylock is the first lock
> > acquisition for the context, there won't be any other opportunity to
> > reset this value.
> >
> >>> +
> >>> +	if (__mutex_trylock(&ww->base)) {
> >>> +		ww_mutex_set_context_fastpath(ww, ww_ctx);
> >>> +		mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ww_ctx->dep_map, _RET_IP_);
> >>> +		return 1;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(ww_mutex_trylock);
> > Updated version below...
> >
> > ---
> > Subject: kernel/locking: Add context to ww_mutex_trylock()
> > From: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> > Date: Thu, 9 Sep 2021 11:32:18 +0200
> >
> > From: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> >
> > i915 will soon gain an eviction path that trylock a whole lot of locks
> > for eviction, getting dmesg failures like below:
> >
> >   BUG: MAX_LOCK_DEPTH too low!
> >   turning off the locking correctness validator.
> >   depth: 48  max: 48!
> >   48 locks held by i915_selftest/5776:
> >    #0: ffff888101a79240 (&dev->mutex){....}-{3:3}, at: __driver_attach+0x88/0x160
> >    #1: ffffc900009778c0 (reservation_ww_class_acquire){+.+.}-{0:0}, at: i915_vma_pin.constprop.63+0x39/0x1b0 [i915]
> >    #2: ffff88800cf74de8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_vma_pin.constprop.63+0x5f/0x1b0 [i915]
> >    #3: ffff88810c7f9e38 (&vm->mutex/1){+.+.}-{3:3}, at: i915_vma_pin_ww+0x1c4/0x9d0 [i915]
> >    #4: ffff88810bad5768 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
> >    #5: ffff88810bad60e8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
> >   ...
> >    #46: ffff88811964d768 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
> >    #47: ffff88811964e0e8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: i915_gem_evict_something+0x110/0x860 [i915]
> >   INFO: lockdep is turned off.
> >
> > Fixing eviction to nest into ww_class_acquire is a high priority, but
> > it requires a rework of the entire driver, which can only be done one
> > step at a time.
> >
> > As an intermediate solution, add an acquire context to
> > ww_mutex_trylock, which allows us to do proper nesting annotations on
> > the trylocks, making the above lockdep splat disappear.
> >
> > This is also useful in regulator_lock_nested, which may avoid dropping
> > regulator_nesting_mutex in the uncontended path, so use it there.
> >
> > TTM may be another user for this, where we could lock a buffer in a
> > fastpath with list locks held, without dropping all locks we hold.
> >
> > [peterz: rework actual ww_mutex_trylock() implementations]
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > ---
> 
> My original patch series with this patch in place still passes i915 selftests, looks good to me. :)

For merge logistics, can we pls have a stable branch? I expect that the
i915 patches will be ready for 5.16.

Or send it in for -rc2 so that the interface change doesn't cause needless
conflicts, whatever you think is best.
-Daniel

> 
> Feel free to apply.
> 
> 
> >  drivers/gpu/drm/drm_modeset_lock.c |    2 
> >  drivers/regulator/core.c           |    2 
> >  include/linux/dma-resv.h           |    2 
> >  include/linux/ww_mutex.h           |   15 ------
> >  kernel/locking/mutex.c             |   41 +++++++++++++++++
> >  kernel/locking/test-ww_mutex.c     |   86 +++++++++++++++++++++++++++----------
> >  kernel/locking/ww_rt_mutex.c       |   25 ++++++++++
> >  lib/locking-selftest.c             |    2 
> >  8 files changed, 137 insertions(+), 38 deletions(-)
> >
> > --- a/drivers/gpu/drm/drm_modeset_lock.c
> > +++ b/drivers/gpu/drm/drm_modeset_lock.c
> > @@ -248,7 +248,7 @@ static inline int modeset_lock(struct dr
> >  	if (ctx->trylock_only) {
> >  		lockdep_assert_held(&ctx->ww_ctx);
> >  
> > -		if (!ww_mutex_trylock(&lock->mutex))
> > +		if (!ww_mutex_trylock(&lock->mutex, NULL))
> >  			return -EBUSY;
> >  		else
> >  			return 0;
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -145,7 +145,7 @@ static inline int regulator_lock_nested(
> >  
> >  	mutex_lock(&regulator_nesting_mutex);
> >  
> > -	if (ww_ctx || !ww_mutex_trylock(&rdev->mutex)) {
> > +	if (!ww_mutex_trylock(&rdev->mutex, ww_ctx)) {
> >  		if (rdev->mutex_owner == current)
> >  			rdev->ref_cnt++;
> >  		else
> > --- a/include/linux/dma-resv.h
> > +++ b/include/linux/dma-resv.h
> > @@ -173,7 +173,7 @@ static inline int dma_resv_lock_slow_int
> >   */
> >  static inline bool __must_check dma_resv_trylock(struct dma_resv *obj)
> >  {
> > -	return ww_mutex_trylock(&obj->lock);
> > +	return ww_mutex_trylock(&obj->lock, NULL);
> >  }
> >  
> >  /**
> > --- a/include/linux/ww_mutex.h
> > +++ b/include/linux/ww_mutex.h
> > @@ -28,12 +28,10 @@
> >  #ifndef CONFIG_PREEMPT_RT
> >  #define WW_MUTEX_BASE			mutex
> >  #define ww_mutex_base_init(l,n,k)	__mutex_init(l,n,k)
> > -#define ww_mutex_base_trylock(l)	mutex_trylock(l)
> >  #define ww_mutex_base_is_locked(b)	mutex_is_locked((b))
> >  #else
> >  #define WW_MUTEX_BASE			rt_mutex
> >  #define ww_mutex_base_init(l,n,k)	__rt_mutex_init(l,n,k)
> > -#define ww_mutex_base_trylock(l)	rt_mutex_trylock(l)
> >  #define ww_mutex_base_is_locked(b)	rt_mutex_base_is_locked(&(b)->rtmutex)
> >  #endif
> >  
> > @@ -339,17 +337,8 @@ ww_mutex_lock_slow_interruptible(struct
> >  
> >  extern void ww_mutex_unlock(struct ww_mutex *lock);
> >  
> > -/**
> > - * ww_mutex_trylock - tries to acquire the w/w mutex without acquire context
> > - * @lock: mutex to lock
> > - *
> > - * Trylocks a mutex without acquire context, so no deadlock detection is
> > - * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise.
> > - */
> > -static inline int __must_check ww_mutex_trylock(struct ww_mutex *lock)
> > -{
> > -	return ww_mutex_base_trylock(&lock->base);
> > -}
> > +extern int __must_check ww_mutex_trylock(struct ww_mutex *lock,
> > +					 struct ww_acquire_ctx *ctx);
> >  
> >  /***
> >   * ww_mutex_destroy - mark a w/w mutex unusable
> > --- a/kernel/locking/mutex.c
> > +++ b/kernel/locking/mutex.c
> > @@ -94,6 +94,9 @@ static inline unsigned long __owner_flag
> >  	return owner & MUTEX_FLAGS;
> >  }
> >  
> > +/*
> > + * Returns: __mutex_owner(lock) on failure or NULL on success.
> > + */
> >  static inline struct task_struct *__mutex_trylock_common(struct mutex *lock, bool handoff)
> >  {
> >  	unsigned long owner, curr = (unsigned long)current;
> > @@ -736,6 +739,44 @@ __ww_mutex_lock(struct mutex *lock, unsi
> >  	return __mutex_lock_common(lock, state, subclass, NULL, ip, ww_ctx, true);
> >  }
> >  
> > +/**
> > + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context
> > + * @ww: mutex to lock
> > + * @ww_ctx: optional w/w acquire context
> > + *
> > + * Trylocks a mutex with the optional acquire context; no deadlock detection is
> > + * possible. Returns 1 if the mutex has been acquired successfully, 0 otherwise.
> > + *
> > + * Unlike ww_mutex_lock, no deadlock handling is performed. However, if a @ctx is
> > + * specified, -EALREADY handling may happen in calls to ww_mutex_trylock.
> > + *
> > + * A mutex acquired with this function must be released with ww_mutex_unlock.
> > + */
> > +int ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ww_ctx)
> > +{
> > +	if (!ww_ctx)
> > +		return mutex_trylock(&ww->base);
> > +
> > +	MUTEX_WARN_ON(ww->base.magic != &ww->base);
> > +
> > +	/*
> > +	 * Reset the wounded flag after a kill. No other process can
> > +	 * race and wound us here, since they can't have a valid owner
> > +	 * pointer if we don't have any locks held.
> > +	 */
> > +	if (ww_ctx->acquired == 0)
> > +		ww_ctx->wounded = 0;
> > +
> > +	if (__mutex_trylock(&ww->base)) {
> > +		ww_mutex_set_context_fastpath(ww, ww_ctx);
> > +		mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ww_ctx->dep_map, _RET_IP_);
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ww_mutex_trylock);
> > +
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >  void __sched
> >  mutex_lock_nested(struct mutex *lock, unsigned int subclass)
> > --- a/kernel/locking/test-ww_mutex.c
> > +++ b/kernel/locking/test-ww_mutex.c
> > @@ -16,6 +16,15 @@
> >  static DEFINE_WD_CLASS(ww_class);
> >  struct workqueue_struct *wq;
> >  
> > +#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
> > +#define ww_acquire_init_noinject(a, b) do { \
> > +		ww_acquire_init((a), (b)); \
> > +		(a)->deadlock_inject_countdown = ~0U; \
> > +	} while (0)
> > +#else
> > +#define ww_acquire_init_noinject(a, b) ww_acquire_init((a), (b))
> > +#endif
> > +
> >  struct test_mutex {
> >  	struct work_struct work;
> >  	struct ww_mutex mutex;
> > @@ -36,7 +45,7 @@ static void test_mutex_work(struct work_
> >  	wait_for_completion(&mtx->go);
> >  
> >  	if (mtx->flags & TEST_MTX_TRY) {
> > -		while (!ww_mutex_trylock(&mtx->mutex))
> > +		while (!ww_mutex_trylock(&mtx->mutex, NULL))
> >  			cond_resched();
> >  	} else {
> >  		ww_mutex_lock(&mtx->mutex, NULL);
> > @@ -109,19 +118,38 @@ static int test_mutex(void)
> >  	return 0;
> >  }
> >  
> > -static int test_aa(void)
> > +static int test_aa(bool trylock)
> >  {
> >  	struct ww_mutex mutex;
> >  	struct ww_acquire_ctx ctx;
> >  	int ret;
> > +	const char *from = trylock ? "trylock" : "lock";
> >  
> >  	ww_mutex_init(&mutex, &ww_class);
> >  	ww_acquire_init(&ctx, &ww_class);
> >  
> > -	ww_mutex_lock(&mutex, &ctx);
> > +	if (!trylock) {
> > +		ret = ww_mutex_lock(&mutex, &ctx);
> > +		if (ret) {
> > +			pr_err("%s: initial lock failed!\n", __func__);
> > +			goto out;
> > +		}
> > +	} else {
> > +		if (!ww_mutex_trylock(&mutex, &ctx)) {
> > +			pr_err("%s: initial trylock failed!\n", __func__);
> > +			goto out;
> > +		}
> > +	}
> >  
> > -	if (ww_mutex_trylock(&mutex))  {
> > -		pr_err("%s: trylocked itself!\n", __func__);
> > +	if (ww_mutex_trylock(&mutex, NULL))  {
> > +		pr_err("%s: trylocked itself without context from %s!\n", __func__, from);
> > +		ww_mutex_unlock(&mutex);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	if (ww_mutex_trylock(&mutex, &ctx))  {
> > +		pr_err("%s: trylocked itself with context from %s!\n", __func__, from);
> >  		ww_mutex_unlock(&mutex);
> >  		ret = -EINVAL;
> >  		goto out;
> > @@ -129,17 +157,17 @@ static int test_aa(void)
> >  
> >  	ret = ww_mutex_lock(&mutex, &ctx);
> >  	if (ret != -EALREADY) {
> > -		pr_err("%s: missed deadlock for recursing, ret=%d\n",
> > -		       __func__, ret);
> > +		pr_err("%s: missed deadlock for recursing, ret=%d from %s\n",
> > +		       __func__, ret, from);
> >  		if (!ret)
> >  			ww_mutex_unlock(&mutex);
> >  		ret = -EINVAL;
> >  		goto out;
> >  	}
> >  
> > +	ww_mutex_unlock(&mutex);
> >  	ret = 0;
> >  out:
> > -	ww_mutex_unlock(&mutex);
> >  	ww_acquire_fini(&ctx);
> >  	return ret;
> >  }
> > @@ -150,7 +178,7 @@ struct test_abba {
> >  	struct ww_mutex b_mutex;
> >  	struct completion a_ready;
> >  	struct completion b_ready;
> > -	bool resolve;
> > +	bool resolve, trylock;
> >  	int result;
> >  };
> >  
> > @@ -160,8 +188,13 @@ static void test_abba_work(struct work_s
> >  	struct ww_acquire_ctx ctx;
> >  	int err;
> >  
> > -	ww_acquire_init(&ctx, &ww_class);
> > -	ww_mutex_lock(&abba->b_mutex, &ctx);
> > +	ww_acquire_init_noinject(&ctx, &ww_class);
> > +	if (!abba->trylock)
> > +		ww_mutex_lock(&abba->b_mutex, &ctx);
> > +	else
> > +		WARN_ON(!ww_mutex_trylock(&abba->b_mutex, &ctx));
> > +
> > +	WARN_ON(READ_ONCE(abba->b_mutex.ctx) != &ctx);
> >  
> >  	complete(&abba->b_ready);
> >  	wait_for_completion(&abba->a_ready);
> > @@ -181,7 +214,7 @@ static void test_abba_work(struct work_s
> >  	abba->result = err;
> >  }
> >  
> > -static int test_abba(bool resolve)
> > +static int test_abba(bool trylock, bool resolve)
> >  {
> >  	struct test_abba abba;
> >  	struct ww_acquire_ctx ctx;
> > @@ -192,12 +225,18 @@ static int test_abba(bool resolve)
> >  	INIT_WORK_ONSTACK(&abba.work, test_abba_work);
> >  	init_completion(&abba.a_ready);
> >  	init_completion(&abba.b_ready);
> > +	abba.trylock = trylock;
> >  	abba.resolve = resolve;
> >  
> >  	schedule_work(&abba.work);
> >  
> > -	ww_acquire_init(&ctx, &ww_class);
> > -	ww_mutex_lock(&abba.a_mutex, &ctx);
> > +	ww_acquire_init_noinject(&ctx, &ww_class);
> > +	if (!trylock)
> > +		ww_mutex_lock(&abba.a_mutex, &ctx);
> > +	else
> > +		WARN_ON(!ww_mutex_trylock(&abba.a_mutex, &ctx));
> > +
> > +	WARN_ON(READ_ONCE(abba.a_mutex.ctx) != &ctx);
> >  
> >  	complete(&abba.a_ready);
> >  	wait_for_completion(&abba.b_ready);
> > @@ -249,7 +288,7 @@ static void test_cycle_work(struct work_
> >  	struct ww_acquire_ctx ctx;
> >  	int err, erra = 0;
> >  
> > -	ww_acquire_init(&ctx, &ww_class);
> > +	ww_acquire_init_noinject(&ctx, &ww_class);
> >  	ww_mutex_lock(&cycle->a_mutex, &ctx);
> >  
> >  	complete(cycle->a_signal);
> > @@ -581,7 +620,9 @@ static int stress(int nlocks, int nthrea
> >  static int __init test_ww_mutex_init(void)
> >  {
> >  	int ncpus = num_online_cpus();
> > -	int ret;
> > +	int ret, i;
> > +
> > +	printk(KERN_INFO "Beginning ww mutex selftests\n");
> >  
> >  	wq = alloc_workqueue("test-ww_mutex", WQ_UNBOUND, 0);
> >  	if (!wq)
> > @@ -591,17 +632,19 @@ static int __init test_ww_mutex_init(voi
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = test_aa();
> > +	ret = test_aa(false);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = test_abba(false);
> > +	ret = test_aa(true);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = test_abba(true);
> > -	if (ret)
> > -		return ret;
> > +	for (i = 0; i < 4; i++) {
> > +		ret = test_abba(i & 1, i & 2);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	ret = test_cycle(ncpus);
> >  	if (ret)
> > @@ -619,6 +662,7 @@ static int __init test_ww_mutex_init(voi
> >  	if (ret)
> >  		return ret;
> >  
> > +	printk(KERN_INFO "All ww mutex selftests passed\n");
> >  	return 0;
> >  }
> >  
> > --- a/kernel/locking/ww_rt_mutex.c
> > +++ b/kernel/locking/ww_rt_mutex.c
> > @@ -9,6 +9,31 @@
> >  #define WW_RT
> >  #include "rtmutex.c"
> >  
> > +int ww_mutex_trylock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx)
> > +{
> > +	struct rt_mutex *rtm = &lock->base;
> > +
> > +	if (!ww_ctx)
> > +		return rt_mutex_trylock(rtm);
> > +
> > +	/*
> > +	 * Reset the wounded flag after a kill. No other process can
> > +	 * race and wound us here, since they can't have a valid owner
> > +	 * pointer if we don't have any locks held.
> > +	 */
> > +	if (ww_ctx->acquired == 0)
> > +		ww_ctx->wounded = 0;
> > +
> > +	if (__rt_mutex_trylock(&rtm->rtmutex)) {
> > +		ww_mutex_set_context_fastpath(lock, ww_ctx);
> > +		mutex_acquire_nest(&rtm->dep_map, 0, 1, ww_ctx->dep_map, _RET_IP_);
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ww_mutex_trylock);
> > +
> >  static int __sched
> >  __ww_rt_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx,
> >  		   unsigned int state, unsigned long ip)
> > --- a/lib/locking-selftest.c
> > +++ b/lib/locking-selftest.c
> > @@ -258,7 +258,7 @@ static void init_shared_classes(void)
> >  #define WWAF(x)			ww_acquire_fini(x)
> >  
> >  #define WWL(x, c)		ww_mutex_lock(x, c)
> > -#define WWT(x)			ww_mutex_trylock(x)
> > +#define WWT(x)			ww_mutex_trylock(x, NULL)
> >  #define WWL1(x)			ww_mutex_lock(x, NULL)
> >  #define WWU(x)			ww_mutex_unlock(x)
> >  
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists