| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAF6AEGtwNrhx788AOnvFNmQXKUvBAaW8qx+8BcTXmdQuc1SD8g@mail.gmail.com>
Date: Sat, 19 Nov 2016 13:55:03 -0500
From: Rob Clark <robdclark@...il.com>
To: hpa@...or.com, terry.rudd@....com, jason.low2@....com,
Peter Zijlstra <peterz@...radead.org>,
David Airlie <airlied@...ux.ie>, dave@...olabs.net,
Chris Wilson <chris@...is-wilson.co.uk>,
Imre Deak <imre.deak@...el.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Ingo Molnar <mingo@...nel.org>, tim.c.chen@...ux.intel.com,
tglx@...utronix.de, Rob Clark <robdclark@...il.com>,
Will Deacon <Will.Deacon@....com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
dingtianhong@...wei.com, paulmck@...ibm.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Cc: linux-tip-commits@...r.kernel.org
Subject: Re: [tip:locking/core] locking/mutex, drm: Introduce mutex_trylock_recursive()
On Wed, Nov 16, 2016 at 3:37 AM, tip-bot for Peter Zijlstra
<tipbot@...or.com> wrote:
> Commit-ID: 0f5225b024d4bffd682aab008c35862e8fdc1865
> Gitweb: http://git.kernel.org/tip/0f5225b024d4bffd682aab008c35862e8fdc1865
> Author: Peter Zijlstra <peterz@...radead.org>
> AuthorDate: Fri, 7 Oct 2016 17:43:51 +0200
> Committer: Ingo Molnar <mingo@...nel.org>
> CommitDate: Tue, 15 Nov 2016 14:19:55 +0100
>
> locking/mutex, drm: Introduce mutex_trylock_recursive()
>
> 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.
btw, for the drm/msm part,
Acked-by: Rob Clark <robdclark@...il.com>
(and also for the following patch removing the deprecated warnings)
I would also like to rework the locking in drm/msm to be more fine
grained and not require this (as i915 plans).. I'm also just one
person with a lot else on my plate so I'm not sure I'll get around to
that quite as quickly as i915.. (otoh I'll be watching how i915
handles this, since I guess I should be able to follow the same
approach)
BR,
-R
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Cc: Chris Wilson <chris@...is-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: Ding Tianhong <dingtianhong@...wei.com>
> Cc: Imre Deak <imre.deak@...el.com>
> Cc: Jason Low <jason.low2@....com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Paul E. McKenney <paulmck@...ibm.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Rob Clark <robdclark@...il.com>
> Cc: Terry Rudd <terry.rudd@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Tim Chen <tim.c.chen@...ux.intel.com>
> Cc: Will Deacon <Will.Deacon@....com>
> Cc: dri-devel@...ts.freedesktop.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Ingo Molnar <mingo@...nel.org>
> ---
> 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 e9bd2a8..c450076 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 6d2e885..b77bca7 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 4d3bcca..6a902f0 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 a8368d1..23f462f6 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*\)/ ) {
Powered by blists - more mailing lists