Skip to content
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

fs/shm/shmfs_alloc.c: Allocate zero-initialized memory in flat build #15159

Merged

Conversation

jlaitine
Copy link
Contributor

POSIX requires that the shm objects are zero-initialized. This has been broken in some earlier commits (starting from 9af5fc5)

Also fix the flat build memory allocation to allocate both object data and payload in the same chunk (as the comment also suggests). This saves allocations and memory in a system with lots of shm objects.

Summary

This PR fixes errors in applications, which run in "CONFIG_BUILD_FLAT", and assume that newly allocated shared memory is zero-initialized.

This also reduces memory consumption in smaller devices, when each shared memory object is allocated in a single chunk as originally.

The errors were introduced earlier when the SHM allocations were cache-line aligned.

Impact

This corrects SHM allocation to return zero-initialized memory

Testing

Tested on an MPFS board, using flat-mode build and several hundred shm objects allocated.

@github-actions github-actions bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Dec 12, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 12, 2024

[Experimental Bot, please feedback here]

The PR mostly meets the NuttX requirements, but is missing some important details. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the changes.
  • Impact Section Addresses Key Areas: It acknowledges the impact on users and memory consumption.
  • Testing Mentions Platform: It specifies the target platform (MPFS board) and build type (flat-mode).

Weaknesses & Missing Information:

  • Missing Issue References: No related NuttX or NuttX Apps issue/PR is linked. Even if there isn't one, stating "N/A" is good practice.
  • Impact Section Incomplete: Several "Impact" fields are missing:
    • Impact on build: While it mentions "flat-mode build," it doesn't explain how the build process is affected by the changes. Does anything need to be reconfigured?
    • Impact on hardware: Specify which architectures are impacted. Even if it's "all," state it explicitly.
    • Impact on documentation: Does the documentation need updating to reflect these changes?
    • Impact on security, compatibility: Even if the answer is "NO," it should be explicitly stated for each.
  • Testing Insufficient:
    • Build Host Details Missing: Provide details about the build host OS, CPU, and compiler version.
    • Target Details Insufficient: "MPFS board" is not specific enough. Provide the exact board configuration used.
    • No Logs: The "Testing logs before/after change" sections are empty. While the description mentions observing behavior with "several hundred shm objects," concrete logs demonstrating the problem before the change and the fix after the change are crucial. Show how the zero-initialization was broken and how memory consumption changed. Quantitative data is ideal.

Recommendations for Improvement:

  1. Add Issue References: If no related issues exist, state "N/A."
  2. Complete Impact Section: Fill in all fields, even with "NO" if applicable. Justify "YES" answers with details. Example for build impact: "No changes to the build process itself, but applications using SHM in flat-mode builds might experience different behavior due to the zero-initialization fix."
  3. Provide Detailed Testing Information:
    • Specify build host details (OS, CPU, compiler).
    • Specify the exact target board and configuration.
    • Include actual logs demonstrating the issue before the change and the successful fix after the change. Include memory usage data or other quantifiable metrics to show the impact.

By addressing these points, the PR will be significantly stronger and adhere more closely to the NuttX requirements. This makes review easier and increases the chances of acceptance.

POSIX requires that the shm objects are zero-initialized. This has been broken
in some earlier commits (starting from 9af5fc5)

Also fix the flat build memory allocation to allocate both object data and payload
in the same chunk (as the comment also suggests). This saves allocations and memory
in a system with lots of shm objects.

Signed-off-by: Jukka Laitinen <[email protected]>
@jlaitine jlaitine force-pushed the shm_allocate_zero_initialized_memory branch from 08e1119 to 38b1c51 Compare December 12, 2024 12:04
@jlaitine
Copy link
Contributor Author

Edit: I noticed that the same zero-initialization problem existed also for PROTECTED build branch, so fixed that as well

@xiaoxiang781216 xiaoxiang781216 merged commit 366977b into apache:master Dec 12, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants