[PATCH 10/22] udev: Use shared NoopDDA to utilize code cache better

[Pauli Nieminen]

On 30/12/10 11:37 +0100, ext Mark Kettenis wrote:
> > Date: Thu, 30 Dec 2010 11:32:18 +0200
> > From: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> > 
> > On 29/12/10 21:19 +0100, ext Mark Kettenis wrote:
> > > > From: Pauli <ext-pauli.nieminen at nokia.com>
> > > > Date: Wed, 29 Dec 2010 21:27:22 +0200
> > > > 
> > > > From: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> > > > 
> > > > Calling function that is in code cache is order of magnitude faster. In
> > > > arm non-cached simple function takes about 1us while cached function
> > > > takes max 200ns.
> > > 
> > > > diff --git a/config/udev.c b/config/udev.c
> > > > index 496bfbf..393723c 100644
> > > > --- a/config/udev.c
> > > > +++ b/config/udev.c
> > > > @@ -253,11 +253,6 @@ wakeup_handler(pointer data, int err, pointer read_mask)
> > > >      }
> > > >  }
> > > >  
> > > > -static void
> > > > -block_handler(pointer data, struct timeval **tv, pointer read_mask)
> > > > -{
> > > > -}
> > > > -
> > > >  int
> > > >  config_udev_init(void)
> > > >  {
> > > > @@ -290,7 +285,7 @@ config_udev_init(void)
> > > >      }
> > > >      udev_enumerate_unref(enumerate);
> > > >  
> > > > -    RegisterBlockAndWakeupHandlers(block_handler, wakeup_handler, NULL);
> > > > +    RegisterBlockAndWakeupHandlers((BlockHandlerProcPtr)NoopDDA, wakeup_handler, NULL);
> > > 
> > > YUCK!  Obfuscating code like this is sooo wrong.  I don't think
> > > optimization hacks like this that only really matter for hardware with
> > > last-century sized caches belong in the Xorg tree.
> > 
> > Obfuscating? Can you explain how this obfuscates code?
> 
> Sure.  The code adds casts.  Casts are almost always bad.  Here they
> are "bad" since calling a function through an incompatible pointer
> yields undefined behaviour according to the C standard.  Now in
> practice, any sane implementation of C will probably do the sane thing
> in this case.  But it still makes me somewhat unhappy to add those casts.
> 

Good point.

Too bad code already assumes compiler does right thing when calling function
pointer. That means neither side assumes anything what other side is doing.
Clever compiler could maybe generate better binary if it did assumptions.
(Specially in architecture like ARM which has high number of registers)

> > I tough that NoopDDA is standard way of assign empty function to any
> > callback. It is already used in most places but these weren't using it.
> 
> OK, I wasn't aware of this.  It does indeed look as if NoopDDA() is
> used in this fashion in a lot more places.  Sorry, I'm not too
> familliar with the bits of code where it is used a lot.  Objection
> withdrawn.
> 
> On the other hand...
> 
> > IMO it is less clear if we have everywhere random empty function just for
> > broken BlockAndWakeupHandlers. But I guess fixing the API might also option.
> 
> ...I think Daniel's suggested diff makes a lot of sense and would be a
> cleaner way to fix things.

I guess I could just redesign the API because current one is suboptimal and
inflexible for WakeupHandlers. Originally I didn't want to break the API
because gains didn't look large enough.

But having had closer look what goes in WakeupHandlers it starts to look like
worth of changes.