| Message ID | 20210118180334.43714-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/syncobj: make lockdep complain on WAIT_FOR_SUBMIT v2 | expand |
Hi, Just a comment about the comments: On 1/18/21 10:03 AM, Christian König wrote: > DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT can't be used when we hold locks > since we are basically waiting for userspace to do something. > > Holding a lock while doing so can trivial deadlock with page faults > etc... > > So make lockdep complain when a driver tries to do this. > > v2: Add lockdep_assert_none_held() macro. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_syncobj.c | 7 +++++++ > include/linux/lockdep.h | 5 +++++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 6e74e6745eca..f51458615158 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -387,6 +387,13 @@ int drm_syncobj_find_fence(struct drm_file *file_private, > if (!syncobj) > return -ENOENT; > > + /* Waiting for userspace with locks help is illegal cause that can held because > + * trivial deadlock with page faults for example. Make lockdep complain trivially > + * about it early on. > + */ > + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) > + lockdep_assert_none_held_once(); > + > *fence = drm_syncobj_fence_get(syncobj); > drm_syncobj_put(syncobj); > thanks.
On Mon, Jan 18, 2021 at 07:03:34PM +0100, Christian König wrote: > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 6e74e6745eca..f51458615158 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -387,6 +387,13 @@ int drm_syncobj_find_fence(struct drm_file *file_private, > if (!syncobj) > return -ENOENT; > > + /* Waiting for userspace with locks help is illegal cause that can > + * trivial deadlock with page faults for example. Make lockdep complain > + * about it early on. > + */ Egads, the cursed comment style is spreading :/ > + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) > + lockdep_assert_none_held_once(); > + Should this not be part of drm_syncobj_fence_add_wait() instead? Also, do you want to sprinkle might_sleep() around ? > *fence = drm_syncobj_fence_get(syncobj); > drm_syncobj_put(syncobj); > > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
Am 19.01.21 um 10:35 schrieb Peter Zijlstra: > On Mon, Jan 18, 2021 at 07:03:34PM +0100, Christian König wrote: > >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c >> index 6e74e6745eca..f51458615158 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -387,6 +387,13 @@ int drm_syncobj_find_fence(struct drm_file *file_private, >> if (!syncobj) >> return -ENOENT; >> >> + /* Waiting for userspace with locks help is illegal cause that can >> + * trivial deadlock with page faults for example. Make lockdep complain >> + * about it early on. >> + */ > Egads, the cursed comment style is spreading :/ > >> + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) >> + lockdep_assert_none_held_once(); >> + > Should this not be part of drm_syncobj_fence_add_wait() instead? drm_syncobj_fence_add_wait() is only called when the previous try of finding the fence wasn't successfully. If we want to check drivers for stupid behavior for the uncommon wait before signal case we need this much earlier. But I'm going to double check if drm_syncobj_fence_add_wait() isn't used elsewhere as well. > Also, do you want to sprinkle might_sleep() around ? Good point. Going to add that as well. Thanks, Christian. > >> *fence = drm_syncobj_fence_get(syncobj); >> drm_syncobj_put(syncobj); >> >> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
On Tue, Jan 19, 2021 at 10:46:53AM +0100, Christian König wrote: > But I'm going to double check if drm_syncobj_fence_add_wait() isn't used > elsewhere as well. drm_syncobj_array_wait_timeout() Which is why I asked.. :-)
Am 19.01.21 um 11:05 schrieb Peter Zijlstra: > On Tue, Jan 19, 2021 at 10:46:53AM +0100, Christian König wrote: >> But I'm going to double check if drm_syncobj_fence_add_wait() isn't used >> elsewhere as well. > drm_syncobj_array_wait_timeout() > > Which is why I asked.. :-) Ok, good point as well. But this isn't driver interface and rather IOCTL implementation. So if we hold any locks there it is our own stupidity. Going to add the appropriate annotation anyway. Thanks, Christian.
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 6e74e6745eca..f51458615158 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -387,6 +387,13 @@ int drm_syncobj_find_fence(struct drm_file *file_private, if (!syncobj) return -ENOENT; + /* Waiting for userspace with locks help is illegal cause that can + * trivial deadlock with page faults for example. Make lockdep complain + * about it early on. + */ + if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) + lockdep_assert_none_held_once(); + *fence = drm_syncobj_fence_get(syncobj); drm_syncobj_put(syncobj); diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index b9e9adec73e8..6eb117c0d0f3 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -310,6 +310,10 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); WARN_ON_ONCE(debug_locks && !lockdep_is_held(l)); \ } while (0) +#define lockdep_assert_none_held_once() do { \ + WARN_ON_ONCE(debug_locks && current->lockdep_depth); \ + } while (0) + #define lockdep_recursing(tsk) ((tsk)->lockdep_recursion) #define lockdep_pin_lock(l) lock_pin_lock(&(l)->dep_map) @@ -387,6 +391,7 @@ extern int lockdep_is_held(const void *); #define lockdep_assert_held_write(l) do { (void)(l); } while (0) #define lockdep_assert_held_read(l) do { (void)(l); } while (0) #define lockdep_assert_held_once(l) do { (void)(l); } while (0) +#define lockdep_assert_none_held_once() do { } while (0) #define lockdep_recursing(tsk) (0)
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT can't be used when we hold locks since we are basically waiting for userspace to do something. Holding a lock while doing so can trivial deadlock with page faults etc... So make lockdep complain when a driver tries to do this. v2: Add lockdep_assert_none_held() macro. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_syncobj.c | 7 +++++++ include/linux/lockdep.h | 5 +++++ 2 files changed, 12 insertions(+)