Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-21 Thread Emil Velikov
On 20 February 2018 at 13:12, Peter Zijlstra  wrote:
> On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>> way of doing this is to check if the buffer object is locked with the ticket
>> of the current submission.
>>
>> Clean up the access to the ww_mutex internals by providing a function
>> for this and extend the check to the thread owning the underlying mutex.
>
>> Signed-off-by: Christian König 
>
> Much thanks for Cc'ing the relevant maintainers :/
>
Doubt it's intentional. The get-maintainer script seems confused and
lists no maintainers?

$ ./scripts/get_maintainer.pl include/linux/ww_mutex.h
linux-ker...@vger.kernel.org (open list)

While the normal mutex header works fine.

$ ./scripts/get_maintainer.pl include/linux/mutex.h
Peter Zijlstra  (maintainer:LOCKING PRIMITIVES)
Ingo Molnar  (maintainer:LOCKING PRIMITIVES)
linux-ker...@vger.kernel.org (open list:LOCKING PRIMITIVES)

HTH
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-21 Thread Maarten Lankhorst
Op 21-02-18 om 00:56 schreef Daniel Vetter:
> On Tue, Feb 20, 2018 at 04:21:58PM +0100, Peter Zijlstra wrote:
>> On Tue, Feb 20, 2018 at 04:05:49PM +0100, Christian König wrote:
>>> Am 20.02.2018 um 15:54 schrieb Peter Zijlstra:
 On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote:
>> OK, but neither case would in fact need the !ctx case right? That's just
>> there for completeness sake?
> Unfortunately not. TTM uses trylock to lock BOs which are about to be
> evicted to make room for all the BOs locked with a ctx.
>
> I need to be able to distinct between the BOs which are trylocked and 
> those
> which are locked with a ctx.
>
> Writing this I actually noticed the current version is buggy, cause even
> when we check the mutex owner we still need to make sure that the ctx in 
> the
> lock is NULL.
 Hurm... I can't remember why trylocks behave like that, and it seems
 rather unfortunate / inconsistent.
>>> Actually for me that is rather fortunate, cause I need to distinct between
>>> the locks acquired through trylock and lock.
>> I suppose that would always be possible using:
>> ww_mutex_trylock(.ctx=NULL), and it could be that there simply weren't
>> any immediate uses for a !NULL trylock and it was thus not implemented.
>>
>> But that is all very long ago..
> I think we simple never had a use-case for interleaving ww_mutex_lock(ctx)
> and ww_mutex_trylock(ctx). Nesting multiple trylocks in ctx-locks happens
> plenty, but not further:
>
> The common use-case for that is locking a bunch of buffers you need (for
> command submission or whatever), and then trylocking other buffers to make
> space for the buffers you need to move into VRAM. I guess if only
> trylocking buffers doesn't succeed in freeing up enough VRAM then we could
> go into blocking ww_mutex_locks which need the ctx (and which would need
> all the trylock-acquired buffers to be annotated with the ctx too). TTM
> currently tries to be far enough away from that corner case (using a
> defensive "never use more than 50% of all memory for gfx" approach) that
> it doesn't seem to need that.
>
> Once we get there it should indeed be simply to add a ctx parameter to
> ww_mutex_trylock to fix this case. The TTM side rework is definitely going
> to be the much bigger issue here ...
> -Daniel

Yes, I think fixing trylock to take a ctx parameter would be a better fix than 
ww_mutex_is_owned_by..

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-21 Thread Christian König

Am 21.02.2018 um 11:54 schrieb Maarten Lankhorst:

Op 21-02-18 om 00:56 schreef Daniel Vetter:

On Tue, Feb 20, 2018 at 04:21:58PM +0100, Peter Zijlstra wrote:

On Tue, Feb 20, 2018 at 04:05:49PM +0100, Christian König wrote:

Am 20.02.2018 um 15:54 schrieb Peter Zijlstra:

On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote:

OK, but neither case would in fact need the !ctx case right? That's just
there for completeness sake?

Unfortunately not. TTM uses trylock to lock BOs which are about to be
evicted to make room for all the BOs locked with a ctx.

