| Message ID | 20180220125829.27060-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <christian.koenig@amd.com> 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.
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 <christian.koenig@amd.com> > 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.
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.
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 <christian.koenig@amd.com> > --- > 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 <daniel.vetter@ffwll.ch> > + */ > +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-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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. >
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.
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-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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..
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
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..
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.
On 20 February 2018 at 13:12, Peter Zijlstra <peterz@infradead.org> 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 <christian.koenig@amd.com> > > 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-kernel@vger.kernel.org (open list) While the normal mutex header works fine. $ ./scripts/get_maintainer.pl include/linux/mutex.h Peter Zijlstra <peterz@infradead.org> (maintainer:LOCKING PRIMITIVES) Ingo Molnar <mingo@redhat.com> (maintainer:LOCKING PRIMITIVES) linux-kernel@vger.kernel.org (open list:LOCKING PRIMITIVES) HTH Emil
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
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 <christian.koenig@amd.com> --- include/linux/ww_mutex.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)