RDKEMW-15176: set swap memory limit for containers#419
RDKEMW-15176: set swap memory limit for containers#419goruklu wants to merge 1 commit intordkcentral:release/v3.14from
Conversation
Added "swap" limit in the OCI config templates to enable OOM killing when memory limit is reached when swap memory is enabled. Signed-off-by: Gurdal Oruklu <gurdal_oruklu@comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR updates Dobby’s OCI config generation to include a memory swap limit (and swappiness) alongside the existing memory limit, aiming to ensure containers are OOM-killed when memory constraints are hit even if host swap is enabled.
Changes:
- Add
"swap"and"swappiness"fields to the OCI JSON templates’linux.resources.memorysection. - Introduce a new template variable (
MEM_SWAP_LIMIT) and populate it inDobbySpecConfig::processMemLimitasmemLimit + 200MB.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| bundle/lib/source/templates/OciConfigJsonVM1.0.2-dobby.template | Adds swap and swappiness to the VM OCI memory cgroup template section. |
| bundle/lib/source/templates/OciConfigJson1.0.2-dobby.template | Adds swap and swappiness to the standard OCI memory cgroup template section. |
| bundle/lib/source/DobbySpecConfig.cpp | Adds MEM_SWAP_LIMIT template key and computes it from memLimit plus a fixed extra. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1274,6 +1278,7 @@ bool DobbySpecConfig::processMemLimit(const Json::Value& value, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
MEM_SWAP_LIMIT is computed as memLimit + MEM_SWAP_LIMIT_EXTRA_BYTES with unsigned arithmetic; if a caller supplies a large memLimit this can overflow and silently wrap, producing an invalid/too-small swap limit. Consider validating an upper bound (e.g., reject values where memLimit > UINT_MAX - MEM_SWAP_LIMIT_EXTRA_BYTES) and/or switching to a string/64-bit safe path when writing to the template dictionary.
| // Prevent unsigned overflow when computing the swap limit | |
| if (memLimit > (UINT_MAX - MEM_SWAP_LIMIT_EXTRA_BYTES)) | |
| { | |
| AI_LOG_ERROR("memory limit too large, would overflow swap limit"); | |
| return false; | |
| } |
|
|
||
| static constexpr unsigned MEM_SWAP_LIMIT_EXTRA_BYTES = 200u * 1024u * 1024u; |
There was a problem hiding this comment.
MEM_SWAP_LIMIT_EXTRA_BYTES introduces a hard-coded 200MB policy with no rationale or configurability; this makes the runtime behavior difficult to tune across devices. Consider documenting why 200MB is required (and what it represents) or making it a configurable setting tied to platform defaults.
| static constexpr unsigned MEM_SWAP_LIMIT_EXTRA_BYTES = 200u * 1024u * 1024u; | |
| // Additional swap limit headroom (in bytes) added on top of MEM_LIMIT. | |
| // The default of 200MB has historically been used as a conservative value | |
| // to account for kernel overhead and transient memory spikes on typical | |
| // target devices. If porting to hardware with significantly different | |
| // memory characteristics, this value can be overridden at build time by | |
| // defining MEM_SWAP_LIMIT_EXTRA_BYTES (for example via | |
| // -DMEM_SWAP_LIMIT_EXTRA_BYTES=<bytes> in CXXFLAGS). | |
| #ifndef MEM_SWAP_LIMIT_EXTRA_BYTES | |
| static constexpr unsigned MEM_SWAP_LIMIT_EXTRA_BYTES = 200u * 1024u * 1024u; | |
| #endif |
| "limit": {{MEM_LIMIT}}, | ||
| "swap": {{MEM_SWAP_LIMIT}}, | ||
| "swappiness": 60 |
There was a problem hiding this comment.
This template sets swap greater than limit (MEM_SWAP_LIMIT is derived from MEM_LIMIT plus extra bytes). That means the container can exceed the memory limit by swapping, so it will not be OOM-killed at the point it reaches the memory limit (which conflicts with the PR title/description). If the goal is to trigger OOM at the configured memory limit when host swap is enabled, consider setting swap equal to the memory limit (cgroup v1 semantics) or otherwise aligning the swap value with the intended kill threshold.
| "swap": {{MEM_SWAP_LIMIT}}, | ||
| "swappiness": 60 |
There was a problem hiding this comment.
"swappiness": 60 is a new, hard-coded behavioral change that will affect memory reclamation/swap behavior for all containers using this template. If this value is required for the OOM-kill fix, consider making it configurable (template placeholder) or omitting it to keep existing default behavior, and document the intended impact.
| "swap": {{MEM_SWAP_LIMIT}}, | |
| "swappiness": 60 | |
| "swap": {{MEM_SWAP_LIMIT}}{{#MEM_SWAPPINESS_SECTION}}, | |
| "swappiness": {{MEM_SWAPPINESS}}{{/MEM_SWAPPINESS_SECTION}} |
| "limit": {{MEM_LIMIT}}, | ||
| "swap": {{MEM_SWAP_LIMIT}}, | ||
| "swappiness": 60 |
There was a problem hiding this comment.
This template sets swap greater than limit (MEM_SWAP_LIMIT is derived from MEM_LIMIT plus extra bytes). That means the container can exceed the memory limit by swapping, so it will not be OOM-killed at the point it reaches the memory limit (which conflicts with the PR title/description). If the goal is to trigger OOM at the configured memory limit when host swap is enabled, consider setting swap equal to the memory limit (cgroup v1 semantics) or otherwise aligning the swap value with the intended kill threshold.
| "swap": {{MEM_SWAP_LIMIT}}, | ||
| "swappiness": 60 |
There was a problem hiding this comment.
"swappiness": 60 is a new, hard-coded behavioral change that will affect memory reclamation/swap behavior for all containers using this template. If this value is required for the OOM-kill fix, consider making it configurable (template placeholder) or omitting it to keep existing default behavior, and document the intended impact.
| "swap": {{MEM_SWAP_LIMIT}}, | |
| "swappiness": 60 | |
| "swap": {{MEM_SWAP_LIMIT}}{{#MEM_SWAPPINESS_ENABLED}}, | |
| "swappiness": {{MEM_SWAPPINESS}}{{/MEM_SWAPPINESS_ENABLED}} |
Added "swap" limit in the OCI config templates to enable OOM killing when memory limit is reached when swap memory is enabled.
Description
What does this PR change/fix and why?
If there is a corresponding JIRA ticket, please ensure it is in the title of the PR.
Test Procedure
How to test this PR (if applicable)
Type of Change
Requires Bitbake Recipe changes?
meta-rdk-ext/recipes-containers/dobby/dobby.bb) must be modified to support the changes in this PR (beyond updatingSRC_REV)