-
Notifications
You must be signed in to change notification settings - Fork 179
lifecycle_for_hugepage: Use dynamic page size and improve cleanup #6624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lifecycle_for_hugepage: Use dynamic page size and improve cleanup #6624
Conversation
Key changes: 1. Replace hardcoded page size mapping with dynamic one; 2. Ensure proper hugepage release in teardown; Signed-off-by: Liang Cong <[email protected]>
WalkthroughConfiguration file hardcodes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes blend simple configuration hardcoding with moderate logic refactoring involving dynamic parameter resolution and expanded scenario handling. Review requires understanding the page size mapping strategy and scenario-specific behaviors, but changes remain localized without major architectural shifts. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/memory/memory_backing/lifecycle_for_hugepage.py (2)
56-60: Good: Dynamic page size retrieval improves portability.Replacing the hardcoded page size mapping with
memory.get_huge_page_size()correctly adapts to different system configurations.However, remove the unnecessary
int()cast on line 60:- params['set_pagenum'] = int(math.floor(total_hugepage_mem/set_pagesize)) + params['set_pagenum'] = math.floor(total_hugepage_mem/set_pagesize)
math.floor()already returns an integer in Python 3.
73-80: Remove unnecessaryint()cast.
math.floor()returns an integer in Python 3, making theint()cast on line 75 redundant.- params['vm_nr_hugepages'] = int(math.floor(total_hugepage_mem/default_hugepage_size)) + params['vm_nr_hugepages'] = math.floor(total_hugepage_mem/default_hugepage_size)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/memory/memory_backing/lifecycle_for_hugepage.cfg(1 hunks)libvirt/tests/src/memory/memory_backing/lifecycle_for_hugepage.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
libvirt/tests/src/memory/memory_backing/lifecycle_for_hugepage.py
75-75: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
🔇 Additional comments (4)
libvirt/tests/cfg/memory/memory_backing/lifecycle_for_hugepage.cfg (1)
45-45: LGTM! Config aligns with dynamic page size handling.The hardcoded "0" is correct for the
default_hugepage_sizescenario, as the Python code now dynamically calculates page sizes viasub_update()at runtime.libvirt/tests/src/memory/memory_backing/lifecycle_for_hugepage.py (3)
70-71: LGTM! Correctly extends dynamic page size handling.Including
'default_hugepage_size'in the condition ensures both scenarios that require system-default page sizes callsub_update()for dynamic calculation.
84-84: LGTM! Correctly includes '1G' scenario for aarch64.Adding
'1G'to the scenario list properly setsHugePages_Freefor 1G hugepage tests, as confirmed by the passing test results.
211-218: LGTM! Proper cleanup with correct page size.Retrieving
set_pagesizefrom params and passing it toset_kernel_hugepages()during teardown ensures hugepages are properly released using the correct page size that was configured during test setup.
|
@dzhengfy please help review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Key changes:
Test results:
x86_64:
(1/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_page_size: STARTED
(1/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_page_size: PASS (104.81 s)
(2/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_hugepage_size: STARTED
(2/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_hugepage_size: PASS (109.37 s)
(3/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.1G: STARTED
(3/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.1G: PASS (123.04 s)
(4/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.0: STARTED
(4/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.0: PASS (32.07 s)
(5/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.scarce_mem: STARTED
(5/5) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.scarce_mem: PASS (34.36 s)
aarch64:
(1/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_page_size: STARTED
(1/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_page_size: PASS (87.33 s)
(2/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_hugepage_size: STARTED
(2/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.default_hugepage_size: PASS (101.12 s)
(3/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.1G: STARTED
(3/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.1G: SKIP: The hugepage size '1048576' is not supported on current arch (support: [16777216, 2048, 524288])
(4/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.2M: STARTED
(4/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.2M: PASS (96.49 s)
(5/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.16G: STARTED
(5/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.16G: PASS (92.80 s)
(6/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.64K: STARTED
(6/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.64K: SKIP: The hugepage size '64' is not supported on current arch (support: [16777216, 2048, 524288])
(7/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.32M: STARTED
(7/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.32M: SKIP: The hugepage size '32768' is not supported on current arch (support: [16777216, 2048, 524288])
(8/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.0: STARTED
(8/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.0: PASS (15.77 s)
(9/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.scarce_mem: STARTED
(9/9) type_specific.io-github-autotest-libvirt.memory.backing.lifecycle.memory_hugepage.scarce_mem: PASS (18.83 s)
Summary by CodeRabbit