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

libc/atomic: decoupling atomic and spinlock to avoid recursion #14198

Merged
merged 1 commit into from
Oct 13, 2024

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Oct 12, 2024

Summary

libc/atomic: decoupling atomic and spinlock to avoid recursion

  1. use irq save in AMP mode
  2. use critical section in SMP mode

Impact

N/A

Testing

gcc14 with sim/smp

@github-actions github-actions bot added Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Oct 12, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 12, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

Meets Requirements? Partially

Breakdown:

  • Summary:
    • Adequate: Provides a brief overview of the change (decoupling atomic and spinlock) and the methods used.
    • Missing: Lacks context on why this change is necessary. What problems were caused by the previous coupling? This is crucial for reviewers to understand the PR's purpose.
  • Impact:
    • Insufficient: "N/A" is rarely accurate for code changes. This section needs careful consideration:
      • Feature Changes: Even if no new features are directly exposed, internal changes can impact existing functionality.
      • Build/Hardware/Documentation: While the impact might be minimal, it's unlikely to be truly "N/A". Be specific. Does this affect certain architectures more than others?
      • Security/Compatibility: These are critical and cannot be dismissed outright. Atomic operations and spinlocks are fundamental to concurrency; any change has potential implications that MUST be addressed, even if the answer is "no impact, and here's why...".
  • Testing:
    • Insufficient:
      • Build Host: Provide OS and compiler versions for reproducibility.
      • Targets: "sim/smp" is vague. Specify the exact simulator/emulator and configurations used.
      • Logs: "your testing logs here" is unhelpful. Include ACTUAL logs demonstrating the issue BEFORE the change, and the improved behavior AFTER.

Recommendation:

Do not submit this PR in its current state. Address the missing information:

  1. Expand the Summary: Explain the problem the change solves.
  2. Thoroughly Analyze Impact: For each point, either state "No impact" with a brief justification OR describe the specific impact.
  3. Provide Detailed Testing Information: Include full build host details, specific target configurations, and relevant log snippets that demonstrate the change's effect.

1. use irq save in AMP mode
2. use critical section in SMP mode

Signed-off-by: chao an <[email protected]>
* Private Data
****************************************************************************/

#ifdef CONFIG_SMP
Copy link
Contributor

Choose a reason for hiding this comment

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

hardware must support atomic operation in SMP, so we.don't.need this.codd.block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but some architectures may not fully support all atomic operations, eg: -march=armv7e-m does not support atomic8(64 bit) OpenAMP/libmetal#301

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@xiaoxiang781216 xiaoxiang781216 merged commit c37b0de into apache:master Oct 13, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants