[PATCH] fix some performance gaps in Xace

[Eamon Walsh]

Arjan van de Ven wrote:
>>From 50d6941ab3af3297212fbcc20231bcaf0c8c6463 Mon Sep 17 00:00:00 2001
> From: root <root at benny.jf.intel.com>
> Date: Tue, 30 Oct 2007 23:43:24 +0100
> Subject: [PATCH] fix some performance gaps in Xace
> 
> The XaceHook function is used in several hotpaths.
> The problem with it (performance wise) is twofold:
>  * The XaceHook function has a big switch() statement for the hook number in it
>  * The XaceHook function uses varargs to reassemble the final dispatch arguments again
> 
> Both are expensive operations... for something that is known at compile time
> 
> This patch turns the hotpath XaceHook call into a direct call to avoid
> the switch and varargs; this gives me over 10% performance gain
> on the x11perf benchmark.

Thanks for the patch.  Could I get a copy of your numbers, I have some 
x11perf results from some point that don't show this big of a hit.


> 
> This is only a conversion of three of the hooks; I would suggest doing all of them this way
> and getting rid of the generic multiplexer entirely... but I'd like input on that from the maintainer
> (who might even be motivated to do that... for me .. with this the performance thing is solved ;)

The audit_begin and audit_end hooks are really only there for trusted 
solaris.  Now that there are DTRACE wrappers in that very same place I'm 
considering deprecating those hooks.  But for now I'll just apply the 
patch (with the camel-case names suggested)


> 
> ---
>  Xext/xace.c    |   37 ++++++++++++++++++++++++++++++++++++-
>  Xext/xace.h    |   12 ++++++++++++
>  dix/dispatch.c |    4 ++--
>  3 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/Xext/xace.c b/Xext/xace.c
> index 6385631..b496faa 100644
> --- a/Xext/xace.c
> +++ b/Xext/xace.c
> @@ -46,6 +46,41 @@ static int (*SwappedUntrustedProcVector[256])(
>  
>  /* Entry point for hook functions.  Called by Xserver.
>   */
> +
> +
> +int XaceHook_core_dispatch(ClientPtr ptr)
> +{
> +    XaceCoreDispatchRec rec = {
> +		ptr,
> +		TRUE	/* default allow */
> +	    };
> +
> +    /* call callbacks and return result, if any. */
> +    CallCallbacks(&XaceHooks[XACE_CORE_DISPATCH], &rec);
> +    return rec.rval;
> +}
> +
> +void XaceHook_audit_begin(ClientPtr ptr)
> +{
> +    XaceAuditRec rec = {
> +        ptr,
> +	0
> +    };
> +    /* call callbacks and return result, if any. */
> +    CallCallbacks(&XaceHooks[XACE_AUDIT_BEGIN], &rec);
> +}
> +
> +void XaceHook_audit_end(ClientPtr ptr, int i)
> +{
> +    XaceAuditRec rec = {
> +        ptr,
> +	i
> +    };
> +    /* call callbacks and return result, if any. */
> +    CallCallbacks(&XaceHooks[XACE_AUDIT_END], &rec);
> +}
> +
> +
>  int XaceHook(int hook, ...)
>  {
>      pointer calldata;	/* data passed to callback */
> @@ -275,7 +310,7 @@ XaceCatchDispatchProc(ClientPtr client)
>      if (!ProcVector[major])
>  	return (BadRequest);
>  
> -    if (!XaceHook(XACE_CORE_DISPATCH, client))
> +    if (!XaceHook_core_dispatch(client))
>  	return (BadAccess);
>  
>      return client->swapped ? 
> diff --git a/Xext/xace.h b/Xext/xace.h
> index 4143cd4..f7dcd05 100644
> --- a/Xext/xace.h
> +++ b/Xext/xace.h
> @@ -68,6 +68,11 @@ extern int XaceHook(
>      ... /*appropriate args for hook*/
>      ); 
>  
> +extern void XaceHook_audit_end(ClientPtr ptr, int i);
> +extern void XaceHook_audit_begin(ClientPtr ptr);
> +
> +
> +
>  /* Register a callback for a given hook.
>   */
>  #define XaceRegisterCallback(hook,callback,data) \
> @@ -98,9 +103,16 @@ extern void XaceCensorImage(
>  
>  #ifdef __GNUC__
>  #define XaceHook(args...) XaceAllowOperation
> +#define XaceHook_audit_end(args...) XaceAllowOperation
> +#define XaceHook_audit_begin(args...) XaceAllowOperation
>  #define XaceCensorImage(args...) { ; }
> +
> +
>  #else
>  #define XaceHook(...) XaceAllowOperation
> +#define XaceHook_audit_end(...) XaceAllowOperation
> +#define XaceHook_audit_begin(...) XaceAllowOperation
> +
>  #define XaceCensorImage(...) { ; }
>  #endif
>  
> diff --git a/dix/dispatch.c b/dix/dispatch.c
> index c313796..b638ea2 100644
> --- a/dix/dispatch.c
> +++ b/dix/dispatch.c
> @@ -498,9 +498,9 @@ Dispatch(void)
>  		if (result > (maxBigRequestSize << 2))
>  		    result = BadLength;
>  		else {
> -		    XaceHook(XACE_AUDIT_BEGIN, client);
> +		    XaceHook_audit_begin(client);
>  		    result = (* client->requestVector[MAJOROP])(client);
> -		    XaceHook(XACE_AUDIT_END, client, result);
> +		    XaceHook_audit_end(client, result);
>  		}
>  #ifdef XSERVER_DTRACE
>  		XSERVER_REQUEST_DONE(GetRequestName(MAJOROP), MAJOROP,


-- 
Eamon Walsh <ewalsh at tycho.nsa.gov>
National Security Agency