I need to be able to distinct between the BOs which are trylocked and those
which are locked with a ctx.

Writing this I actually noticed the current version is buggy, cause even
when we check the mutex owner we still need to make sure that the ctx in the
lock is NULL.

Hurm... I can't remember why trylocks behave like that, and it seems
rather unfortunate / inconsistent.

Actually for me that is rather fortunate, cause I need to distinct between
the locks acquired through trylock and lock.

I suppose that would always be possible using:
ww_mutex_trylock(.ctx=NULL), and it could be that there simply weren't
any immediate uses for a !NULL trylock and it was thus not implemented.

But that is all very long ago..

I think we simple never had a use-case for interleaving ww_mutex_lock(ctx)
and ww_mutex_trylock(ctx). Nesting multiple trylocks in ctx-locks happens
plenty, but not further:

The common use-case for that is locking a bunch of buffers you need (for
command submission or whatever), and then trylocking other buffers to make
space for the buffers you need to move into VRAM. I guess if only
trylocking buffers doesn't succeed in freeing up enough VRAM then we could
go into blocking ww_mutex_locks which need the ctx (and which would need
all the trylock-acquired buffers to be annotated with the ctx too). TTM
currently tries to be far enough away from that corner case (using a
defensive "never use more than 50% of all memory for gfx" approach) that
it doesn't seem to need that.

Once we get there it should indeed be simply to add a ctx parameter to
ww_mutex_trylock to fix this case. The TTM side rework is definitely going
to be the much bigger issue here ...
-Daniel

Yes, I think fixing trylock to take a ctx parameter would be a better fix than 
ww_mutex_is_owned_by..


Yeah, but as I noted now multiple times that won't work.

See I need to distinct between the BOs acquired with and without a 
context. Otherwise the whole approach doesn't make much sense.


Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Daniel Vetter
On Tue, Feb 20, 2018 at 04:21:58PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 20, 2018 at 04:05:49PM +0100, Christian König wrote:
> > Am 20.02.2018 um 15:54 schrieb Peter Zijlstra:
> > > On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote:
> > > > > OK, but neither case would in fact need the !ctx case right? That's 
> > > > > just
> > > > > there for completeness sake?
> > > > Unfortunately not. TTM uses trylock to lock BOs which are about to be
> > > > evicted to make room for all the BOs locked with a ctx.
> > > > 
> > > > I need to be able to distinct between the BOs which are trylocked and 
> > > > those
> > > > which are locked with a ctx.
> > > > 
> > > > Writing this I actually noticed the current version is buggy, cause even
> > > > when we check the mutex owner we still need to make sure that the ctx 
> > > > in the
> > > > lock is NULL.
> > > Hurm... I can't remember why trylocks behave like that, and it seems
> > > rather unfortunate / inconsistent.
> > 
> > Actually for me that is rather fortunate, cause I need to distinct between
> > the locks acquired through trylock and lock.
> 
> I suppose that would always be possible using:
> ww_mutex_trylock(.ctx=NULL), and it could be that there simply weren't
> any immediate uses for a !NULL trylock and it was thus not implemented.
> 
> But that is all very long ago..

I think we simple never had a use-case for interleaving ww_mutex_lock(ctx)
and ww_mutex_trylock(ctx). Nesting multiple trylocks in ctx-locks happens
plenty, but not further:

The common use-case for that is locking a bunch of buffers you need (for
command submission or whatever), and then trylocking other buffers to make
space for the buffers you need to move into VRAM. I guess if only
trylocking buffers doesn't succeed in freeing up enough VRAM then we could
go into blocking ww_mutex_locks which need the ctx (and which would need
all the trylock-acquired buffers to be annotated with the ctx too). TTM
currently tries to be far enough away from that corner case (using a
defensive "never use more than 50% of all memory for gfx" approach) that
it doesn't seem to need that.

