Fixing devPrivates

[Keith Packard]

On Thu, 29 Apr 2010 20:39:24 -0400, Eamon Walsh <ewalsh at tycho.nsa.gov> wrote:
> 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.

Right, having the privates system do the combining automatically seems a
lot more maintainable...

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

I've removed all keys from DIX; I'll be restructuring the patch sequence
so that those changes are separated out from the other privates changes.
If DIX needs a pointer in every object, there's no reason it shouldn't
just have a nicely typed pointer.

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

The current system works well -- getting to a single implementation and
turning ad-hoc code into well specified interfaces has been a great
benefit. The change here is largely incremental, using an abstract type
for the private key and requiring that all keys be registered. It's a
small change in theory, but impacts every single private key. Turns out
the server tree has around 100 private keys in various code paths.

> 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).

Actually, few of the drivers register keys, so most of them won't need
to change.

And, yes, I'd still like people to think about merging drivers into the
server; I would have fixed any of them present in this patch. But, there
are cogent arguments against merging as well, mostly involving different
release schedules and the need to provide support for multiple server
versions in "enterprise" distributions. We'll sort out what makes sense
at some point; no-one is going to "force" anyone to merge code against
their will (as if that could even happen).

> 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

Right, so the plan is to simply re-write all of the other keys in the
system. As they're all allocated before any of the underlying objects
are created, there won't be anything using them yet. Except for
the default colormap for every screen. Hrm.

> 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 only extension using PRIVATE_ALL is SELinux, and at least RedHat
define SELinux as a part of their X build but most of their users don't
turn it on, so we want to avoid allocating private storage for cases
where the extension is compiled but not enabled.

> Or, make PRIVATE_LAST a variable, like lastResourceType, and
> provide some API to bump it.

Yup, as soon as we need that we can change the implementation easily
enough.

> (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).

The list of possible objects with private allocation doesn't impact
storage anywhere except for needing the header for a list of per-type
private keys. Extending that array should be trivial.

> 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.

I thought the reason you aren't allocating the extra space with the
privates was that you wanted to have that extra space only allocated for
objects using it; the whole privates system is "lazy" about allocating
privates to conserve storage.

I've defined a new function, FreePixmap, which is used as a book-end to
drivers using AllocatePixmap (all of them that I know of). Call that and
the right thing will 'just happen'. I discovered that the screen pixmap
was never getting freed on server reset though, so I added a call to
FreePixmap in fbCloseScreen.

> 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.

The keys need to be allocated before any objects are created, which
means that any extension would need to register the key in the extension
initialization. I've hand-audited the core server code to find all of
the existing private keys (nicely labeled with DevPrivateKey) to make
sure they are initialized at the right time.

Any missing keys will generate an assert when code tries to use them. At
least we'll find out :-)

> 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.

Awesome. This will simplify the code.

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

They're already part of the ABI as they're in the DevPrivateKeyRec which
defines the key itself. Any change to those structures will require an
ABI bump. Having them in the inline functions doesn't change this at
all, just makes the code shorter, if we're willing to remove the assert
calls at some point :-)

Thanks very much for your review; the more eyes on a change like this
the better I feel. It took a day or so of fairly steady debugging to get
the server running again once I'd changed things.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100429/8fcdf4db/attachment.pgp>