Re: [PATCH] drm/i915: Only expand COND once in wait_for()

From: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
To: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"chris@chris-wilson.co.uk" <chris@chris-wilson.co.uk>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH] drm/i915: Only expand COND once in wait_for()
Date: Wed, 14 Sep 2016 13:37:10 +0000	[thread overview]
Message-ID: <1473860228.2435.106.camel@intel.com> (raw)
In-Reply-To: <20160914092221.GH25204@nuc-i3427.alporthouse.com>

Em Qua, 2016-09-14 às 10:22 +0100, Chris Wilson escreveu:
> On Tue, Sep 13, 2016 at 08:40:19PM +0100, Chris Wilson wrote:
> > 
> > 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	 1410fd
> > 	before
> > 1305341	   5184	   1152	1311677	 1403bd
> > 	after
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index cb99a2540863..597899d71df9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -52,13 +52,16 @@
> >   */
> >  #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__;							
> > \
> 
> Ok, this is the magic. Missed initializer, gcc goes wild trimming
> undefined code. Patch is completely bogus.

IMHO, expanding a macro argument only once is an improvement on its
own, even if the resulting binary is not smaller, since it makes the
code a little safer.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx