Fixing devPrivates
Keith Packard
keithp at keithp.com
Wed Apr 28 23:59:06 PDT 2010
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.
("up to" because each object only gets private storage for the
largest key index ever used with that object).
There are other minor issues, like failing to merge allocation of the
privates and the base object and separate allocations for every
pre-requested data.
However, there's also much to like about the current system as it
replaced the ad-hoc mess developed some 20 years ago. So, I've tried to
keep the best parts while fixing the memory usage issue.
There are two key API changes that I'm proposing:
1) The 'key' becomes a data structure holding lots of key-related data.
This lets the privates code find this related data without resorting
to an internal array.
2) Every key must be registered with the privates system during each
server generation. Fail to register a key and you'll get an abort()
as soon as you try to use it.
It's possible to adapt to this change with some very small adjustments
in your code; simply replace 'int' in the index variable declaration
with 'DevPrivateKeyRec' and make sure dixRegisterPrivateKey is called
(potentially replacing an existing call to dixRequestPrivate). The rest
of your code should work just fine.
Here's what the key looks like:
typedef struct _DevPrivateKeyRec {
int offset;
int size;
Bool initialized;
DevPrivateType type;
CallbackListPtr initfuncs;
CallbackListPtr deletefuncs;
struct _DevPrivateKeyRec *next;
} DevPrivateKeyRec, *DevPrivateKey;
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.
And, here's what the new key registration function looks like:
/*
* Register a new private index for the private type.
*
* If you just need to store a pointer, ask for zero byte
* and you will be able to use dixSetPrivate to store a pointer
* and dixLookupPrivate to fetch it.
*
* If you want to store more data in every object, you can ask for
* a number of bytes, but you will not be able to use dixSetPrivate
* and dixLookupPrivate and will instead use dixLookupPrivateAddr
* to get a pointer to the base of your pre-allocated storage.
*/
extern _X_EXPORT Bool
dixRegisterPrivateKey(DevPrivateKey key, DevPrivateType type,
unsigned size);
There's the 'type' again; you can pass one of the known types or
'PRIVATE_ALL' which gives you a private index in every datatype.
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.
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.
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.
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.
I wanted to get code running before I sent out this message, and I've
pushed that to:
git://people.freedesktop.org/~keithp/xserver.git fix-private-usage
I'll wait for comments on this email and go fix up the code to match
before submitting it for review. I'd love to have it prepared as a
sequence of small patches, but frankly given the ABI breakage, I'm not
sure that makes sense. The overall diffstat from master is:
129 files changed, 1213 insertions(+), 912 deletions(-)
What I don't have yet is any kind of memory usage or performance
comparison with the current devPrivates implementation. I should
probably do that just to document the benefit in more concrete terms,
but I'd rather not bother and just get this merged fairly soon.
As you might imagine, touching 129 files in the core of the X server
will cause conflicts with a bunch of the queued patches; I'd prefer to
merge my patch first (of course), in particular it has a fairly tight
interaction with the MAXSCREENS work.
Most pressing is the API change; the actual privates implementation is
probably fine, but could use improvement, especially as it interacts
with XSELinux. So, please focus your attention on the proposed API and
plan on ignoring the implementation for now :-)
Here's the complete new privates.h file for reference:
/***********************************************************
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHOR BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
******************************************************************/
#ifndef PRIVATES_H
#define PRIVATES_H 1
#include <X11/Xdefs.h>
#include <X11/Xosdefs.h>
#include <X11/Xfuncproto.h>
#include "misc.h"
/*****************************************************************
* STUFF FOR PRIVATES
*****************************************************************/
typedef struct _Private PrivateRec, *PrivatePtr;
typedef enum {
/* If you need a private in everything, then you get this type
*/
PRIVATE_ALL,
/* Otherwise, you get a private in just the requested structure
*/
/* core privates */
PRIVATE_SCREEN,
PRIVATE_DEVICE,
PRIVATE_CLIENT,
PRIVATE_PROPERTY,
PRIVATE_SELECTION,
PRIVATE_EXTENSION,
PRIVATE_WINDOW,
PRIVATE_PIXMAP,
PRIVATE_GC,
PRIVATE_CURSOR,
PRIVATE_CURSOR_BITS,
PRIVATE_COLORMAP,
/* extension privates */
PRIVATE_DBE_SCREEN,
PRIVATE_DBE_WINDOW,
PRIVATE_DAMAGE,
PRIVATE_GLYPH,
PRIVATE_GLYPHSET,
PRIVATE_PICTURE,
/* last private type */
PRIVATE_LAST,
} DevPrivateType;
typedef struct _DevPrivateKeyRec {
int offset;
int size;
Bool initialized;
DevPrivateType type;
CallbackListPtr initfuncs;
CallbackListPtr deletefuncs;
struct _DevPrivateKeyRec *next;
} DevPrivateKeyRec, *DevPrivateKey;
/*
* Register callbacks to be called on private allocation/freeing.
* The calldata argument to the callbacks is a PrivateCallbackPtr.
*/
typedef struct _PrivateCallback {
DevPrivateKey key; /* private registration key */
pointer *value; /* address of private pointer */
} PrivateCallbackRec;
/*
* Register a new private index for the private type.
*
* If you just need to store a pointer, ask for zero byte
* and you will be able to use dixSetPrivate to store a pointer
* and dixLookupPrivate to fetch it.
*
* If you want to store more data in every object, you can ask for
* a number of bytes, but you will not be able to use dixSetPrivate
* and dixLookupPrivate and will instead use dixLookupPrivateAddr
* to get a pointer to the base of your pre-allocated storage.
*/
extern _X_EXPORT Bool
dixRegisterPrivateKey(DevPrivateKey key, DevPrivateType type, unsigned size);
/*
* Get the address of the private storage
*/
static inline void *
dixGetPrivateAddr(PrivatePtr *privates, const DevPrivateKey key)
{
assert(key->initialized);
return (char *) (*privates) + key->offset;
}
/*
* Get a private pointer
*/
static inline void *
dixGetPrivate(PrivatePtr *privates, const DevPrivateKey key)
{
assert (key->size == 0);
return *(void **) dixGetPrivateAddr(privates, key);
}
/*
* Set a private pointer.
*/
static inline void
dixSetPrivate(PrivatePtr *privates, const DevPrivateKey key, pointer val)
{
assert (key->size == 0);
*(pointer *) dixGetPrivateAddr(privates, key) = val;
}
#include "dix.h"
#include "resource.h"
/*
* For privates with defined storage, return the address of the
* storage. For privates without defined storage, return the pointer
* contents
*/
static inline pointer
dixLookupPrivate(PrivatePtr *privates, const DevPrivateKey key)
{
assert (key->initialized);
if (key->size)
return dixGetPrivateAddr(privates, key);
else
return dixGetPrivate(privates, key);
}
static inline void *
dixLookupPrivateAddr(PrivatePtr *privates, const DevPrivateKey key)
{
assert (key->initialized);
return dixGetPrivateAddr(privates, key);
}
/*
* Register a callback for initializing or deleting a structure containing
* the specified devprivate key
*/
extern _X_EXPORT int
dixRegisterPrivateInitFunc(const DevPrivateKey key,
CallbackProcPtr callback, pointer userdata);
extern _X_EXPORT int
dixRegisterPrivateDeleteFunc(const DevPrivateKey key,
CallbackProcPtr callback, pointer userdata);
/*
* Initialize privates by zeroing them and calling the init callbacks
*/
void
_dixInitPrivates(PrivatePtr *privates, void *addr, DevPrivateType type);
#define dixInitPrivates(o, v, type) _dixInitPrivates(&(o)->devPrivates, (v), type);
/*
* Clean up privates by calling the delete callbacks
*/
void
_dixFiniPrivates(PrivatePtr privates);
#define dixFiniPrivates(o) _dixFiniPrivates((o)->devPrivates)
/*
* Allocates private data at object creation time. Required
* for all objects other than ScreenRecs.
*/
extern _X_EXPORT void *
_dixAllocateObjectWithPrivates(unsigned size, unsigned clear, unsigned offset, DevPrivateType type);
#define dixAllocateObjectWithPrivates(t, type) (t *) _dixAllocateObjectWithPrivates(sizeof(t), sizeof(t), offsetof(t, devPrivates), type)
extern _X_EXPORT void
_dixFreeObjectWithPrivates(void *object, PrivatePtr privates);
#define dixFreeObjectWithPrivates(o) _dixFreeObjectWithPrivates(o, (o)->devPrivates)
/*
* Frees separately allocated private data (screens and clients)
*/
extern _X_EXPORT void
dixFreePrivates(PrivatePtr privates);
/*
* Allocates private data separately from main object (clients and colormaps)
*/
extern _X_EXPORT Bool
dixAllocatePrivates(PrivatePtr *privates, DevPrivateType type);
/*
* Return size of privates for the specified type
*/
extern _X_EXPORT int
dixPrivatesSize(DevPrivateType type);
/*
* Resets the subsystem, called from the main loop.
*/
extern _X_EXPORT void
dixResetPrivates(void);
/*
* Looks up the offset where the devPrivates field is located.
* Returns -1 if the specified resource has no dev privates
*/
extern _X_EXPORT int
dixLookupPrivateOffset(RESTYPE type);
/*
* Convenience macro for adding an offset to an object pointer
* when making a call to one of the devPrivates functions
*/
#define DEVPRIV_AT(ptr, offset) ((PrivatePtr *)((char *)(ptr) + offset))
#endif /* PRIVATES_H */
--
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/20100428/8451ce1f/attachment-0001.pgp>
More information about the xorg-devel
mailing list