Do not use grant tables for allocating framebuffer #233
Conversation
|
I can confirm it indeed does fix this issue. On a system with 4K screen, without this change the Xorg takes over 90MB RSS, and with 300MB sys-net (HVM, with wired+wireless devices) results in OOM-killer killing it. With this change, nothing gets killed, and Xorg takes below 1MB RSS (and there is some swap usage). Virtual size of Xorg is similar in both cases, about 270MB (as expected). Note, this will not only fix systems with huge monitors, but also reduce RAM usage in all the VMs. |
|
And this I tested too, after connecting external display one can interact with windows anywhere on the screen and it appears nothing explodes (at least after a short tests). |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061202-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061004-4.3&flavor=update
Failed tests13 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 10 fixed
Unstable testsPerformance TestsPerformance degradation:8 performance degradations
Remaining performance tests:64 tests
|
I can't recall a specific reason. Most likely I was just making sure that all pixmaps are gnt backed. Based on your testing and thinking about it again, I think your change makes sense. Unlike the old method the gnt based protocol doesn't even have a method to specify an offset. So you would have to share the complete pixmap (or in theory some page aligned part of it). So the X server would need to give the framebuffer pixmap to a client and that would need to map it. Based on that I would even go so far and throw it out completely. Do we have someone that uses the glamor setup (I see it was added around 2021)? Then we could let them test drive it, to be really sure. |
xf86-video-dummy/src/dummy_driver.c
Outdated
| if (!newFBBase) { | ||
| xf86DrvMsg(pScrn->scrnIndex, X_ERROR, | ||
| "Unable to set up a virtual screen size of %dx%d, " | ||
| "cannot allocate memory (%lu bytes)\n", |
There was a problem hiding this comment.
| "cannot allocate memory (%lu bytes)\n", | |
| "cannot allocate memory (%zu bytes)\n", |
nitpick: %zu for size_t
Hmm, on openQA the guivm tests failed due to qubes-menu dying with |
It was added together (and for) GVT-g support, and the most important part (dom0 kernel patch) is not in our repos, as it was too much risk. That technology is basically dead. Theoretically glamor should work also with any GPU passthrough, and did managed to get it initialized (at least according to X server logs), but I'm not really sure how to test it properly. I can tell it didn't crash (even without limiting this change to non-glamor case), but I can't be sure it actually exercised all possible paths. |
Most recent yes, but earlier did pass (see edits history of that report, especially the initial comment) |
Given that state and that it's not likely needed, sounds like it would be fine to remove that case and if someone will work on it again in the future they can re-add it, should it actually turn out to be necessary. OTOH keeping that if also doesn't cost much. I'm fine either way.
I see, great. |
What I'm worried about is that FBBase is given out in |
|
That's a good point, I have to take another look, but I'm wondering if DGA ever worked, for example |
|
Sorry, meant |
No idea, especially as you pointed out, there appears to be no way to actually see the framebuffer from dom0/gui domain... DGA is there for quite a bit longer than glamor (git blame says the original import of the dummy driver in 2013, and unmodified since then). If it was always broken and nobody complained, I guess we can simply remove it completely. OTOH, with GPU passthrough there may be use cases where something would like to put rendered result via DGA directly (some games?) - in which case it might be better to keep it for now (disabled? we have configure option for it) and fix it later. |
So having looked at it:
I would vote for dropping it. But if you prefer to just disable it, fine with me too. |
Ok, lets do it then. |
It appears to be broken forever. Since nobody complained, it looks like nobody actually need it. Drop it. See discussion at QubesOS#233
marmarek
force-pushed
the
no-grants-fbbase
branch
from
a6fb44b to
fc21a08
Compare
It appears to be broken forever. Since nobody complained, it looks like nobody actually need it. Drop it. See discussion at QubesOS#233
marmarek
force-pushed
the
no-grants-fbbase
branch
from
fc21a08 to
01cc96b
Compare
If glamor is not used (which is default), FBBasePriv seems to be unused. GUI agent doesn't share the root window with GUI daemon. If glamor is used, FBBasePriv probably unused in practice too - the GUI agent doesn't support sharing part of the pixmap anyway, but I'm not 100% sure about it (see qubes_create_screen_resources() - the "pixmap" of the root window is passed to glamor, and it isn't clear if it wouldn't end up used in some window that is handled by the GUI agent). Based on this observation, use normal memory for the framebuffer. The main benefit is not having the framebuffer mlock()-ed, which helps especially with small VMs. But also, allocating it as normal userspace memory gives the kernel more flexibility in finding memory for it (it doesn't need to be physically continuous, etc). Due to the glamor case, leave the grant-based FBBase allocation in the code, but disabled by default. And leave a #define to re-enable it, with appropriate comment. Fixes QubesOS/qubes-issues#9992
It appears to be broken forever. Since nobody complained, it looks like nobody actually need it. Drop it. See discussion at QubesOS#233
If FBBase is not backed by grant tables, it's now possible to reallocate it. This is especially useful when new external display is connected and the overall resolution is increased. Note the realloc() call may change FBBase address. In practice, FBBase is given to fbScreenInit, and it does save it at some point (fbScreenInit->fbFinishScreenInit->miScreenInit->miScreenDevPrivateInit), but then it's used only to attach it to the screen pixmap, which is updated via ModifyPixmapHeader() call few lines below anyway. This change adjusts also pScrn->videoRam. But fortunately X server seems to use it only at startup, and only to validate initial modes list. Fixes QubesOS/qubes-issues#7448
marmarek
force-pushed
the
no-grants-fbbase
branch
from
01cc96b to
49420c5
Compare
|
Then the last question is about |
|
Ugh, now I found this in one of VMs: I haven't noticed any breakage, like missing content of some window, but nevertheless it tried to dump content of a window without grants. OTOH, it appears as at least one of those messages are related to this window: which definitely is not the root window. Maybe unrelated issue? Some race condition or something? |
|
Hmm ... Did you notice any effects, like missing window content? Do you have a reproducer? If not, one idea would be to make, for testing, that error fatal and let openQA run and see if we ever hit it there.
Possibly. Or given the window size, likely. But we should still figure out where that pixmap is coming from. |
Not really, everything looks normal.
Yes. It appears as starting chromium triggers this. I tried to capture that window content in the VM using |
|
So turns out to be a race. You get the error when I could easily trigger it with other programs too. For details see #236. |
|
Let's see which corner that PR will uncover ... ;] |
It seems like the framebuffer is never shared with the GUI daemon directly
(at least without GPU passthrough), so it doesn't need to be backed by grant tables. Allocating it as normal userspace memory has several benefits:
memory)
There is one corner case I'm not sure about when an actual GPU is available and
glamor is used. So, in this case I keep the old behavior.
See commit messages for more details.
@HW42 can you sanity check this? You added grant tables support initially, so
maybe you remember why it was done this way...
Fixes QubesOS/qubes-issues#7448
Fixes QubesOS/qubes-issues#9992