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

mm/map: Remove MM_MAP_COUNT_MAX Kconfig and related check #14819

Closed
wants to merge 1 commit into from

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

since it's not necessary to limit the number of map entries.

Impact

remove the artificial limitation

Testing

ci

since it's not necessary to limit the number of map entries.

Signed-off-by: Xiang Xiao <[email protected]>
@github-actions github-actions bot added Area: Memory Management Memory Management issues Size: S The size of the change in this PR is small labels Nov 16, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 16, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements. The provided information is far too vague and lacks crucial details. Here's why:

  • Summary: "since it's not necessary to limit the number of map entries" is not a sufficient explanation. What map entries? What was the limit? Why was it there in the first place? What part of the codebase was changed? What is the functional change? This section needs to clearly articulate the what and why of the change. Issue references are missing.

  • Impact: "remove the artificial limitation" is similarly vague. What limitation? What are the ramifications of removing it? While it says it impacts the user, build, etc., by saying "NO", it provides no reasoning or context. For a proper assessment, consider the memory usage implications of removing this limit. Could it lead to out-of-memory errors in certain scenarios? Even if the answer to all impacts is "NO", a brief justification ("No change to user experience," for example) is preferable for clarity.

  • Testing: "ci" is insufficient. "ci" presumably means Continuous Integration, but that doesn't provide any specifics. What targets were tested? What tests were run? What were the results? Crucially, there are no "before" and "after" logs demonstrating the actual change in behavior. This section requires concrete evidence that the change works as intended and doesn't introduce regressions.

In short, the PR description needs significantly more detail and concrete information to be considered acceptable. It needs to provide a clear understanding of the change, its implications, and thorough verification through testing.

Comment on lines -101 to -106
config MM_MAP_COUNT_MAX
int "The maximum number of memory map areas for each task"
default 1024
---help---
The maximum number of memory map areas for each task.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoxiang781216 could you please elaborate further? Why isn't it necessary to limit the number of map entries? Which other alternative do we have? Please include this information in the commit message!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation could support any number map if the system has enough memory, so it's an artificial limitation and better to remove to simplify the configuration.

@XuNeo
Copy link
Contributor

XuNeo commented Nov 16, 2024

LTP test will fail if it's removed.

@patacongo
Copy link
Contributor

This could be security problem couldn't it? Any user application could cause a kernel simply by consuming memory regions.

Linux, apparently, restricts the number of regions to 65535. But this can be changed by writing to a proc file sysem entry: See https://www.ibm.com/docs/en/safer-payments/6.5?topic=settings-increasing-virtual-memory-map-size

That is a better solution than a configuration setting, but functionally very similar. This option should only be available to privileged applications.

@patacongo
Copy link
Contributor

Linux, apparently, restricts the number of regions to 65535. But this can be changed by writing to a proc file sysem entry: See https://www.ibm.com/docs/en/safer-payments/6.5?topic=settings-increasing-virtual-memory-map-size

POSIX permits implementation-specific limits on the number of memory regions, but does not require them. For example for mmap(): https://pubs.opengroup.org/onlinepubs/9799919799/functions/mmap.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Memory Management Memory Management 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.

6 participants