* Re: [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery
@ 2016-08-26 3:25 Andreas Mohr
2016-08-26 9:10 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Mohr @ 2016-08-26 3:25 UTC (permalink / raw)
To: Waiman Long; +Cc: Daniel Vetter, Peter Zijlstra, linux-kernel
Hi,
[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]
> > But initerim I guess we could set our own owner field and check that
> > to keep the duct-tape from getting off completely.
> > -Daniel
>
> Another alternative is to provide a standard mutex API that returns the
> owner of the lock if there is a real need for this capability. Peeking
> into lock internal is not a good practice.
>From personal experience here I would suggest that
the core issue here is that
this would create an inherently race-window-tainted API,
which clearly is something to be avoided:
The point is that the lock *owner* value is *volatile*
whenever it is *not* our own context instance
that is currently holding the lock
while querying this API
(i.e., thus not guaranteeing that the owner value will *not* be changed interim!),
since in such a situation
(not-privately-locked case!!)
lock ownership may change at any point from under
our just-observed result value.
Returning such inherently racy information from a publicly offered mutex API
is, errrrr, not so good, to put it rather mildly.
So, it seems the most we could provide
which would offer a reliable, non-racy API protocol
is something like:
static bool mutex_is_locked_by_us(struct mutex *mutex)
since during execution of this processing it would be guaranteed that:
- either we do have the lock, thus *we* *RELIABLY* are and will be "the owner"
- or we simply do not have it, thus *we* *RELIABLY* are and will be "not the owner"
[but note that in that case
this mutex API implementation code would have
a potentially unholy dependency on task stuff such as "current",
but which it probably already has anyway]
HTH,
Andreas Mohr
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery
2016-08-26 3:25 [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery Andreas Mohr
@ 2016-08-26 9:10 ` Peter Zijlstra
2016-08-26 14:33 ` Waiman Long
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2016-08-26 9:10 UTC (permalink / raw)
To: Andreas Mohr; +Cc: Waiman Long, Daniel Vetter, linux-kernel
On Fri, Aug 26, 2016 at 05:25:09AM +0200, Andreas Mohr wrote:
> > Another alternative is to provide a standard mutex API that returns the
> > owner of the lock if there is a real need for this capability. Peeking
> > into lock internal is not a good practice.
> So, it seems the most we could provide which would offer a reliable,
> non-racy API protocol is something like:
>
> static bool mutex_is_locked_by_us(struct mutex *mutex)
>
> since during execution of this processing it would be guaranteed that:
> - either we do have the lock, thus *we* *RELIABLY* are and will be "the owner"
> - or we simply do not have it, thus *we* *RELIABLY* are and will be "not the owner"
Right, and that is exactly what they attempted and need. And the new
mutex implementation could actually do this much better than the old
one.
But yes, such an interface should be part of the mutex implementation
proper, not something hacked on in random places.
Fwiw, the build bot seems to have found another instance of this thing
:/ drivers/gpu/drm/msm/msm_gem_shrinker.c includes an exact copy.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery
2016-08-26 9:10 ` Peter Zijlstra
@ 2016-08-26 14:33 ` Waiman Long
0 siblings, 0 replies; 6+ messages in thread
From: Waiman Long @ 2016-08-26 14:33 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Andreas Mohr, Daniel Vetter, linux-kernel
On 08/26/2016 05:10 AM, Peter Zijlstra wrote:
> On Fri, Aug 26, 2016 at 05:25:09AM +0200, Andreas Mohr wrote:
>>> Another alternative is to provide a standard mutex API that returns the
>>> owner of the lock if there is a real need for this capability. Peeking
>>> into lock internal is not a good practice.
>
>> So, it seems the most we could provide which would offer a reliable,
>> non-racy API protocol is something like:
>>
>> static bool mutex_is_locked_by_us(struct mutex *mutex)
>>
>> since during execution of this processing it would be guaranteed that:
>> - either we do have the lock, thus *we* *RELIABLY* are and will be "the owner"
>> - or we simply do not have it, thus *we* *RELIABLY* are and will be "not the owner"
> Right, and that is exactly what they attempted and need. And the new
> mutex implementation could actually do this much better than the old
> one.
>
> But yes, such an interface should be part of the mutex implementation
> proper, not something hacked on in random places.
It is what exactly I have in mind. The actual API implemented is subject
to negotiation. The important thing is that it has to be within the core
mutex code.
> Fwiw, the build bot seems to have found another instance of this thing
> :/ drivers/gpu/drm/msm/msm_gem_shrinker.c includes an exact copy.
This seems to be a new file that was introduced since 4.8.
Cheers,
Longman
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH -v2 0/4] locking/mutex: Rewrite basic mutex
@ 2016-08-25 18:37 Peter Zijlstra
2016-08-25 18:37 ` [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2016-08-25 18:37 UTC (permalink / raw)
To: Linus Torvalds, Waiman Long, Jason Low, Ding Tianhong,
Thomas Gleixner, Will Deacon, Ingo Molnar, Imre Deak,
Linux Kernel Mailing List, Davidlohr Bueso, Tim Chen, Terry Rudd,
Paul E. McKenney, Jason Low, Peter Zijlstra
Cc: Chris Wilson, Daniel Vetter
... might still eat your pets and set your house on fire ...
Passes reaim high_systime on 4 socket haswell..
4.8.0-rc3-00185-g9f55477 +2 +all
Forks Jobs/min Jobs/min Jobs/min
10 1040.00 1039.83 1039.83
20 1600.00 1599.79 1600.00
30 2127.27 2127.27 2127.03
40 2599.73 2599.46 2599.19
50 3088.50 3088.50 3088.50
60 3531.08 3531.08 3531.41
70 3969.83 3969.47 3969.83
80 4377.80 4377.80 4377.80
90 4797.95 4798.77 4798.77
100 5198.27 5198.27 5198.70
200 9038.89 9038.89 9040.20
300 12642.67 12643.52 12643.52
400 15970.31 15993.85 15994.87
500 19365.65 19339.24 19368.06
600 22537.92 22542.00 22513.53
700 25536.39 25653.37 25552.83
800 28673.18 28664.94 28671.53
900 31596.71 31685.85 31700.16
1000 34651.27 34643.57 34649.34
1100 37679.09 37693.57 37693.57
1200 40675.76 40675.76 40175.98
1300 43573.08 43575.42 42993.43
1400 46398.98 46406.37 46440.91
1500 48977.03 49203.60 48432.16
1600 51940.48 51970.23 51037.73
1700 54929.58 54646.61 54923.89
1800 57432.12 57538.04 57529.19
1900 60437.38 60434.30 60446.62
2000 62950.82 62973.05 61760.78
Still need to add spinning to the top waiter etc... lockdep is still
broken and there might still be races.
---
arch/alpha/include/asm/mutex.h | 9 -
arch/arc/include/asm/mutex.h | 18 --
arch/arm/include/asm/mutex.h | 21 --
arch/arm64/include/asm/Kbuild | 1 -
arch/avr32/include/asm/mutex.h | 9 -
arch/blackfin/include/asm/Kbuild | 1 -
arch/c6x/include/asm/mutex.h | 6 -
arch/cris/include/asm/mutex.h | 9 -
arch/frv/include/asm/mutex.h | 9 -
arch/h8300/include/asm/mutex.h | 9 -
arch/hexagon/include/asm/mutex.h | 8 -
arch/ia64/include/asm/mutex.h | 90 --------
arch/m32r/include/asm/mutex.h | 9 -
arch/m68k/include/asm/Kbuild | 1 -
arch/metag/include/asm/Kbuild | 1 -
arch/microblaze/include/asm/mutex.h | 1 -
arch/mips/include/asm/Kbuild | 1 -
arch/mn10300/include/asm/mutex.h | 16 --
arch/nios2/include/asm/mutex.h | 1 -
arch/openrisc/include/asm/mutex.h | 27 ---
arch/parisc/include/asm/Kbuild | 1 -
arch/powerpc/include/asm/mutex.h | 132 ------------
arch/s390/include/asm/mutex.h | 9 -
arch/score/include/asm/mutex.h | 6 -
arch/sh/include/asm/mutex-llsc.h | 109 ----------
arch/sh/include/asm/mutex.h | 12 --
arch/sparc/include/asm/Kbuild | 1 -
arch/tile/include/asm/Kbuild | 1 -
arch/um/include/asm/Kbuild | 1 -
arch/unicore32/include/asm/mutex.h | 20 --
arch/x86/include/asm/mutex.h | 5 -
arch/x86/include/asm/mutex_32.h | 110 ----------
arch/x86/include/asm/mutex_64.h | 127 -----------
arch/xtensa/include/asm/mutex.h | 9 -
drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +--
include/asm-generic/mutex-dec.h | 88 --------
include/asm-generic/mutex-null.h | 19 --
include/asm-generic/mutex-xchg.h | 120 -----------
include/asm-generic/mutex.h | 9 -
include/linux/mutex-debug.h | 24 ---
include/linux/mutex.h | 46 ++--
kernel/Kconfig.locks | 2 +-
kernel/locking/mutex-debug.c | 13 --
kernel/locking/mutex-debug.h | 10 -
kernel/locking/mutex.c | 347 +++++++++++++++----------------
kernel/locking/mutex.h | 26 ---
kernel/sched/core.c | 2 +-
47 files changed, 199 insertions(+), 1323 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery
2016-08-25 18:37 [RFC][PATCH -v2 0/4] locking/mutex: Rewrite basic mutex Peter Zijlstra
@ 2016-08-25 18:37 ` Peter Zijlstra
2016-08-25 19:36 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2016-08-25 18:37 UTC (permalink / raw)
To: Linus Torvalds, Waiman Long, Jason Low, Ding Tianhong,
Thomas Gleixner, Will Deacon, Ingo Molnar, Imre Deak,
Linux Kernel Mailing List, Davidlohr Bueso, Tim Chen, Terry Rudd,
Paul E. McKenney, Jason Low, Peter Zijlstra
Cc: Chris Wilson, Daniel Vetter
[-- Attachment #1: peterz-revert-drm-i915-brainmelt.patch --]
[-- Type: text/plain, Size: 1470 bytes --]
Poking at lock internals is not cool. Since I'm going to change the
implementation this will break, take it out.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++-----------------------
1 file changed, 3 insertions(+), 23 deletions(-)
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -35,19 +35,6 @@
#include "i915_drv.h"
#include "i915_trace.h"
-static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
-{
- if (!mutex_is_locked(mutex))
- return false;
-
-#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
- return mutex->owner == task;
-#else
- /* Since UP may be pre-empted, we cannot assume that we own the lock */
- return false;
-#endif
-}
-
static int num_vma_bound(struct drm_i915_gem_object *obj)
{
struct i915_vma *vma;
@@ -238,17 +225,10 @@ unsigned long i915_gem_shrink_all(struct
static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
{
- if (!mutex_trylock(&dev->struct_mutex)) {
- if (!mutex_is_locked_by(&dev->struct_mutex, current))
- return false;
-
- if (to_i915(dev)->mm.shrinker_no_lock_stealing)
- return false;
-
- *unlock = false;
- } else
- *unlock = true;
+ if (!mutex_trylock(&dev->struct_mutex))
+ return false;
+ *unlock = true;
return true;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery
2016-08-25 18:37 ` [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery Peter Zijlstra
@ 2016-08-25 19:36 ` Daniel Vetter
2016-08-25 19:59 ` Waiman Long
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2016-08-25 19:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Waiman Long, Jason Low, Ding Tianhong,
Thomas Gleixner, Will Deacon, Ingo Molnar, Imre Deak,
Linux Kernel Mailing List, Davidlohr Bueso, Tim Chen, Terry Rudd,
Paul E. McKenney, Jason Low, Chris Wilson
On Thu, Aug 25, 2016 at 8:37 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> Poking at lock internals is not cool. Since I'm going to change the
> implementation this will break, take it out.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
It's horrible, but we die without this in spurious oom. And we haven't
made much progress in recent years to throw out the locking scheme and
replace it by something else that doesn't need a recursive mutex.
But initerim I guess we could set our own owner field and check that
to keep the duct-tape from getting off completely.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem_shrinker.c | 26 +++-----------------------
> 1 file changed, 3 insertions(+), 23 deletions(-)
>
> --- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
> @@ -35,19 +35,6 @@
> #include "i915_drv.h"
> #include "i915_trace.h"
>
> -static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
> -{
> - if (!mutex_is_locked(mutex))
> - return false;
> -
> -#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
> - return mutex->owner == task;
> -#else
> - /* Since UP may be pre-empted, we cannot assume that we own the lock */
> - return false;
> -#endif
> -}
> -
> static int num_vma_bound(struct drm_i915_gem_object *obj)
> {
> struct i915_vma *vma;
> @@ -238,17 +225,10 @@ unsigned long i915_gem_shrink_all(struct
>
> static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
> {
> - if (!mutex_trylock(&dev->struct_mutex)) {
> - if (!mutex_is_locked_by(&dev->struct_mutex, current))
> - return false;
> -
> - if (to_i915(dev)->mm.shrinker_no_lock_stealing)
> - return false;
> -
> - *unlock = false;
> - } else
> - *unlock = true;
> + if (!mutex_trylock(&dev->struct_mutex))
> + return false;
>
> + *unlock = true;
> return true;
> }
>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery
2016-08-25 19:36 ` Daniel Vetter
@ 2016-08-25 19:59 ` Waiman Long
0 siblings, 0 replies; 6+ messages in thread
From: Waiman Long @ 2016-08-25 19:59 UTC (permalink / raw)
To: Daniel Vetter
Cc: Peter Zijlstra, Linus Torvalds, Jason Low, Ding Tianhong,
Thomas Gleixner, Will Deacon, Ingo Molnar, Imre Deak,
Linux Kernel Mailing List, Davidlohr Bueso, Tim Chen, Terry Rudd,
Paul E. McKenney, Jason Low, Chris Wilson
On 08/25/2016 03:36 PM, Daniel Vetter wrote:
> On Thu, Aug 25, 2016 at 8:37 PM, Peter Zijlstra<peterz@infradead.org> wrote:
>> Poking at lock internals is not cool. Since I'm going to change the
>> implementation this will break, take it out.
>>
>> Cc: Chris Wilson<chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter<daniel.vetter@ffwll.ch>
>> Signed-off-by: Peter Zijlstra (Intel)<peterz@infradead.org>
> It's horrible, but we die without this in spurious oom. And we haven't
> made much progress in recent years to throw out the locking scheme and
> replace it by something else that doesn't need a recursive mutex.
>
> But initerim I guess we could set our own owner field and check that
> to keep the duct-tape from getting off completely.
> -Daniel
Another alternative is to provide a standard mutex API that returns the
owner of the lock if there is a real need for this capability. Peeking
into lock internal is not a good practice.
Cheers,
Longman
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-26 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-26 3:25 [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery Andreas Mohr
2016-08-26 9:10 ` Peter Zijlstra
2016-08-26 14:33 ` Waiman Long
-- strict thread matches above, loose matches on Subject: below --
2016-08-25 18:37 [RFC][PATCH -v2 0/4] locking/mutex: Rewrite basic mutex Peter Zijlstra
2016-08-25 18:37 ` [RFC][PATCH -v2 1/4] locking/drm/i915: Kill mutex trickery Peter Zijlstra
2016-08-25 19:36 ` Daniel Vetter
2016-08-25 19:59 ` Waiman Long
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox