On Thu, 29 Apr 2010 20:39:24 -0400, Eamon Walsh <[email protected]> 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. -- [email protected]
pgpDGBondjfW2.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
