| Message ID | 20210816084855.75586-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Ditch the i915_gem_ww_ctx loop member | expand |
On Mon, 16 Aug 2021 at 09:49, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote: > > It's only used by the for_i915_gem_ww() macro and we can use > the (typically) on-stack _err variable in its place. > > While initially setting the _err variable to -EDEADLK to enter the > loop, we clear it before actually entering using fetch_and_zero() to > avoid empty loops or code not setting the _err variable running forever. > > Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++--------------- > 1 file changed, 8 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h > index f6b1a796667b..98348b1e6182 100644 > --- a/drivers/gpu/drm/i915/i915_gem_ww.h > +++ b/drivers/gpu/drm/i915/i915_gem_ww.h > @@ -7,12 +7,13 @@ > > #include <drm/drm_drv.h> > > +#include "i915_utils.h" > + > struct i915_gem_ww_ctx { > struct ww_acquire_ctx ctx; > struct list_head obj_list; > struct drm_i915_gem_object *contended; > - unsigned short intr; > - unsigned short loop; > + bool intr; > }; > > void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); > @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); > /* Internal functions used by the inlines! Don't use. */ > static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) > { > - ww->loop = 0; > if (err == -EDEADLK) { > err = i915_gem_ww_ctx_backoff(ww); > if (!err) > - ww->loop = 1; > + err = -EDEADLK; > } > > - if (!ww->loop) > + if (err != -EDEADLK) > i915_gem_ww_ctx_fini(ww); > > return err; > } > > -static inline void > -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr) > -{ > - i915_gem_ww_ctx_init(ww, intr); > - ww->loop = 1; > -} > - > -#define for_i915_gem_ww(_ww, _err, _intr) \ > - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \ > +#define for_i915_gem_ww(_ww, _err, _intr) \ > + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \ > + fetch_and_zero(&_err) == -EDEADLK; \ Doesn't this now hide "normal" errors, like say get_pages() returning -ENOSPC or so? > _err = __i915_gem_ww_fini(_ww, _err)) > - > #endif > -- > 2.31.1 >
On 8/16/21 3:25 PM, Matthew Auld wrote: > On Mon, 16 Aug 2021 at 09:49, Thomas Hellström > <thomas.hellstrom@linux.intel.com> wrote: >> It's only used by the for_i915_gem_ww() macro and we can use >> the (typically) on-stack _err variable in its place. >> >> While initially setting the _err variable to -EDEADLK to enter the >> loop, we clear it before actually entering using fetch_and_zero() to >> avoid empty loops or code not setting the _err variable running forever. >> >> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++--------------- >> 1 file changed, 8 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h >> index f6b1a796667b..98348b1e6182 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_ww.h >> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h >> @@ -7,12 +7,13 @@ >> >> #include <drm/drm_drv.h> >> >> +#include "i915_utils.h" >> + >> struct i915_gem_ww_ctx { >> struct ww_acquire_ctx ctx; >> struct list_head obj_list; >> struct drm_i915_gem_object *contended; >> - unsigned short intr; >> - unsigned short loop; >> + bool intr; >> }; >> >> void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); >> @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); >> /* Internal functions used by the inlines! Don't use. */ >> static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) >> { >> - ww->loop = 0; >> if (err == -EDEADLK) { >> err = i915_gem_ww_ctx_backoff(ww); >> if (!err) >> - ww->loop = 1; >> + err = -EDEADLK; >> } >> >> - if (!ww->loop) >> + if (err != -EDEADLK) >> i915_gem_ww_ctx_fini(ww); >> >> return err; >> } >> >> -static inline void >> -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr) >> -{ >> - i915_gem_ww_ctx_init(ww, intr); >> - ww->loop = 1; >> -} >> - >> -#define for_i915_gem_ww(_ww, _err, _intr) \ >> - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \ >> +#define for_i915_gem_ww(_ww, _err, _intr) \ >> + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \ >> + fetch_and_zero(&_err) == -EDEADLK; \ > Doesn't this now hide "normal" errors, like say get_pages() returning > -ENOSPC or so? Yes, good catch. We should either just clear the -EDEADLK case, or not clear the error at all.. /Thomas
Op 16-08-2021 om 15:30 schreef Thomas Hellström: > > On 8/16/21 3:25 PM, Matthew Auld wrote: >> On Mon, 16 Aug 2021 at 09:49, Thomas Hellström >> <thomas.hellstrom@linux.intel.com> wrote: >>> It's only used by the for_i915_gem_ww() macro and we can use >>> the (typically) on-stack _err variable in its place. >>> >>> While initially setting the _err variable to -EDEADLK to enter the >>> loop, we clear it before actually entering using fetch_and_zero() to >>> avoid empty loops or code not setting the _err variable running forever. >>> >>> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++--------------- >>> 1 file changed, 8 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h >>> index f6b1a796667b..98348b1e6182 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_ww.h >>> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h >>> @@ -7,12 +7,13 @@ >>> >>> #include <drm/drm_drv.h> >>> >>> +#include "i915_utils.h" >>> + >>> struct i915_gem_ww_ctx { >>> struct ww_acquire_ctx ctx; >>> struct list_head obj_list; >>> struct drm_i915_gem_object *contended; >>> - unsigned short intr; >>> - unsigned short loop; >>> + bool intr; >>> }; >>> >>> void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); >>> @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); >>> /* Internal functions used by the inlines! Don't use. */ >>> static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) >>> { >>> - ww->loop = 0; >>> if (err == -EDEADLK) { >>> err = i915_gem_ww_ctx_backoff(ww); >>> if (!err) >>> - ww->loop = 1; >>> + err = -EDEADLK; >>> } >>> >>> - if (!ww->loop) >>> + if (err != -EDEADLK) >>> i915_gem_ww_ctx_fini(ww); >>> >>> return err; >>> } >>> >>> -static inline void >>> -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr) >>> -{ >>> - i915_gem_ww_ctx_init(ww, intr); >>> - ww->loop = 1; >>> -} >>> - >>> -#define for_i915_gem_ww(_ww, _err, _intr) \ >>> - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \ >>> +#define for_i915_gem_ww(_ww, _err, _intr) \ >>> + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \ >>> + fetch_and_zero(&_err) == -EDEADLK; \ >> Doesn't this now hide "normal" errors, like say get_pages() returning >> -ENOSPC or so? > > Yes, good catch. We should either just clear the -EDEADLK case, or not clear the error at all.. > > /Thomas I believe not setting _err is a bug anyway. Why would you do such a loop without at least one err = ww_mutex_lock(&ww); ? Infinite loop would catch that at first test. ~Maarten
On 8/16/21 3:34 PM, Maarten Lankhorst wrote: > Op 16-08-2021 om 15:30 schreef Thomas Hellström: >> On 8/16/21 3:25 PM, Matthew Auld wrote: >>> On Mon, 16 Aug 2021 at 09:49, Thomas Hellström >>> <thomas.hellstrom@linux.intel.com> wrote: >>>> It's only used by the for_i915_gem_ww() macro and we can use >>>> the (typically) on-stack _err variable in its place. >>>> >>>> While initially setting the _err variable to -EDEADLK to enter the >>>> loop, we clear it before actually entering using fetch_and_zero() to >>>> avoid empty loops or code not setting the _err variable running forever. >>>> >>>> Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++--------------- >>>> 1 file changed, 8 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h >>>> index f6b1a796667b..98348b1e6182 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem_ww.h >>>> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h >>>> @@ -7,12 +7,13 @@ >>>> >>>> #include <drm/drm_drv.h> >>>> >>>> +#include "i915_utils.h" >>>> + >>>> struct i915_gem_ww_ctx { >>>> struct ww_acquire_ctx ctx; >>>> struct list_head obj_list; >>>> struct drm_i915_gem_object *contended; >>>> - unsigned short intr; >>>> - unsigned short loop; >>>> + bool intr; >>>> }; >>>> >>>> void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); >>>> @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); >>>> /* Internal functions used by the inlines! Don't use. */ >>>> static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) >>>> { >>>> - ww->loop = 0; >>>> if (err == -EDEADLK) { >>>> err = i915_gem_ww_ctx_backoff(ww); >>>> if (!err) >>>> - ww->loop = 1; >>>> + err = -EDEADLK; >>>> } >>>> >>>> - if (!ww->loop) >>>> + if (err != -EDEADLK) >>>> i915_gem_ww_ctx_fini(ww); >>>> >>>> return err; >>>> } >>>> >>>> -static inline void >>>> -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr) >>>> -{ >>>> - i915_gem_ww_ctx_init(ww, intr); >>>> - ww->loop = 1; >>>> -} >>>> - >>>> -#define for_i915_gem_ww(_ww, _err, _intr) \ >>>> - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \ >>>> +#define for_i915_gem_ww(_ww, _err, _intr) \ >>>> + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \ >>>> + fetch_and_zero(&_err) == -EDEADLK; \ >>> Doesn't this now hide "normal" errors, like say get_pages() returning >>> -ENOSPC or so? >> Yes, good catch. We should either just clear the -EDEADLK case, or not clear the error at all.. >> >> /Thomas > I believe not setting _err is a bug anyway. Why would you do such a loop without at least one err = ww_mutex_lock(&ww); ? > > Infinite loop would catch that at first test. OK, I'll skip the clearing then. /Thomas > > ~Maarten >
diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h index f6b1a796667b..98348b1e6182 100644 --- a/drivers/gpu/drm/i915/i915_gem_ww.h +++ b/drivers/gpu/drm/i915/i915_gem_ww.h @@ -7,12 +7,13 @@ #include <drm/drm_drv.h> +#include "i915_utils.h" + struct i915_gem_ww_ctx { struct ww_acquire_ctx ctx; struct list_head obj_list; struct drm_i915_gem_object *contended; - unsigned short intr; - unsigned short loop; + bool intr; }; void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr); @@ -23,28 +24,20 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj); /* Internal functions used by the inlines! Don't use. */ static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err) { - ww->loop = 0; if (err == -EDEADLK) { err = i915_gem_ww_ctx_backoff(ww); if (!err) - ww->loop = 1; + err = -EDEADLK; } - if (!ww->loop) + if (err != -EDEADLK) i915_gem_ww_ctx_fini(ww); return err; } -static inline void -__i915_gem_ww_init(struct i915_gem_ww_ctx *ww, bool intr) -{ - i915_gem_ww_ctx_init(ww, intr); - ww->loop = 1; -} - -#define for_i915_gem_ww(_ww, _err, _intr) \ - for (__i915_gem_ww_init(_ww, _intr); (_ww)->loop; \ +#define for_i915_gem_ww(_ww, _err, _intr) \ + for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \ + fetch_and_zero(&_err) == -EDEADLK; \ _err = __i915_gem_ww_fini(_ww, _err)) - #endif
It's only used by the for_i915_gem_ww() macro and we can use the (typically) on-stack _err variable in its place. While initially setting the _err variable to -EDEADLK to enter the loop, we clear it before actually entering using fetch_and_zero() to avoid empty loops or code not setting the _err variable running forever. Suggested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem_ww.h | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)