Once we get there it should indeed be simply to add a ctx parameter to
ww_mutex_trylock to fix this case. The TTM side rework is definitely going
to be the much bigger issue here ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Peter Zijlstra
On Tue, Feb 20, 2018 at 04:05:49PM +0100, Christian König wrote:
> Am 20.02.2018 um 15:54 schrieb Peter Zijlstra:
> > On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote:
> > > > OK, but neither case would in fact need the !ctx case right? That's just
> > > > there for completeness sake?
> > > Unfortunately not. TTM uses trylock to lock BOs which are about to be
> > > evicted to make room for all the BOs locked with a ctx.
> > > 
> > > I need to be able to distinct between the BOs which are trylocked and 
> > > those
> > > which are locked with a ctx.
> > > 
> > > Writing this I actually noticed the current version is buggy, cause even
> > > when we check the mutex owner we still need to make sure that the ctx in 
> > > the
> > > lock is NULL.
> > Hurm... I can't remember why trylocks behave like that, and it seems
> > rather unfortunate / inconsistent.
> 
> Actually for me that is rather fortunate, cause I need to distinct between
> the locks acquired through trylock and lock.

I suppose that would always be possible using:
ww_mutex_trylock(.ctx=NULL), and it could be that there simply weren't
any immediate uses for a !NULL trylock and it was thus not implemented.

But that is all very long ago..

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Peter Zijlstra
On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote:
> > OK, but neither case would in fact need the !ctx case right? That's just
> > there for completeness sake?
> 
> Unfortunately not. TTM uses trylock to lock BOs which are about to be
> evicted to make room for all the BOs locked with a ctx.
> 
> I need to be able to distinct between the BOs which are trylocked and those
> which are locked with a ctx.
> 
> Writing this I actually noticed the current version is buggy, cause even
> when we check the mutex owner we still need to make sure that the ctx in the
> lock is NULL.

Hurm... I can't remember why trylocks behave like that, and it seems
rather unfortunate / inconsistent.

Chris, Maarten, do either one of you remember?

I'm thinking that if we do acquire the trylock, the thing should join
the ctx such that a subsequent contending mutex_lock() can ww right.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Christian König

Am 20.02.2018 um 15:54 schrieb Peter Zijlstra:

On Tue, Feb 20, 2018 at 03:34:07PM +0100, Christian König wrote:

OK, but neither case would in fact need the !ctx case right? That's just
there for completeness sake?

Unfortunately not. TTM uses trylock to lock BOs which are about to be
evicted to make room for all the BOs locked with a ctx.

I need to be able to distinct between the BOs which are trylocked and those
which are locked with a ctx.

Writing this I actually noticed the current version is buggy, cause even
when we check the mutex owner we still need to make sure that the ctx in the
lock is NULL.

Hurm... I can't remember why trylocks behave like that, and it seems
rather unfortunate / inconsistent.


Actually for me that is rather fortunate, cause I need to distinct 
between the locks acquired through trylock and lock.


In other words when the amdgpu does it's checking if userspace sends 
nonsense to the kernel it should only get a green signal when the lock 
was intentionally locked using the context.


If the lock was just taken because TTM trylocked it because TTM had to 
evict the BO to make room then the check should fail and we need to tell 
userspace that it did something wrong.


Regards,
Christian.


Chris, Maarten, do either one of you remember?

I'm thinking that if we do acquire the trylock, the thing should join
the ctx such that a subsequent contending mutex_lock() can ww right.
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Christian König

Am 20.02.2018 um 14:57 schrieb Peter Zijlstra:

On Tue, Feb 20, 2018 at 02:26:55PM +0100, Christian König wrote:

+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+   struct ww_acquire_ctx *ctx)
+{
+   if (ctx)
+   return likely(READ_ONCE(lock->ctx) == ctx);
+   else
+   return likely(__mutex_owner(&lock->base) == current);
+}

Much better than the previous version. If you want to bike-shed, you can
leave out the 'else' and unindent the last line.

Thanks for the suggestion, going to do this.

You might also want likely(ctx), since ww_mutex without ctx is
a-typical I would think.


I do worry about potential users of .ctx = NULL, though. It makes it far
too easy to do recursive locking, which is something we should strongly
discourage.

