Re: [PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

2016-05-26 Thread Keith Packard 
Mark Kettenis  writes:

> Hmm.  This is marked as _X_EXPORT, so presumably part of the driver
> ABI.  Exporting variables like this is generally a bad idea, at least
> for ELF DSOs.  Copy relocations and all that.

even libc exposes a pile of data, including stdin, stderr and stdout,
and that seems to work just fine.

Switching this from a #define to a exported variable makes the change
minimally invasive while solving the problem nicely. Anything more
complicated will be harder to understand and end up slower than this
solution. I don't see any benefit there.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

2016-05-03 Thread Adam Jackson 
On Tue, 2016-05-03 at 17:35 +0200, Mark Kettenis wrote:

> Hmm.  This is marked as _X_EXPORT, so presumably part of the driver
> ABI.  Exporting variables like this is generally a bad idea, at least
> for ELF DSOs.  Copy relocations and all that.

It would be part of the driver ABI, yes, and I'm certainly sympathetic
to avoiding exposing variables directly as a matter of style. But it
very much wouldn't be the first, and in this particular case I don't
think it matters much. Relocations to variables defined in the server
seem to end up as direct relocs not copies:

dmt:~/git/xserver% eu-readelf -r hw/xfree86/dixmods/.libs/libglx.so | grep noGlx
  0x00240d40  X86_64_64   00  +0 noGlxExtension

Even if they were copy relocs, I believe that only affects
initializaton performance as opposed to runtime. Perhaps there's some
other negative property I'm missing here, please do enlighten me if so.

> So isn't it better to keep this as a function call?  You'd still make
> it return a precalculated value instead of doing the ilog2 on each
> call.  Marking it as a "pure" function in addition to that should fine
> too.

I guess the question is what API the callers need, and the answer seems
to be CLIENT_ID(), CLIENT_BITS(), and possibly RESOURCE_ID_MASK and
CLIENTOFFSET; at least, those are the matches I find outside of the
resource system itself, in xserver itself. I have some difficulty
imagining out-of-tree code that would need to be much more intimate
with the resource sytem than CLIENT_ID() and CLIENT_BITS().

I'll withdraw this for the moment and conjure up something better.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

2016-05-03 Thread Mark Kettenis 
> From: Adam Jackson 
> Date: Tue,  3 May 2016 11:24:36 -0400
> 
> This turns out to be a remarkably hot function in XID lookup.  Consider
> the expansion of CLIENT_ID. We start with:
> 
>  #define RESOURCE_CLIENT_BITS ResourceClientBits()
>  #define RESOURCE_AND_CLIENT_COUNT 29
>  #define CLIENTOFFSET (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS)
> 
> So that's effectively:
> 
>  #define CLIENTOFFSET (29 - RCB())
> 
> Then:
> 
>  #define RESOURCE_CLIENT_MASK \
> ((1 << RESOURCE_CLIENT_BITS) - 1) << CLIENTOFFSET
> 
> Is effectively:
> 
>  #define RESOURCE_CLIENT_MASK (((1 << RCB()) - 1) << (29 - RCB())
> 
> And:
> 
>  #define CLIENT_BITS(id) ((id) & RESOURCE_CLIENT_MASK)
>  #define CLIENT_ID(id) ((int)(CLIENT_BITS(id) >> CLIENTOFFSET))
> 
> Is:
> 
>  #define CLIENT_ID(id) \
> ((id) & (((1 << RCB()) - 1) << (29 - RCB( >> (29 - RCB())
> 
> Since ResourceClientBits isn't marked __attribute__((pure)), the
> optimizer isn't allowed to CSE those function calls, since it doesn't
> know they don't have side effects. And, worse, ResourceClientBits
> doesn't just return an integer, it computes the ilog2 afresh every time.
> 
> Instead we can just compute the value we need at argv parse.
> 
> v2: Ensure ResourceClientBits is initialized even when we don't pass
> -maxclients (Alan Coopersmith)

Hmm.  This is marked as _X_EXPORT, so presumably part of the driver
ABI.  Exporting variables like this is generally a bad idea, at least
for ELF DSOs.  Copy relocations and all that.

So isn't it better to keep this as a function call?  You'd still make
it return a precalculated value instead of doing the ilog2 on each
call.  Marking it as a "pure" function in addition to that should fine
too.

Cheers,

Mark


> Signed-off-by: Adam Jackson 
> ---
>  dix/resource.c | 23 ---
>  include/resource.h |  4 ++--
>  os/osinit.c|  1 +
>  os/utils.c | 13 +
>  4 files changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/dix/resource.c b/dix/resource.c
> index ad71b24..f06bf58 100644
> --- a/dix/resource.c
> +++ b/dix/resource.c
> @@ -600,29 +600,6 @@ CreateNewResourceClass(void)
>  
>  static ClientResourceRec clientTable[MAXCLIENTS];
>  
> -static unsigned int
> -ilog2(int val)
> -{
> -int bits;
> -
> -if (val <= 0)
> - return 0;
> -for (bits = 0; val != 0; bits++)
> - val >>= 1;
> -return bits - 1;
> -}
> -
> -/*
> - * ResourceClientBits
> - *Returns the client bit offset in the client + resources ID field
> - */
> -
> -unsigned int
> -ResourceClientBits(void)
> -{
> -return (ilog2(LimitClients));
> -}
> -
>  /*
>   * InitClientResources
>   *When a new client is created, call this to allocate space
> diff --git a/include/resource.h b/include/resource.h
> index 5871a4c..3cce6c6 100644
> --- a/include/resource.h
> +++ b/include/resource.h
> @@ -85,10 +85,10 @@ typedef uint32_t RESTYPE;
>  #define RT_LASTPREDEF((RESTYPE)9)
>  #define RT_NONE  ((RESTYPE)0)
>  
> -extern _X_EXPORT unsigned int ResourceClientBits(void);
> +extern _X_EXPORT unsigned int ResourceClientBits;
>  /* bits and fields within a resource id */
>  #define RESOURCE_AND_CLIENT_COUNT   29  /* 29 bits for XIDs */
> -#define RESOURCE_CLIENT_BITSResourceClientBits() /* client field 
> offset */
> +#define RESOURCE_CLIENT_BITSResourceClientBits /* client field 
> offset */
>  #define CLIENTOFFSET (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS)
>  /* resource field */
>  #define RESOURCE_ID_MASK ((1 << CLIENTOFFSET) - 1)
> diff --git a/os/osinit.c b/os/osinit.c
> index 54b39a0..92aced3 100644
> --- a/os/osinit.c
> +++ b/os/osinit.c
> @@ -88,6 +88,7 @@ int limitNoFile = -1;
>  
>  /* The actual user defined max number of clients */
>  int LimitClients = LIMITCLIENTS;
> +unsigned int ResourceClientBits;
>  
>  static OsSigWrapperPtr OsSigWrapper = NULL;
>  
> diff --git a/os/utils.c b/os/utils.c
> index e48d9f8..8b81d39 100644
> --- a/os/utils.c
> +++ b/os/utils.c
> @@ -513,6 +513,18 @@ AdjustWaitForDelay(void *waitTime, unsigned long 
> newdelay)
>  }
>  }
>  
> +static unsigned int
> +ilog2(int val)
> +{
> +int bits;
> +
> +if (val <= 0)
> + return 0;
> +for (bits = 0; val != 0; bits++)
> + val >>= 1;
> +return bits - 1;
> +}
> +
>  void
>  UseMsg(void)
>  {
> @@ -1061,6 +1073,7 @@ ProcessCommandLine(int argc, char *argv[])
>  FatalError("Unrecognized option: %s\n", argv[i]);
>  }
>  }
> +ResourceClientBits = ilog2(LimitClients);
>  }
>  
>  /* Implement a simple-minded font authorization scheme.  The authorization
> -- 
> 2.7.4
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 
> 
___

[PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

2016-05-03 Thread Adam Jackson 
This turns out to be a remarkably hot function in XID lookup.  Consider
the expansion of CLIENT_ID. We start with:

 #define RESOURCE_CLIENT_BITS ResourceClientBits()
 #define RESOURCE_AND_CLIENT_COUNT 29
 #define CLIENTOFFSET (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS)

So that's effectively:

 #define CLIENTOFFSET (29 - RCB())

Then:

 #define RESOURCE_CLIENT_MASK \
((1 << RESOURCE_CLIENT_BITS) - 1) << CLIENTOFFSET

Is effectively:

 #define RESOURCE_CLIENT_MASK (((1 << RCB()) - 1) << (29 - RCB())

And:

 #define CLIENT_BITS(id) ((id) & RESOURCE_CLIENT_MASK)
 #define CLIENT_ID(id) ((int)(CLIENT_BITS(id) >> CLIENTOFFSET))

Is:

 #define CLIENT_ID(id) \
((id) & (((1 << RCB()) - 1) << (29 - RCB( >> (29 - RCB())

Since ResourceClientBits isn't marked __attribute__((pure)), the
optimizer isn't allowed to CSE those function calls, since it doesn't
know they don't have side effects. And, worse, ResourceClientBits
doesn't just return an integer, it computes the ilog2 afresh every time.

Instead we can just compute the value we need at argv parse.

v2: Ensure ResourceClientBits is initialized even when we don't pass
-maxclients (Alan Coopersmith)

Signed-off-by: Adam Jackson 
---
 dix/resource.c | 23 ---
 include/resource.h |  4 ++--
 os/osinit.c|  1 +
 os/utils.c | 13 +
 4 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/dix/resource.c b/dix/resource.c
index ad71b24..f06bf58 100644
--- a/dix/resource.c
+++ b/dix/resource.c
@@ -600,29 +600,6 @@ CreateNewResourceClass(void)
 
 static ClientResourceRec clientTable[MAXCLIENTS];
 
-static unsigned int
-ilog2(int val)
-{
-int bits;
-
-if (val <= 0)
-   return 0;
-for (bits = 0; val != 0; bits++)
-   val >>= 1;
-return bits - 1;
-}
-
-/*
- * ResourceClientBits
- *Returns the client bit offset in the client + resources ID field
- */
-
-unsigned int
-ResourceClientBits(void)
-{
-return (ilog2(LimitClients));
-}
-
 /*
  * InitClientResources
  *When a new client is created, call this to allocate space
diff --git a/include/resource.h b/include/resource.h
index 5871a4c..3cce6c6 100644
--- a/include/resource.h
+++ b/include/resource.h
@@ -85,10 +85,10 @@ typedef uint32_t RESTYPE;
 #define RT_LASTPREDEF  ((RESTYPE)9)
 #define RT_NONE((RESTYPE)0)
 
-extern _X_EXPORT unsigned int ResourceClientBits(void);
+extern _X_EXPORT unsigned int ResourceClientBits;
 /* bits and fields within a resource id */
 #define RESOURCE_AND_CLIENT_COUNT   29  /* 29 bits for XIDs */
-#define RESOURCE_CLIENT_BITSResourceClientBits() /* client field 
offset */
+#define RESOURCE_CLIENT_BITSResourceClientBits /* client field offset 
*/
 #define CLIENTOFFSET   (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS)
 /* resource field */
 #define RESOURCE_ID_MASK   ((1 << CLIENTOFFSET) - 1)
diff --git a/os/osinit.c b/os/osinit.c
index 54b39a0..92aced3 100644
--- a/os/osinit.c
+++ b/os/osinit.c
@@ -88,6 +88,7 @@ int limitNoFile = -1;
 
 /* The actual user defined max number of clients */
 int LimitClients = LIMITCLIENTS;
+unsigned int ResourceClientBits;
 
 static OsSigWrapperPtr OsSigWrapper = NULL;
 
diff --git a/os/utils.c b/os/utils.c
index e48d9f8..8b81d39 100644
--- a/os/utils.c
+++ b/os/utils.c
@@ -513,6 +513,18 @@ AdjustWaitForDelay(void *waitTime, unsigned long newdelay)
 }
 }
 
