Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
* Peter Zijlstra wrote: > On Sat, Nov 12, 2016 at 11:58:49AM +0100, Ingo Molnar wrote: > > > Could you send a final version of trylock_recursive() patch for me to apply? > > The latest lives here: > > lkml.kernel.org/r/20161109103813.gn3...@twins.programming.kicks-ass.net > > But I would really like someone to actually test that before you stick > it in. Agreed ... Thanks, Ingo
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Sat, Nov 12, 2016 at 11:58:49AM +0100, Ingo Molnar wrote: > Could you send a final version of trylock_recursive() patch for me to apply? The latest lives here: lkml.kernel.org/r/20161109103813.gn3...@twins.programming.kicks-ass.net But I would really like someone to actually test that before you stick it in.
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
* Peter Zijlstra wrote: > On Fri, Nov 11, 2016 at 12:22:02PM +0100, Daniel Vetter wrote: > > > Once all your locking rework is assembled it might be good to have a > > topic branch I could pull in. Both for testing and to handle conflicts > > before it goes boom in the merge window ;-) Not necessary ofc, but I > > think it'd be useful. > > Everything except the trylock_recursive is already in tip/locking/core. > > Ingo, is that all there is in that branch? It has other bits as well - but I can create a separate topic branch for this, which would be c7faee2109f9 plus trylock_recursive(). Could you send a final version of trylock_recursive() patch for me to apply? Thanks, Ingo
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, Nov 11, 2016 at 12:22:02PM +0100, Daniel Vetter wrote: > Once all your locking rework is assembled it might be good to have a > topic branch I could pull in. Both for testing and to handle conflicts > before it goes boom in the merge window ;-) Not necessary ofc, but I > think it'd be useful. Everything except the trylock_recursive is already in tip/locking/core. Ingo, is that all there is in that branch?
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Tue, Oct 18, 2016 at 2:57 PM, Peter Zijlstra wrote:
> On Tue, Oct 18, 2016 at 02:48:41PM +0200, Peter Zijlstra wrote:
>> On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote:
>> > Poking at lock internals is not cool. Since I'm going to change the
>> > implementation this will break, take it out.
>> >
>> > Cc: Daniel Vetter
>> > Cc: Chris Wilson
>> > Cc: Rob Clark
>> > Signed-off-by: Peter Zijlstra (Intel)
>> > ---
>> > drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++---
>> > drivers/gpu/drm/msm/msm_gem_shrinker.c | 23 +++
>> > 2 files changed, 6 insertions(+), 43 deletions(-)
>>
>> OK, so it appears that i915 changed their locking around and got rid of
>> this thing entirely. Much appreciated Chris!!
>
> Hmm, I might have spoken too soon. My patch conflicted and I seem to
> have read too much in the Changelog of 3b4e896f14b1 ("drm/i915: Remove
> unused no-shrinker-steal").
Once all your locking rework is assembled it might be good to have a
topic branch I could pull in. Both for testing and to handle conflicts
before it goes boom in the merge window ;-) Not necessary ofc, but I
think it'd be useful.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, Oct 07, 2016 at 05:43:51PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote:
> > Poking at lock internals is not cool. Since I'm going to change the
> > implementation this will break, take it out.
>
>
> So something like the below would serve as a replacement for your
> previous hacks. Is this API something acceptable to people?
Compile tested only.. Daniel reminded me in another thread.
---
Subject: locking/mutex,drm: Introduce mutex_trylock_recursive()
From: Peter Zijlstra
Date: Fri, 7 Oct 2016 17:43:51 +0200
By popular DRM demand, introduce mutex_trylock_recursive() to fix up the
two GEM users.
Without this it is very easy for these drivers to get stuck in
low-memory situations and trigger OOM. Work is in progress to remove the
need for this in at least i915.
Cc: Daniel Vetter
Cc: Chris Wilson
Cc: Rob Clark
Signed-off-by: Peter Zijlstra (Intel)
---
drivers/gpu/drm/i915/i915_gem_shrinker.c | 15 ---
drivers/gpu/drm/msm/msm_gem_shrinker.c | 16
include/linux/mutex.h| 31 +++
scripts/checkpatch.pl| 6 ++
4 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c
b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index e9bd2a81d03a..5543d993a50e 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -227,11 +227,20 @@ unsigned long i915_gem_shrink_all(struct drm_i915_private
*dev_priv)
static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
{
- if (!mutex_trylock(&dev->struct_mutex))
+ switch (mutex_trylock_recursive(&dev->struct_mutex)) {
+ case MUTEX_TRYLOCK_FAILED:
return false;
- *unlock = true;
- return true;
+ case MUTEX_TRYLOCK_SUCCESS:
+ *unlock = true;
+ return true;
+
+ case MUTEX_TRYLOCK_RECURSIVE:
+ *unlock = false;
+ return true;
+ }
+
+ BUG();
}
static unsigned long
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index 6d2e885bd58e..62b8cc653823 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -20,13 +20,21 @@
static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
{
- if (!mutex_trylock(&dev->struct_mutex))
+ switch (mutex_trylock_recursive(&dev->struct_mutex)) {
+ case MUTEX_TRYLOCK_FAILED:
return false;
- *unlock = true;
- return true;
-}
+ case MUTEX_TRYLOCK_SUCCESS:
+ *unlock = true;
+ return true;
+ case MUTEX_TRYLOCK_RECURSIVE:
+ *unlock = false;
+ return true;
+ }
+
+ BUG();
+}
static unsigned long
msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 4d3bccabbea5..6a902f0a2148 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -189,4 +189,35 @@ extern void mutex_unlock(struct mutex *lock);
extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
+/*
+ * These values are chosen such that FAIL and SUCCESS match the
+ * values of the regular mutex_trylock().
+ */
+enum mutex_trylock_recursive_enum {
+ MUTEX_TRYLOCK_FAILED= 0,
+ MUTEX_TRYLOCK_SUCCESS = 1,
+ MUTEX_TRYLOCK_RECURSIVE,
+};
+
+/**
+ * mutex_trylock_recursive - trylock variant that allows recursive locking
+ * @lock: mutex to be locked
+ *
+ * This function should not be used, _ever_. It is purely for hysterical GEM
+ * raisins, and once those are gone this will be removed.
+ *
+ * Returns:
+ * MUTEX_TRYLOCK_FAILED- trylock failed,
+ * MUTEX_TRYLOCK_SUCCESS - lock acquired,
+ * MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
+ */
+static inline __deprecated __must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock)
+{
+ if (unlikely(__mutex_owner(lock) == current))
+ return MUTEX_TRYLOCK_RECURSIVE;
+
+ return mutex_trylock(lock);
+}
+
#endif /* __LINUX_MUTEX_H */
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a8368d1c4348..23f462f64a3f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6076,6 +6076,12 @@ sub process {
}
}
+# check for mutex_trylock_recursive usage
+ if ($line =~ /mutex_trylock_recursive/) {
+ ERROR("LOCKING",
+ "recursive locking is bad, do not use this
ever.\n" . $herecurr);
+ }
+
# check for lockdep_set_novalidate_class
if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
$line =~ /__lockdep_no_validate__\s*\)/ ) {
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Tue, Oct 18, 2016 at 02:48:41PM +0200, Peter Zijlstra wrote: > On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote: > > Poking at lock internals is not cool. Since I'm going to change the > > implementation this will break, take it out. > > > > Cc: Daniel Vetter > > Cc: Chris Wilson > > Cc: Rob Clark > > Signed-off-by: Peter Zijlstra (Intel) > > --- > > drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++--- > > drivers/gpu/drm/msm/msm_gem_shrinker.c | 23 +++ > > 2 files changed, 6 insertions(+), 43 deletions(-) > > OK, so it appears that i915 changed their locking around and got rid of > this thing entirely. Much appreciated Chris!! It's not dead yet! Sorry. It's close though, in the next cycle we may be at a point where we don't rely on recursion locking in the shrinker. -Chris -- Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Tue, Oct 18, 2016 at 02:48:41PM +0200, Peter Zijlstra wrote:
> On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote:
> > Poking at lock internals is not cool. Since I'm going to change the
> > implementation this will break, take it out.
> >
> > Cc: Daniel Vetter
> > Cc: Chris Wilson
> > Cc: Rob Clark
> > Signed-off-by: Peter Zijlstra (Intel)
> > ---
> > drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++---
> > drivers/gpu/drm/msm/msm_gem_shrinker.c | 23 +++
> > 2 files changed, 6 insertions(+), 43 deletions(-)
>
> OK, so it appears that i915 changed their locking around and got rid of
> this thing entirely. Much appreciated Chris!!
Hmm, I might have spoken too soon. My patch conflicted and I seem to
have read too much in the Changelog of 3b4e896f14b1 ("drm/i915: Remove
unused no-shrinker-steal").
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote: > Poking at lock internals is not cool. Since I'm going to change the > implementation this will break, take it out. > > Cc: Daniel Vetter > Cc: Chris Wilson > Cc: Rob Clark > Signed-off-by: Peter Zijlstra (Intel) > --- > drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++--- > drivers/gpu/drm/msm/msm_gem_shrinker.c | 23 +++ > 2 files changed, 6 insertions(+), 43 deletions(-) OK, so it appears that i915 changed their locking around and got rid of this thing entirely. Much appreciated Chris!! Rob, is there any chance you can do the same for msm? Not having to provide a replacement function and opening the door to recursive locking would be ever so good.
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Sat, Oct 08, 2016 at 04:11:25PM +0200, Thomas Gleixner wrote: > Well, when you add just trylock_recursive then people are going to use it > anyway no matter whether it is easy or not. > > So if we decide to provide something which supports recursive locking for > mutexes then we are better off doing it with a proper set of functions and > not just a single undebugable wrapper. So ideally I'd say, no recursive stuff at all. But that means the GEM people need to either do custom hacks (and you know that will spread), or need to be convinced to rework their locking somehow.
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Sat, Oct 08, 2016 at 01:58:07PM +0200, Thomas Gleixner wrote: > Hmm. I'm not a great fan of this, because that requires an conditional > unlock mechanism. > >res = trylock_recursive(lock); >if (res == FAILED) > goto out; >. > >if (res == SUCCESS) > unlock(lock); > > While if you actually keep track of recursion you can do: > > if (!trylock_recursive(lock)) > goto out; > > > > unlock_recursive(lock); > > or even: > > lock_recursive(lock); > > unlock_recursive(lock); > > That's making lock/trylock and unlock symetric, so its obvious in the > source what's going on and the recursion tracking allows for better > debugability. Hurm,. so I thought that in general we disliked recursive locking because it quickly turns in to a horrible mess. Adding such primitives makes it 'easy' to use recursive locking and then where does it stop?
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Sat, 8 Oct 2016, Peter Zijlstra wrote: > On Sat, Oct 08, 2016 at 01:58:07PM +0200, Thomas Gleixner wrote: > > Hmm. I'm not a great fan of this, because that requires an conditional > > unlock mechanism. > > > >res = trylock_recursive(lock); > >if (res == FAILED) > >goto out; > >. > > > >if (res == SUCCESS) > >unlock(lock); > > > > While if you actually keep track of recursion you can do: > > > > if (!trylock_recursive(lock)) > > goto out; > > > > > > > > unlock_recursive(lock); > > > > or even: > > > > lock_recursive(lock); > > > > unlock_recursive(lock); > > > > That's making lock/trylock and unlock symetric, so its obvious in the > > source what's going on and the recursion tracking allows for better > > debugability. > > Hurm,. so I thought that in general we disliked recursive locking > because it quickly turns in to a horrible mess. > > Adding such primitives makes it 'easy' to use recursive locking and then > where does it stop? Well, when you add just trylock_recursive then people are going to use it anyway no matter whether it is easy or not. So if we decide to provide something which supports recursive locking for mutexes then we are better off doing it with a proper set of functions and not just a single undebugable wrapper. Thanks, tglx
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, 7 Oct 2016, Peter Zijlstra wrote:
> On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote:
> > Poking at lock internals is not cool. Since I'm going to change the
> > implementation this will break, take it out.
>
>
> So something like the below would serve as a replacement for your
> previous hacks. Is this API something acceptable to people? Ingo,
> Thomas?
>
> ---
> include/linux/mutex.h | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 4d3bccabbea5..afcff2c85957 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -189,4 +189,29 @@ extern void mutex_unlock(struct mutex *lock);
>
> extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
>
> +enum mutex_trylock_recursive_enum {
> + mutex_trylock_failed = 0,
> + mutex_trylock_success = 1,
> + mutex_trylock_recursive,
Upper case enum symbols please, if at all.
> +};
> +
> +/**
> + * mutex_trylock_recursive - trylock variant that allows recursive locking
> + * @lock: mutex to be locked
> + *
> + *
> + * Returns:
> + * mutex_trylock_failed- trylock failed,
> + * mutex_trylock_success - lock acquired,
> + * mutex_trylock_recursive - we already owned the lock.
> + */
> +static inline enum mutex_trylock_recursive_enum
> +mutex_trylock_recursive(struct mutex *lock)
> +{
> + if (unlikely(__mutex_owner(lock) == current))
> + return mutex_trylock_recursive;
> +
> + return mutex_trylock(lock);
> +}
Hmm. I'm not a great fan of this, because that requires an conditional
unlock mechanism.
res = trylock_recursive(lock);
if (res == FAILED)
goto out;
.
if (res == SUCCESS)
unlock(lock);
While if you actually keep track of recursion you can do:
if (!trylock_recursive(lock))
goto out;
unlock_recursive(lock);
or even:
lock_recursive(lock);
unlock_recursive(lock);
That's making lock/trylock and unlock symetric, so its obvious in the
source what's going on and the recursion tracking allows for better
debugability.
Thanks,
tglx
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On 10/07/2016 12:13 PM, Peter Zijlstra wrote: On Fri, Oct 07, 2016 at 08:58:43AM -0700, Linus Torvalds wrote: The other choice would be to just make the choices be negative (== recursive), zero (== failed) or positive (== got lock), which allows for the same value re-use for the non-recursive case, and you could avoid the enum entirely. I thought about that, but liked the enum better for having to then spell it out. I'll go make the enum shout and add comment as you suggest. I like the idea of having a tri-state returned value (<0, 0, >0). I don't mind having the enum, but just making mutex_trylock_recursive equal to -1 will be great. Cheers, Longman
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, Oct 07, 2016 at 08:58:43AM -0700, Linus Torvalds wrote: > Ugh. I think the concept is fine, but can we place make these enum's > be all upper case or something to make them really stand out visually. OK. > The other choice would be to just make the choices be negative (== > recursive), zero (== failed) or positive (== got lock), which allows > for the same value re-use for the non-recursive case, and you could > avoid the enum entirely. I thought about that, but liked the enum better for having to then spell it out. I'll go make the enum shout and add comment as you suggest.
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, Oct 7, 2016 at 8:43 AM, Peter Zijlstra wrote: > > So something like the below would serve as a replacement for your > previous hacks. Is this API something acceptable to people? Ingo, > Thomas? Ugh. I think the concept is fine, but can we place make these enum's be all upper case or something to make them really stand out visually. Also, it's a bit subtle how this depends on the success/failure enums to just have the same values as the return value of the non-try mutex. So a comment about why that particular numbering is important above the enum definition, please? You _kind_ of imply that magic numbering requirementby explicitly using " = 0" and " = 1", but quite frankly, in many ways that's really nasty, because I can see somebody looking at that enum definition and saying "that makes no sense: it sets *two* of the three values explicitly, and the values it sets explicitly are the same that it would get implicitly _anyway_, so I'll just clean this up". So I really think that enum needs a comment on the forced choice of values, and making them all caps would make the users more obvious, I think. The other choice would be to just make the choices be negative (== recursive), zero (== failed) or positive (== got lock), which allows for the same value re-use for the non-recursive case, and you could avoid the enum entirely. Linus
Re: [PATCH -v4 1/8] locking/drm: Kill mutex trickery
On Fri, Oct 07, 2016 at 04:52:44PM +0200, Peter Zijlstra wrote:
> Poking at lock internals is not cool. Since I'm going to change the
> implementation this will break, take it out.
So something like the below would serve as a replacement for your
previous hacks. Is this API something acceptable to people? Ingo,
Thomas?
---
include/linux/mutex.h | 25 +
1 file changed, 25 insertions(+)
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 4d3bccabbea5..afcff2c85957 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -189,4 +189,29 @@ extern void mutex_unlock(struct mutex *lock);
extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
+enum mutex_trylock_recursive_enum {
+ mutex_trylock_failed = 0,
+ mutex_trylock_success = 1,
+ mutex_trylock_recursive,
+};
+
+/**
+ * mutex_trylock_recursive - trylock variant that allows recursive locking
+ * @lock: mutex to be locked
+ *
+ *
+ * Returns:
+ * mutex_trylock_failed- trylock failed,
+ * mutex_trylock_success - lock acquired,
+ * mutex_trylock_recursive - we already owned the lock.
+ */
+static inline enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock)
+{
+ if (unlikely(__mutex_owner(lock) == current))
+ return mutex_trylock_recursive;
+
+ return mutex_trylock(lock);
+}
+
#endif /* __LINUX_MUTEX_H */