Well, one of the addressed use cases is indeed checking for recursive
locking. But recursive locking is something rather normal for ww_mutex and
we are just exercising an existing code path.

But that would be the ctx case, right? I'm not sure there is a lot of
!ctx use out there, and in that case it really is rather like a normal
mutex.


E.g. the most common use case for the ww_mutex is in the graphics drivers
where usespace sends us a list of buffer objects to work with.

Now when userspace sends us duplicates in that buffer list the expectation
is to get -EALREADY from ww_mutex_lock when we try to lock the same ww_mutex
twice.

Right, I remember that much.. :-)


The intention behind this function is now to a) be able to extend those
checks to make sure user space doesn't sends us potentially harmful nonsense
and b) allow to check for recursion in TTM during buffer object eviction
which uses ww_mutex_trylock instead of ww_mutex_lock.

OK, but neither case would in fact need the !ctx case right? That's just
there for completeness sake?


Unfortunately not. TTM uses trylock to lock BOs which are about to be 
evicted to make room for all the BOs locked with a ctx.


I need to be able to distinct between the BOs which are trylocked and 
those which are locked with a ctx.


Writing this I actually noticed the current version is buggy, cause even 
when we check the mutex owner we still need to make sure that the ctx in 
the lock is NULL.


Time for v4 of the patch,
Christian.



But yes, I cannot think of a better fallback there either.



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Peter Zijlstra
On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
> 
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.

> Signed-off-by: Christian König 

Much thanks for Cc'ing the relevant maintainers :/

> ---
>  include/linux/ww_mutex.h | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..14e4149d3d9d 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex 
> *lock)
>   return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that 
> context
> + * @lock: the mutex to be queried
> + * @ctx: the w/w acquire context to test
> + *
> + * If @ctx is not NULL test if the mutex is owned by this context.
> + * If @ctx is NULL test if the mutex is owned by the current thread.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> + struct ww_acquire_ctx *ctx)
> +{
> + if (ctx)
> + return likely(READ_ONCE(lock->ctx) == ctx);
> + else
> + return likely(__mutex_owner(&lock->base) == current);
> +}

Much better than the previous version. If you want to bike-shed, you can
leave out the 'else' and unindent the last line.

I do worry about potential users of .ctx = NULL, though. It makes it far
too easy to do recursive locking, which is something we should strongly
discourage.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Peter Zijlstra
On Tue, Feb 20, 2018 at 02:26:55PM +0100, Christian König wrote:
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > + struct ww_acquire_ctx *ctx)
> > > +{
> > > + if (ctx)
> > > + return likely(READ_ONCE(lock->ctx) == ctx);
> > > + else
> > > + return likely(__mutex_owner(&lock->base) == current);
> > > +}
> > Much better than the previous version. If you want to bike-shed, you can
> > leave out the 'else' and unindent the last line.
> 
> Thanks for the suggestion, going to do this.

You might also want likely(ctx), since ww_mutex without ctx is
a-typical I would think.

> > I do worry about potential users of .ctx = NULL, though. It makes it far
> > too easy to do recursive locking, which is something we should strongly
> > discourage.
> 
> Well, one of the addressed use cases is indeed checking for recursive
> locking. But recursive locking is something rather normal for ww_mutex and
> we are just exercising an existing code path.

But that would be the ctx case, right? I'm not sure there is a lot of
!ctx use out there, and in that case it really is rather like a normal
mutex.

> E.g. the most common use case for the ww_mutex is in the graphics drivers
> where usespace sends us a list of buffer objects to work with.
> 
> Now when userspace sends us duplicates in that buffer list the expectation
> is to get -EALREADY from ww_mutex_lock when we try to lock the same ww_mutex
> twice.

Right, I remember that much.. :-)

> The intention behind this function is now to a) be able to extend those
> checks to make sure user space doesn't sends us potentially harmful nonsense
> and b) allow to check for recursion in TTM during buffer object eviction
> which uses ww_mutex_trylock instead of ww_mutex_lock.

OK, but neither case would in fact need the !ctx case right? That's just
there for completeness sake?

