| Message ID | 1473855033-26980-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qua, 2016-09-14 às 13:10 +0100, Dave Gordon escreveu: > Commentary from Chris Wilson's original version: > > > > > I was looking at some wait_for() timeouts on a slow system, with > > lots of > > debug enabled (KASAN, lockdep, mmio_debug). Thinking that we were > > mishandling the timeout, I tried to ensure that we loop at least > > once > > after first testing COND. However, the double test of COND either > > side > > of the timeout check makes that unlikely. But we can do an > > equivalent > > loop, that keeps the COND check after testing for timeout (required > > so > > that we are not preempted between testing COND and then testing for > > a > > timeout) without expanding COND twice. > > > > The advantage of only expanding COND once is a dramatic reduction > > in > > code size: > > > > text data bss dec hex > > 1308733 5184 1152 1315069 141 > > 0fd before > > 1305341 5184 1152 1311677 140 > > 3bd after > > but it turned out that due to a missing iniitialiser, gcc had "gone > wild trimming undefined code" :( This version acheives a rather more > modest (but still worthwhile) gain of ~550 bytes. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Original-idea-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index abe7a4d..8fd16ad 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -52,11 +52,15 @@ > */ > #define _wait_for(COND, US, W) ({ \ > unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + > 1; \ > - int ret__ = 0; > \ > - while (!(COND)) { > \ > - if (time_after(jiffies, timeout__)) { > \ > - if (!(COND)) > \ > - ret__ = -ETIMEDOUT; > \ > + int ret__; > \ > + for (;;) { > \ > + bool expired__ = time_after(jiffies, timeout__); > \ > + if (COND) { > \ > + ret__ = 0; > \ > + break; > \ > + } > \ > + if (expired__) { > \ > + ret__ = -ETIMEDOUT; > \ > break; > \ > } > \ > if ((W) && drm_can_sleep()) { > \
On Wed, Sep 14, 2016 at 03:53:32PM -0300, Paulo Zanoni wrote: > Em Qua, 2016-09-14 às 13:10 +0100, Dave Gordon escreveu: > > Commentary from Chris Wilson's original version: > > > > > > > > I was looking at some wait_for() timeouts on a slow system, with > > > lots of > > > debug enabled (KASAN, lockdep, mmio_debug). Thinking that we were > > > mishandling the timeout, I tried to ensure that we loop at least > > > once > > > after first testing COND. However, the double test of COND either > > > side > > > of the timeout check makes that unlikely. But we can do an > > > equivalent > > > loop, that keeps the COND check after testing for timeout (required > > > so > > > that we are not preempted between testing COND and then testing for > > > a > > > timeout) without expanding COND twice. > > > > > > The advantage of only expanding COND once is a dramatic reduction > > > in > > > code size: > > > > > > text data bss dec hex > > > 1308733 5184 1152 1315069 141 > > > 0fd before > > > 1305341 5184 1152 1311677 140 > > > 3bd after > > > > but it turned out that due to a missing iniitialiser, gcc had "gone > > wild trimming undefined code" :( This version acheives a rather more > > modest (but still worthwhile) gain of ~550 bytes. > > > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > Original-idea-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Thanks for the improvement and review, pushed. -Chris
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index abe7a4d..8fd16ad 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -52,11 +52,15 @@ */ #define _wait_for(COND, US, W) ({ \ unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1; \ - int ret__ = 0; \ - while (!(COND)) { \ - if (time_after(jiffies, timeout__)) { \ - if (!(COND)) \ - ret__ = -ETIMEDOUT; \ + int ret__; \ + for (;;) { \ + bool expired__ = time_after(jiffies, timeout__); \ + if (COND) { \ + ret__ = 0; \ + break; \ + } \ + if (expired__) { \ + ret__ = -ETIMEDOUT; \ break; \ } \ if ((W) && drm_can_sleep()) { \