+static unsigned int
+ilog2(int val)
+{
+int bits;
+
+if (val <= 0)
+   return 0;
+for (bits = 0; val != 0; bits++)
+   val >>= 1;
+return bits - 1;
+}
+
 void
 UseMsg(void)
 {
@@ -1061,6 +1073,7 @@ ProcessCommandLine(int argc, char *argv[])
 FatalError("Unrecognized option: %s\n", argv[i]);
 }
 }
+ResourceClientBits = ilog2(LimitClients);
 }
 
 /* Implement a simple-minded font authorization scheme.  The authorization
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

2016-04-30 Thread Alan Coopersmith 

On 04/29/16 11:22 AM, Adam Jackson wrote:

@@ -871,6 +883,7 @@ ProcessCommandLine(int argc, char *argv[])
LimitClients != 512) {
FatalError("maxclients must be one of 64, 128, 256 or 
512\n");
}
+ResourceClientBits = ilog2(LimitClients);
} else
UseMsg();
}



Don't you also need to initialize the default case somewhere when
-maxclients isn't used?

--
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

2016-04-29 Thread Adam Jackson 
This turns out to be a remarkably hot function in XID lookup.  Consider
the expansion of CLIENT_ID. We start with:

 #define RESOURCE_CLIENT_BITS ResourceClientBits()
 #define RESOURCE_AND_CLIENT_COUNT 29
 #define CLIENTOFFSET (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS)

So that's effectively:

 #define CLIENTOFFSET (29 - RCB())

Then:

 #define RESOURCE_CLIENT_MASK \
((1 << RESOURCE_CLIENT_BITS) - 1) << CLIENTOFFSET

Is effectively:

 #define RESOURCE_CLIENT_MASK (((1 << RCB()) - 1) << (29 - RCB())

And:

 #define CLIENT_BITS(id) ((id) & RESOURCE_CLIENT_MASK)
 #define CLIENT_ID(id) ((int)(CLIENT_BITS(id) >> CLIENTOFFSET))

Is:

 #define CLIENT_ID(id) \
((id) & (((1 << RCB()) - 1) << (29 - RCB( >> (29 - RCB())

Since ResourceClientBits isn't marked __attribute__((pure)), the
optimizer isn't allowed to CSE those function calls, since it doesn't
know they don't have side effects. And, worse, ResourceClientBits
doesn't just return an integer, it computes the ilog2 afresh every time.

Instead we can just compute the value we need at argv parse.

Signed-off-by: Adam Jackson 
---
 dix/resource.c | 23 ---
 include/resource.h |  4 ++--
 os/osinit.c|  1 +
 os/utils.c | 13 +
 4 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/dix/resource.c b/dix/resource.c
index ad71b24..f06bf58 100644
--- a/dix/resource.c
+++ b/dix/resource.c
@@ -600,29 +600,6 @@ CreateNewResourceClass(void)
 
 static ClientResourceRec clientTable[MAXCLIENTS];
 
-static unsigned int
-ilog2(int val)
-{
-int bits;
-
-if (val <= 0)
-   return 0;
-for (bits = 0; val != 0; bits++)
-   val >>= 1;
-return bits - 1;
-}
-
-/*
- * ResourceClientBits
- *Returns the client bit offset in the client + resources ID field
- */
-
-unsigned int
-ResourceClientBits(void)
-{
-return (ilog2(LimitClients));
-}
-
 /*
  * InitClientResources
  *When a new client is created, call this to allocate space
diff --git a/include/resource.h b/include/resource.h
index 5871a4c..3cce6c6 100644
--- a/include/resource.h
+++ b/include/resource.h
@@ -85,10 +85,10 @@ typedef uint32_t RESTYPE;
 #define RT_LASTPREDEF  ((RESTYPE)9)
 #define RT_NONE((RESTYPE)0)
 
-extern _X_EXPORT unsigned int ResourceClientBits(void);
+extern _X_EXPORT unsigned int ResourceClientBits;
 /* bits and fields within a resource id */
 #define RESOURCE_AND_CLIENT_COUNT   29  /* 29 bits for XIDs */
