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: make the alignment length in mm consistent with Kasan #15397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Jan 2, 2025

Summary

mm: The alignment length in mm is consistent with Kasan

preceding will cause the mm alignment to be inconsistent with the kasan alignment

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

@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 Jan 2, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 2, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a title and attempts to address the summary and impact, it lacks crucial details and proper formatting.

Here's a breakdown of what's missing:

  • Summary: While the summary mentions what is being changed, it lacks clarity. It repeats the title and doesn't explain why this change is necessary. What problem does the inconsistency cause? What are the benefits of aligning mm with Kasan? There's no mention of related issues.
  • Impact: The PR placeholder text is left in. This section needs to be filled out with specifics. Every impact item needs a YES/NO answer followed by a description if the answer is YES. Even if the impact is NO across the board, the PR submitter should explicitly state this.
  • Testing: Completely empty. This section requires details about the testing environment (host and target) and, importantly, logs from before and after the change to demonstrate the fix. Simply stating "works as intended" is insufficient.

To be accepted, the PR needs significant revision to address these shortcomings. The submitter must:

  1. Expand the Summary: Explain the rationale behind the change. What problem does it solve? What are the benefits? Include links to related issues if applicable.
  2. Complete the Impact Assessment: Address each point specifically, even if the answer is NO.
  3. Provide Thorough Testing Information: Detail the testing environment and include "before" and "after" logs to demonstrate the change's effectiveness.

In short, the PR needs more context, detail, and evidence of testing to meet the NuttX requirements.

@W-M-R W-M-R changed the title mm: The alignment length in mm is consistent with Kasan mm: make the alignment length in mm consistent with Kasan Jan 2, 2025
@W-M-R W-M-R marked this pull request as draft January 2, 2025 07:15
@W-M-R W-M-R marked this pull request as ready for review January 2, 2025 14:10
mm/mm_heap/mm.h Outdated Show resolved Hide resolved
#define KASAN_SHADOW_SIZE(size) \
(KASAN_BYTES_PER_WORD * ((size) / KASAN_SHADOW_SCALE / KASAN_BITS_PER_WORD))
Copy link
Contributor

Choose a reason for hiding this comment

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

how about remove KASAN_SHADOW_SCALE

Copy link
Contributor Author

@W-M-R W-M-R Jan 6, 2025

Choose a reason for hiding this comment

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

After aligning the prefix and structure, mm and kasan can share the same ALIGN, or write
#define KASAN_SHADOW_SCALE MM_ALIGN

preceding will cause the mm alignment to be inconsistent with the kasan alignment

Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
union
{
mmsize_t preceding; /* Physical preceding chunk size */
uint8_t align[MM_ALIGN];
Copy link
Contributor

Choose a reason for hiding this comment

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

where extend mm_allocnode_s?

Copy link
Contributor Author

@W-M-R W-M-R Jan 7, 2025

Choose a reason for hiding this comment

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

Expanded by the last

struct mm_allocnode_s
{
xxxxx;
}aligned_data(MM_ALIGN);

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.

4 participants