Conversation
hugepages_allocated() previously only checked that *some* hugepages were free (HugePages_Free != 0). If a user increased --suricata-hugepages on a machine that already had hugepages mounted, the new allocation request was silently ignored. Add _parse_size_to_bytes() helper and compare the currently allocated hugepage memory (HugePages_Total * Hugepagesize from /proc/meminfo) against the requested amount, re-running dpdk-hugepages.py --setup only when the mounted amount is lower. Also validate --suricata-hugepages in pytest_configure and raise a clean pytest.UsageError on invalid input (e.g. 6X or abc) instead of a raw ValueError traceback from the session fixture. docs: document hugepages re-allocation behavior Update README.md (note after DEFAULT_HUGEPAGES, binary-search setup step) and pytest_start.sh -sh help text to describe that hugepages are re-allocated when the currently mounted amount is lower than the requested --suricata-hugepages value. hugepages: simplify allocation check and robust size parsing - Read /proc/meminfo stdout directly instead of writing to a temp file and reading it back with a second cat command. - Parse size strings loosely (any trailing unit chars) and validate the suffix against the multiplier table, so the space-separated form from /proc/meminfo (e.g. '2048 kB') is accepted. - Catch KeyError from the deferred suffix validation in _validate_hugepages_option. rename this commit conftest: try dpdk-hugepages --reserve as last-ditch effort If dpdk-hugepages.py --setup fails, attempt --reserve before giving up so hugepages can still be pinned. --reserve still runs after a successful --setup so the pages are actually reserved for Suricata. hugepages: account for double allocation when checking; lower default descriptors docs: simplify hugepages help text; restore default rx/tx descriptors docs: clarify hugepages re-allocation behavior
There was a problem hiding this comment.
Pull request overview
This pull request updates the Suricata test suite’s hugepages handling to be more robust and user-friendly by accepting human-readable memory sizes, improving the hugepages allocation check, and strengthening allocation error handling and logging.
Changes:
- Added size parsing/formatting helpers (
_parse_size_to_bytes,_bytes_to_size) and switched--suricata-hugepagesto parse human-readable sizes into bytes. - Updated hugepages allocation detection to compare allocated hugepage memory against the requested amount (instead of only checking “some free”).
- Improved hugepages allocation flow to attempt
dpdk-hugepages.py --setupand fall back to--reserve, with clearer logging/error reporting; updated README documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Adds a clarification note describing how DEFAULT_HUGEPAGES / --suricata-hugepages affects allocation behavior. |
| conftest.py | Introduces size parsing/formatting utilities, changes hugepages checks to be request-aware, and improves allocation error handling/logging. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| (e.g. ``2048 kB``) is also accepted. | ||
| """ | ||
| size = size.strip().upper() | ||
| match = re.fullmatch(r"(\d+)\s*(.+)?", size) |
There was a problem hiding this comment.
(.+)? says to optionally match any character one or more times. Hard to say how + and ? interact together and it might be implementation dependent. I think that you meant (.*), which will match anything (still including whitespace) for as many characters as it can (rest of the string, possibly "").
What you want is ([^\s]*), which will match all non-whitespace characters, until the first whitespace.
| try: | ||
| _, stderr = process_set_hugepages.run() | ||
| stderr_parts.append(stderr) | ||
| except executable.ExecutableProcessError as e: | ||
| logger.warning( | ||
| "dpdk-hugepages.py --setup failed (%s). Trying --reserve as a " | ||
| "last-ditch effort.", | ||
| e, | ||
| ) | ||
| try: | ||
| _, stderr = process_reserve_hugepages.run() | ||
| stderr_parts.append(stderr) | ||
| except executable.ExecutableProcessError as reserve_e: | ||
| logger.critical( | ||
| "Failed to allocate huge-pages (%s). Continuing with the " | ||
| "currently allocated huge-pages; tests that require more will " | ||
| "fail with a specific error.", | ||
| reserve_e, | ||
| ) | ||
| return |
There was a problem hiding this comment.
I would skip the "last ditch effort". LLMs love this kind of stuff, so that they can be considered "agentic", but in practice if --setup fails, the issue likely needs to be fixed manually anyway.
There was a problem hiding this comment.
I found this approach to be working. Setup only works for the first time allocation, but reallocation with setup does not want to work properly. The reserve workaround works. I will checkout if there is a way to make it work more normally though.
There was a problem hiding this comment.
I just tested on merlot with --setup 4G, --setup 8G and --setup 2G and catting /proc/meminfo always gave the correct values. This seems to be an issue with the machine where you tested it.
Notably merlot is on EL10, meanwhile you likely used one of the OL8.10 machines, so that could be it. I will check behavior on EL9 later today, which we will migrate to this weekend.
|
I have some miscellaneous fixes in a branch on my fork and it depends on some stuff from this PR, so would it be possible to have this done in the next few days, so that I can follow it up with some changes to how |
This pull request improves the handling of hugepages allocation for Suricata tests, making the process more robust and user-friendly. It introduces utilities to parse and format memory sizes, ensures the requested amount of hugepages is correctly allocated, and enhances error handling and logging during allocation. The changes also clarify documentation for users.
Improvements to hugepages allocation and configuration:
_parse_size_to_bytesand_bytes_to_sizeutility functions inconftest.pyto handle parsing and formatting of size strings like6G,512M, etc., ensuring consistent and accurate conversion between human-readable sizes and bytes.--suricata-hugepagespytest option to use the new parser, allowing users to specify hugepages in various formats (e.g.,6G,2048 kB).hugepages_allocatedto check if the currently allocated hugepages meet or exceed the requested amount, and to account for the fact that the setup script allocates double the requested amount.check_hugepagesto handle errors more gracefully, attempting both--setupand--reserve, and providing better logging and error reporting.Documentation updates:
README.mdwith a detailed explanation of theDEFAULT_HUGEPAGESsetting and its behavior, making it clearer for users how hugepages allocation works.Minor code cleanup:
conftest.pyto tidy up the code.