-#define RESOURCE_CLIENT_BITSResourceClientBits() /* client field 
offset */
+#define RESOURCE_CLIENT_BITSResourceClientBits /* client field offset 
*/
 #define CLIENTOFFSET   (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS)
 /* resource field */
 #define RESOURCE_ID_MASK   ((1 << CLIENTOFFSET) - 1)
diff --git a/os/osinit.c b/os/osinit.c
index 54b39a0..92aced3 100644
--- a/os/osinit.c
+++ b/os/osinit.c
@@ -88,6 +88,7 @@ int limitNoFile = -1;
 
 /* The actual user defined max number of clients */
 int LimitClients = LIMITCLIENTS;
+unsigned int ResourceClientBits;
 
 static OsSigWrapperPtr OsSigWrapper = NULL;
 
diff --git a/os/utils.c b/os/utils.c
index e48d9f8..8f23634 100644
--- a/os/utils.c
+++ b/os/utils.c
@@ -513,6 +513,18 @@ AdjustWaitForDelay(void *waitTime, unsigned long newdelay)
 }
 }
 
+static unsigned int
+ilog2(int val)
+{
+int bits;
+
+if (val <= 0)
+   return 0;
+for (bits = 0; val != 0; bits++)
+   val >>= 1;
+return bits - 1;
+}
+
 void
 UseMsg(void)
 {
@@ -871,6 +883,7 @@ ProcessCommandLine(int argc, char *argv[])
LimitClients != 512) {
FatalError("maxclients must be one of 64, 128, 256 or 
512\n");
}
+ResourceClientBits = ilog2(LimitClients);
} else
UseMsg();
}
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel