Bug 3053

Created attachment 2441 [details] [review]
Prototype patch

The prototype patch currently lacks support for allocating pixmaps via
|MapAlloc()| but I don't see how this can be done except in two ways:
a) edit each single Xfree86 loadable driver and add the support there
b) add a new function |FreePixmap()| which takes care about proper deallocation
(including MIT-SHM and Xinerama cases (actually the introduction of
|FreePixmap()| is a precondition for getting XineramaV2 working)), however this
would mean that a bump of the major version of the Xfree86 module loader ABI
would become mandatory as the drivers would have to call the function
|FreePixmap()| instead of |xfree()|.

[a] would be a painfull mess (which is beyond powers as this work+testing would
exceed my current amount of free time) and [b] likely won't happen in the
forseeable future so it may just be possible to add that API as-is and forward
[a] or [b] to a later point... ;-((

ajax:
Do you care to make a small patch which switches the allocation of the
front+backbuffers for the Mesa software renderer over from |malloc()| to
|MapMalloc()|, please (I don't know exactly where it does the
allocations/deallocations and it may be better that someone who knows the
details does the job...) ?

Comment on attachment 2441 [details] [review]
Prototype patch

I forgot one thing about the patch - it's mainly coming from Solaris/Xsun where
Sun added this API already long long ago.

Though in the Xsun case it was done by calling these functions from the cfb
pixmap allocate and deallocate functions, instead of requiring all DDX'es to
be changed.

(In reply to comment #4)
> Though in the Xsun case it was done by calling these functions from the cfb
> pixmap allocate and deallocate functions, instead of requiring all DDX'es to
> be changed.

"cfb"/"mfb" have become very rare in Xorg code and almost all consumers have
been switched over to "fb" (which is IMO a pity because "fb" lacks lots of
features present in the "cfb" vesion of Solaris (multiple concurrent
overlays/underlays and visuals with transparent colors come in mind)).
For "fb" the same applies as I have outlined in [b]: It cannot be done as some
of the loadable drivers allocate the framebuffer memory in their own fashion.
Such a change would break them.

could you give an example of a driver that has a non-standard pixmap allocator?

in general i'm opposed to new command-line arguments, we have too many as it is and they never get 
used.

also:

+     flags = reserveswap == FALSE ? MAP_PRIVATE|MAP_NORESERVE : MAP_PRIVATE;

Linux's mmap(3) man page suggests that MAP_NORESERVE is non-standard, and that specifying it may 
lead to a situation where you can segfault on write.  This would be less than ideal.

I think you also need a way to check for the existance of the MAP_ANON flag.  Yes, it's very common, 
but it's not in POSIX or SUSv2.

(In reply to comment #6)
> could you give an example of a driver that has a non-standard pixmap allocator?
> 
> in general i'm opposed to new command-line arguments, we have too many as
> it is and they never get used.

But there should be a way to turn this feature off on demand if it (possibly)
causes trouble (which I don't expect as Sun has tested the approach since ten or
more years). Additionally this should be a tuneable as the "best" value depends
on the architeture (mainly the MMU implementation and memory size).
And there are already command-line switches for other memory-specific stuff and
since this feature can improve the performane and long-term usuability of a
Xserver instance it may be worth to add it.
 
> also:
> 
> +     flags = reserveswap == FALSE ? MAP_PRIVATE|MAP_NORESERVE : MAP_PRIVATE;
> 
> Linux's mmap(3) man page suggests that MAP_NORESERVE is non-standard,
> and that specifying it may lead to a situation where you can segfault on 
> write.  This would be less than ideal.

1. #ifdef MAP_NORESERVE can be used to catch this
2. Linux already overcommits memory so even a plain, simple user process can
segfault when using |malloc()| when the VM system runs out of space.

> I think you also need a way to check for the existance of the MAP_ANON flag.  
> Yes, it's very common,  but it's not in POSIX or SUSv2.

#ifdef MAP_ANON can be used then, too.

(In reply to comment #6)
> could you give an example of a driver that has a non-standard pixmap 
> allocator?

Xnest, Xdmx, Postscript, PCL, any XAA driver, xf8_32bpp, xf4bgpp, xf1bpp and
nvidia's binary driver AFAIK.

(In reply to comment #8)
> (In reply to comment #6)
> > could you give an example of a driver that has a non-standard pixmap 
> > allocator?
> 
> Xnest, Xdmx, Postscript, PCL, any XAA driver, xf8_32bpp, xf4bgpp, xf1bpp and
> nvidia's binary driver AFAIK.

i think you're being a little dramatic here.  i don't see anything in (to pick a
random XAA driver) i128 that looks anything like a custom pixmap allocator.

i strongly suspect that the allocator itself is entirely within either XAA or
the framebuffer core (as alan indicated for cfb), which reduces the testing
burden significantly.

also i'm not quite clear whether you're talking about Pixmaps (things with an
XID that can be touched by clients) or pixmaps (chunks of memory holding
pixels).  not all pixmaps are Pixmaps.

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > could you give an example of a driver that has a non-standard pixmap 
> > > allocator?
> > 
> > Xnest, Xdmx, Postscript, PCL, any XAA driver, xf8_32bpp, xf4bgpp, xf1bpp and
> > nvidia's binary driver AFAIK.
> 
> i think you're being a little dramatic here.  i don't see anything in
> (to pick a random XAA driver) i128 that looks anything like a custom pixmap 
> allocator.

It's not a XAA driver, I am meaning the XAA core.

> i strongly suspect that the allocator itself is entirely within either XAA or
> the framebuffer core (as alan indicated for cfb), which reduces the testing
> burden significantly.

Any driver which will try to free the pixmap memory using |xfree()| will crash
then. Sun could simply add this as they control all their drivers but for Xorg
it's not possible as there is no control over 3rd party drivers. Right now the
spec does not forbid explicit re-/deallocation within a loadable driver so this
way is closed.

> also i'm not quite clear whether you're talking about Pixmaps (things with an
> XID that can be touched by clients) or pixmaps (chunks of memory holding
> pixels).

Mainly I am thinking about chunk of memory holding pixels.

> > i think you're being a little dramatic here.  i don't see anything in
> > (to pick a random XAA driver) i128 that looks anything like a custom pixmap 
> > allocator.
> 
> It's not a XAA driver, I am meaning the XAA core.

if the core is all that needs modifying then this is changing code in one place
rather than 30.  i have no fear of touching every driver and even less of
touching only one module.  this is what testsuites are for.
 
> Any driver which will try to free the pixmap memory using |xfree()| will crash
> then. Sun could simply add this as they control all their drivers but for Xorg
> it's not possible as there is no control over 3rd party drivers. Right now the
> spec does not forbid explicit re-/deallocation within a loadable driver so
> this way is closed.

this way is not closed, because this is 7.0, and we're allowed to make formerly
legal things illegal.  no third party will ship 7.0 drivers that crash within
five minutes.  they already have to make changes for 7.0 compatibility, at
minimum building dlloader modules by default.

this is starting to sound like a convincing case.

there's a need to start documenting these sorts of requirements and restrictions
that will arise in 7.0, so i can just add this to the list.

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

no.