From: Daniel Vetter <daniel@ffwll.ch> To: Peter Zijlstra <peterz@infradead.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, Waiman Long <longman@redhat.com>, Boqun Feng <boqun.feng@gmail.com>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, linux-kernel@vger.kernel.org Subject: Re: [PATCH] kernel/locking: Add context to ww_mutex_trylock. Date: Wed, 8 Sep 2021 20:30:54 +0200 [thread overview] Message-ID: <YTkBXhu1QRxfqq1R@phenom.ffwll.local> (raw) In-Reply-To: <YTiM/zf8BuNw7wes@hirez.programming.kicks-ass.net> On Wed, Sep 08, 2021 at 12:14:23PM +0200, Peter Zijlstra wrote: > On Tue, Sep 07, 2021 at 03:20:44PM +0200, Maarten Lankhorst wrote: > > 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. > > > 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. > > Fair enough I suppose. What's maybe missing from the commit message - we'll probably use this for ttm too eventually - even when we add full ww_mutex locking we'll still have the trylock fastpath. This is because we have a lock inversion against list locks in these eviction paths, and the slow path unroll to drop that list lock is a bit nasty (and defintely expensive). iow even long term this here is needed in some form I think. -Daniel > > > +/** > > + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context > > + * @lock: mutex to lock > > + * @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 and -EDEADLK handling may happen in calls to ww_mutex_lock. > > + * > > + * A mutex acquired with this function must be released with ww_mutex_unlock. > > + */ > > +int __sched > > +ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ctx) > > +{ > > + bool locked; > > + > > + if (!ctx) > > + return mutex_trylock(&ww->base); > > + > > +#ifdef CONFIG_DEBUG_MUTEXES > > + DEBUG_LOCKS_WARN_ON(ww->base.magic != &ww->base); > > +#endif > > + > > + preempt_disable(); > > + locked = __mutex_trylock(&ww->base); > > + > > + if (locked) { > > + ww_mutex_set_context_fastpath(ww, ctx); > > + mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ctx->dep_map, _RET_IP_); > > + } > > + preempt_enable(); > > + > > + return locked; > > +} > > +EXPORT_SYMBOL(ww_mutex_trylock); > > You'll need a similar hunk in ww_rt_mutex.c -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Peter Zijlstra <peterz@infradead.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>, Waiman Long <longman@redhat.com>, Boqun Feng <boqun.feng@gmail.com>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, linux-kernel@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH] kernel/locking: Add context to ww_mutex_trylock. Date: Wed, 8 Sep 2021 20:30:54 +0200 [thread overview] Message-ID: <YTkBXhu1QRxfqq1R@phenom.ffwll.local> (raw) In-Reply-To: <YTiM/zf8BuNw7wes@hirez.programming.kicks-ass.net> On Wed, Sep 08, 2021 at 12:14:23PM +0200, Peter Zijlstra wrote: > On Tue, Sep 07, 2021 at 03:20:44PM +0200, Maarten Lankhorst wrote: > > 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. > > > 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. > > Fair enough I suppose. What's maybe missing from the commit message - we'll probably use this for ttm too eventually - even when we add full ww_mutex locking we'll still have the trylock fastpath. This is because we have a lock inversion against list locks in these eviction paths, and the slow path unroll to drop that list lock is a bit nasty (and defintely expensive). iow even long term this here is needed in some form I think. -Daniel > > > +/** > > + * ww_mutex_trylock - tries to acquire the w/w mutex with optional acquire context > > + * @lock: mutex to lock > > + * @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 and -EDEADLK handling may happen in calls to ww_mutex_lock. > > + * > > + * A mutex acquired with this function must be released with ww_mutex_unlock. > > + */ > > +int __sched > > +ww_mutex_trylock(struct ww_mutex *ww, struct ww_acquire_ctx *ctx) > > +{ > > + bool locked; > > + > > + if (!ctx) > > + return mutex_trylock(&ww->base); > > + > > +#ifdef CONFIG_DEBUG_MUTEXES > > + DEBUG_LOCKS_WARN_ON(ww->base.magic != &ww->base); > > +#endif > > + > > + preempt_disable(); > > + locked = __mutex_trylock(&ww->base); > > + > > + if (locked) { > > + ww_mutex_set_context_fastpath(ww, ctx); > > + mutex_acquire_nest(&ww->base.dep_map, 0, 1, &ctx->dep_map, _RET_IP_); > > + } > > + preempt_enable(); > > + > > + return locked; > > +} > > +EXPORT_SYMBOL(ww_mutex_trylock); > > You'll need a similar hunk in ww_rt_mutex.c -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
next prev parent reply other threads:[~2021-09-08 18:31 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-07 13:20 [PATCH] kernel/locking: Add context to ww_mutex_trylock Maarten Lankhorst 2021-09-07 13:20 ` [Intel-gfx] " Maarten Lankhorst 2021-09-07 13:54 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork 2021-09-07 14:27 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2021-09-07 14:59 ` [PATCH] " kernel test robot 2021-09-07 14:59 ` kernel test robot 2021-09-07 14:59 ` [Intel-gfx] " kernel test robot 2021-09-07 17:28 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 2021-09-08 10:14 ` [PATCH] " Peter Zijlstra 2021-09-08 10:14 ` [Intel-gfx] " Peter Zijlstra 2021-09-08 18:30 ` Daniel Vetter [this message] 2021-09-08 18:30 ` Daniel Vetter 2021-09-09 5:38 ` Maarten Lankhorst 2021-09-09 5:38 ` [Intel-gfx] " Maarten Lankhorst 2021-09-09 8:22 ` Peter Zijlstra 2021-09-09 8:22 ` [Intel-gfx] " Peter Zijlstra 2021-09-09 9:32 ` [PATCH v2] " Maarten Lankhorst 2021-09-09 9:32 ` [Intel-gfx] " Maarten Lankhorst 2021-09-10 15:02 ` Peter Zijlstra 2021-09-10 15:02 ` [Intel-gfx] " Peter Zijlstra 2021-09-10 17:27 ` Peter Zijlstra 2021-09-10 17:27 ` [Intel-gfx] " Peter Zijlstra 2021-09-13 8:42 ` Maarten Lankhorst 2021-09-13 8:42 ` [Intel-gfx] " Maarten Lankhorst 2021-09-14 6:50 ` Peter Zijlstra 2021-09-14 6:50 ` [Intel-gfx] " Peter Zijlstra 2021-09-14 12:43 ` Maarten Lankhorst 2021-09-14 12:43 ` [Intel-gfx] " Maarten Lankhorst 2021-09-14 13:54 ` Daniel Vetter 2021-09-14 13:54 ` [Intel-gfx] " Daniel Vetter 2021-09-16 13:00 ` Maarten Lankhorst 2021-09-16 13:00 ` [Intel-gfx] " Maarten Lankhorst 2021-09-16 13:28 ` Peter Zijlstra 2021-09-16 13:28 ` [Intel-gfx] " Peter Zijlstra 2021-09-17 13:13 ` Peter Zijlstra 2021-09-17 13:13 ` [Intel-gfx] " Peter Zijlstra 2021-09-20 15:02 ` Joonas Lahtinen 2021-09-20 15:02 ` [Intel-gfx] " Joonas Lahtinen 2021-09-17 13:17 ` [tip: locking/core] kernel/locking: Add context to ww_mutex_trylock() tip-bot2 for Maarten Lankhorst 2021-11-04 12:27 ` [PATCH] kernel/locking: Use a pointer in ww_mutex_trylock() Sebastian Andrzej Siewior 2021-11-04 12:27 ` [Intel-gfx] " Sebastian Andrzej Siewior 2021-11-04 12:27 ` Sebastian Andrzej Siewior 2021-11-17 13:59 ` [tip: locking/core] " tip-bot2 for Sebastian Andrzej Siewior 2021-09-10 18:06 ` [PATCH v2] kernel/locking: Add context to ww_mutex_trylock Mark Brown 2021-09-10 18:06 ` [Intel-gfx] " Mark Brown 2021-09-09 9:50 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for kernel/locking: Add context to ww_mutex_trylock. (rev2) Patchwork 2021-09-10 15:36 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for kernel/locking: Add context to ww_mutex_trylock. (rev3) Patchwork 2021-09-14 9:31 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for kernel/locking: Add context to ww_mutex_trylock. (rev4) Patchwork 2021-09-14 9:58 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 2021-11-04 16:00 ` [Intel-gfx] ✓ Fi.CI.BAT: success for kernel/locking: Add context to ww_mutex_trylock. (rev5) Patchwork 2021-11-04 18:15 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork 2021-09-09 11:02 [PATCH] kernel/locking: Add context to ww_mutex_trylock kernel test robot 2021-09-14 3:49 ` kernel test robot 2021-09-14 3:49 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YTkBXhu1QRxfqq1R@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=boqun.feng@gmail.com \ --cc=broonie@kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=longman@redhat.com \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.