Cleanup qmemman #694
Cleanup qmemman #694
Conversation
ben-grande
force-pushed
the
qmemman-cleanup
branch
from
186d4c8 to
25ab744
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #694 +/- ##
==========================================
- Coverage 70.89% 70.82% -0.08%
==========================================
Files 61 61
Lines 13463 13473 +10
==========================================
- Hits 9545 9542 -3
- Misses 3918 3931 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ben-grande
force-pushed
the
qmemman-cleanup
branch
from
25ab744 to
466b7bd
Compare
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025071903-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 tests10 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 10 fixed
Unstable testsPerformance TestsPerformance degradation:9 performance degradations
Remaining performance tests:63 tests
|
|
openQA says "no": |
ben-grande
force-pushed
the
qmemman-cleanup
branch
from
466b7bd to
7e40b86
Compare
|
|
This one feels like pylint/mypy should notice? Maybe adding some type hints would help? |
There is a chance bigger than 0 that it might say "yes" now. I updated the code on my testbench, restarted qubesd and qubes-qmemman, started some domains, shutdown others, started various domains, updated all templates. No errors so far. |
ben-grande
force-pushed
the
qmemman-cleanup
branch
3 times, most recently
from
73622c2 to
91570da
Compare
ben-grande
force-pushed
the
qmemman-cleanup
branch
2 times, most recently
from
322ee00 to
41d7138
Compare
| # Overhead of per-page Xen structures, taken from OpenStack | ||
| # nova/virt/xenapi/driver.py | ||
| # see https://wiki.openstack.org/wiki/XenServer/Overhead | ||
| # we divide total and free physical memory by this to get | ||
| # "assignable" memory |
There was a problem hiding this comment.
This comment belongs to MEM_OVERHEAD_FACTOR - if you move one, move the other too.
There was a problem hiding this comment.
I moved the top part of the commend until see link because the rest we divide appears to be related to the code after.
qubes/qmemman/systemstate.py
Outdated
|
|
||
| def clear_outdated_error_markers(self) -> None: | ||
| # Clear outdated errors. | ||
| for _, dom in self.dom_dict.items(): |
There was a problem hiding this comment.
If you don't use key, the maybe iterate over values()?
qubes/qmemman/systemstate.py
Outdated
|
|
||
| for i in self.domdict.keys(): | ||
| self.domdict[i].no_progress = False | ||
| for _, dom in self.dom_dict.items(): |
qubes/qmemman/systemstate.py
Outdated
| # domain not responding to memset requests, remove it | ||
| # from donors | ||
| self.domdict[i].no_progress = True | ||
| dom.no_progress = True |
There was a problem hiding this comment.
Later commit fixes it, but this one mixes prev_mem_actual iteration) with self.domdict - dom is the former here, not the latter. (and similar few lines above and below). Please adjust this commit to not introduce a bug temporarily.
qubes/qmemman/systemstate.py
Outdated
| def xs_wrapper(self, key, value=None) -> Optional[str]: | ||
| if value: | ||
| self.xs.write("", key, value) | ||
| return None | ||
| return self.xs.read("", key) |
There was a problem hiding this comment.
Please don't, this doesn't save much (just one empty parameter), but makes it harder to see if it's a read or write (and easier to make a mistake).
OTOH the get_xs_path helper is useful change.
qubes/tests/qmemman.py
Outdated
|
|
||
|
|
||
| def MB(val): | ||
| def in_megabyte(val): |
There was a problem hiding this comment.
The idea of this function was to be concise, the new name kinda fails at that... If you don't like a function MB, maybe define it as a constant to multiply by instead?
qubes/tools/qmemmand.py
Outdated
| def xs_wrapper(self, key, value=None): | ||
| if value: | ||
| self.handle.write("", key, value) | ||
| return None | ||
| return self.handle.read("", key) | ||
|
|
There was a problem hiding this comment.
The same comment as in the other place.
- Enable pylint; - Rename keyword 'id' or variable 'i' to 'domid', which is always xid but better to use a VMM agnostic name; - Move constants to module scope; - Pylint trips on dictionary iteration of .keys() with 'consider-using-dict-items' and 'unnecessary-dict-index'. Fixing that helped find some useless assignments which were already objects of the dictionary; - Assign current domain dictionary domid looping to an object for easier reading and modification; and - Separate 'do_balance()' to avoid 'too-many-nested-blocks'.
ben-grande
force-pushed
the
qmemman-cleanup
branch
from
05c3799 to
d03bc0b
Compare
|
All issues were addressed. |
3 participants
Plan: