Fixing devPrivates

[Eamon Walsh]

On 04/29/2010 02:59 AM, Keith Packard wrote:
> I've been wanting to do this for a while, but the recent work to
> eliminate MAXSCREENS has pushed me to get it done sooner rather than
> later.
>
> What's the problem with devPrivates?
>
> The biggest issue is that the index space is global. Which is to say
> that anyone allocating a private index for any datatype using privates
> is able to use that private index in any other datatype too. Allocate a
> GC private index and use that to store data on a colormap.
>
> Having global indices is useful; XSELinux uses this to attach security
> labels to piles of stuff in the server. Keeping this capability is a
> hard requirement for any changes in the devPrivates code. The key is to
> allow global indices without requiring that all indices be global
>
> However, it's also amazingly expensive. With the current implementation,
> each DevPrivateKey in use allocates 8 bytes in all of the following data
> structures:
>
>  * screen
>  * device
>  * client
>  * window property
>  * selection
>  * extension
>  * window
>  * pixmap
>  * gc
>  * cursor
>  * cursor_bits
>  * colormap
>  * dbe screen private
>  * dbe window private
>  * damage tracking objects
>  * render glyphs
>  * render glyphsets
>  * render pictures
>
> My X server has 57 DevPrivateKeys in use right now. That's up to 456
> bytes in each of thousands of objects.
>   

The number 57 could probably be lowered by going through and finding
keys that could be combined.  If key X is only used for pixmaps by
subsystem A, subsystem B could reuse that value for screens or
whatever.  I actually started to do this review, but the trouble I ran
into is that keys can't be combined if they request different
pre-allocated sizes.  At least not easily.

You could also lower the number by removing unnecessary keys from dix,
which I see the patch does some of already.

So just tossing out that incremental improvements could be made to the
current system.  Although, I won't really defend it.

Also, changing this API again would be easier if the drivers were
in-tree.  I seem to recall this was talked about at XDC last year.  Is
that idea dead again?  (Not that I want that discussion rehashed in this
thread).


> Most of this should be fairly obvious, the one exception is probably the
> 'type' value. This is a fixed enumeration of all possible data
> structures holding devPrivates. Let me make that a bit louder.
>
> I AM PROPOSING TO ELIMINATE THE ABILITY TO USE THE DEV PRIVATES
> INFRASTRUCTURE IN DATA TYPES DEFINED OUTSIDE THE CORE SERVER.
>
> If you're using dev privates in something other than one of the above
> listed data structures, please let me know -- fixing it won't be hard,
> just slightly annoying.
>   

I thought about exactly this type of scheme, with an enumeration of
types including a special PRIVATE_ALL value.  One reason I didn't use it
is that if someone does a late PRIVATE_ALL registration, the offsets for
all object types get bumped forward to the *largest* offset of any type,
which is not much better than global keys.  It would be best if the
PRIVATES_ALL privates were ordered first in the list.  But you can't
reorder things once registration has begun.  You could set aside the
first 1 or 2 indices for use by PRIVATE_ALL.  But then that space gets
wasted if no one uses it!

I wonder if it would be possible to manage this at configure time.  Like
any extension that uses PRIVATES_ALL could bump some configure.ac define
that ends up setting aside space at the front of the list.

The second reason is that it creates a hard-coded list of object types,
which doesn't jive with the X server's resource system where you can add
new resource types.  At the least, I think if we are going to hardcode
knowledge of the RandR, Damage, and DBE resources in privates.h, they
should also be hardcoded in resource.h.  Add RT_FOO constants for those
types and remove the dynamic registration from those extensions.  Also
maybe add some extra "PRIVATE_UNUSED" constants just for future
expansion.  Or, make PRIVATE_LAST a variable, like lastResourceType, and
provide some API to bump it.

(Also, DBE?  We just killed Multibuffer, is DBE far behind?  At least
put the DBE constants at the end of the list where they won't fragment
if they ever disappear).



>              Some details about the implementation:
>
>  1) The base object and all of the private storage is usually allocated
>     in a single giant block. The base WindowRec is 132 bytes, the total
>     allocation for window privates is 28 bytes for a total allocation of
>     160 bytes. The code doesn't allocate pointers for privates which
>     have pre-allocated storage, so if you need a private structure in
>     every object (like fb's GC private structure), just ask the privates
>     code to allocate some space there. There are two exceptions to this
>     rule -- screens and and the server client, each of which are
>     allocated before most of the initialization code in the server is
>     run.
>   

ISTR all kinds of trouble calling dixFreePrivates(), and/or allocating
objects together with privates, because some objects were allocated or
freed deep in the guts of the DDX instead of in dix.  I'm pretty sure it
was pixmaps that were the problem.  Don't recall for sure, your code
seems to be a lot cleaner.

>     
>  2) All DevPrivateKeys must be registered before any dynamic objects are
>     created (anything but a screen or serverClient). This is required by
>     the above optimization.
>   

I'm worried about things like scratch/global objects created in
extension load functions before other extensions have had a chance to
load and register.  For example, I thought the Composite overlay window
might be a problem (but it appears to be created as-needed).  I guess
the assertion in the registration function will catch this if it does
happen.

>     
>  3) The init/delete callbacks only work for PRIVATE_ALL keys. I'd love
>     to figure out how to eliminate these completely as the only user
>     is XSELinux. Note that because the storage for all of the indices
>     is allocated when the object is created, the init callbacks will
>     be called for every object, not just when the object has a label.
>     I suggest that a good plan would be to limit the scope of
>     PRIVATE_ALL to a subset of the objects in the server that are used
>     by XSELinux (and perhaps rename it as well). That would reduce
>     memory usage when XSELinux is enabled.
>   

Recent changes to libselinux have made these callbacks less necessary. 
They are now only used to free a string, which could be done in a
separate private, and to set a default security label of "unlabeled" on
everything, which should in theory always be updated to a real label
before being read.

I am willing to drop them entirely.


>  4) Fetching a devPrivate is now a very short operation:
>
>         static inline void *
>         dixGetPrivateAddr(PrivatePtr *privates, const DevPrivateKey key)
>         {
>             assert(key->initialized);
>             return (char *) (*privates) + key->offset;
>         }
>
>     Because all of the storage is always available, it really is as
>     simple as adding the 'offset' value to the devPrivates field in the
>     object. Note that 'key' is likely to be the address of a global
>     variable, so this reduces to a couple of fetches and an add. The
>     other operations are similarly short and have become inlined.
>   

Just a warning, making the lookup functions static inlines means that
the internal representation of the privates list becomes part of the
server ABI.




-- 

Eamon Walsh 
National Security Agency