But yes, I cannot think of a better fallback there either.

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Daniel Vetter
On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
> 
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
> 
> v2: split amdgpu changes into separate patch as suggested by Alex
> v3: change logic as suggested by Daniel
> 
> Signed-off-by: Christian König 
> ---
>  include/linux/ww_mutex.h | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..14e4149d3d9d 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex 
> *lock)
>   return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that 
> context
> + * @lock: the mutex to be queried
> + * @ctx: the w/w acquire context to test
> + *
> + * If @ctx is not NULL test if the mutex is owned by this context.
> + * If @ctx is NULL test if the mutex is owned by the current thread.

This is a bit much how and not so much why, but I couldn't come up with a
concise text what was materially better.

Reviewed-by: Daniel Vetter 

> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> + struct ww_acquire_ctx *ctx)
> +{
> + if (ctx)
> + return likely(READ_ONCE(lock->ctx) == ctx);
> + else
> + return likely(__mutex_owner(&lock->base) == current);
> +}
> +
>  #endif
> -- 
> 2.14.1
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


Re: [PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Christian König

Am 20.02.2018 um 14:12 schrieb Peter Zijlstra:

On Tue, Feb 20, 2018 at 01:58:26PM +0100, Christian König wrote:

amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.

Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.
Signed-off-by: Christian König 

Much thanks for Cc'ing the relevant maintainers :/


Sorry for that.


---
  include/linux/ww_mutex.h | 17 +
  1 file changed, 17 insertions(+)

diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..14e4149d3d9d 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex 
*lock)
return mutex_is_locked(&lock->base);
  }
  
+/**

+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @ctx: the w/w acquire context to test
+ *
+ * If @ctx is not NULL test if the mutex is owned by this context.
+ * If @ctx is NULL test if the mutex is owned by the current thread.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+   struct ww_acquire_ctx *ctx)
+{
+   if (ctx)
+   return likely(READ_ONCE(lock->ctx) == ctx);
+   else
+   return likely(__mutex_owner(&lock->base) == current);
+}

Much better than the previous version. If you want to bike-shed, you can
leave out the 'else' and unindent the last line.


Thanks for the suggestion, going to do this.


I do worry about potential users of .ctx = NULL, though. It makes it far
too easy to do recursive locking, which is something we should strongly
discourage.


Well, one of the addressed use cases is indeed checking for recursive 
locking. But recursive locking is something rather normal for ww_mutex 
and we are just exercising an existing code path.


E.g. the most common use case for the ww_mutex is in the graphics 
drivers where usespace sends us a list of buffer objects to work with.


Now when userspace sends us duplicates in that buffer list the 
expectation is to get -EALREADY from ww_mutex_lock when we try to lock 
the same ww_mutex twice.


Depending on the driver this then results in returning an error code to 
userspace or just ignoring the duplicate (because of backward 
compatibility).



The intention behind this function is now to a) be able to extend those 
checks to make sure user space doesn't sends us potentially harmful 
nonsense and b) allow to check for recursion in TTM during buffer object 
eviction which uses ww_mutex_trylock instead of ww_mutex_lock.


Regards,
Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[PATCH 1/4] locking/ww_mutex: add ww_mutex_is_owned_by function v3

2018-02-20 Thread Christian König
amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.

Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.

v2: split amdgpu changes into separate patch as suggested by Alex
v3: change logic as suggested by Daniel

Signed-off-by: Christian König 
---
 include/linux/ww_mutex.h | 17 +
 1 file changed, 17 insertions(+)

diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..14e4149d3d9d 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex 
*lock)
return mutex_is_locked(&lock->base);
 }
 
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @ctx: the w/w acquire context to test
+ *
+ * If @ctx is not NULL test if the mutex is owned by this context.
+ * If @ctx is NULL test if the mutex is owned by the current thread.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+   struct ww_acquire_ctx *ctx)
+{
+   if (ctx)
+   return likely(READ_ONCE(lock->ctx) == ctx);
+   else
+   return likely(__mutex_owner(&lock->base) == current);
+}
+
 #endif
-- 
2.14.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx