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

arch: Remove the abuse of spinlock in smp boot #15728

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

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

What's really need is a done signal sent from the secondary cpu to the boot cpu, so let's simplify the logic by:

  1. Change the spinlock to a bool flag
  2. Set the flag to true in the secondary cpu
  3. Wait the flag set in the boot cpu before continue booting

This also remove all bad usage of spinlock from the code base:

  1. Lock spinlock in one thread, but unlock in a different thread
  2. Lock spinlock twice in one thread, but unlock only once

Impact

Normalize the spinlock usage to avoid the future abuse by copy/paste

Testing

local

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: sparc Issues related to the SPARC architecture Arch: xtensa Issues related to the Xtensa architecture labels Jan 30, 2025
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Jan 30, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 30, 2025

[Experimental Bot, please feedback here]

This PR description does not fully meet the NuttX requirements. While it provides a decent summary of the what and why, it lacks crucial details needed for proper review. Here's a breakdown:

Missing/Insufficient Information:

  • Summary:

    • Lacks a clear statement of necessity. "Simplify logic" is vague. Explain the problem the current logic causes (e.g., race conditions, deadlocks, inefficiency).
    • Doesn't specify the functional part of the code being changed (e.g., boot process, specific driver, core kernel).
    • "Bad usage of spinlock" needs more detail. What are the specific negative consequences of this bad usage?
  • Impact:

    • Needs to address all impact categories explicitly, even if the answer is "NO". Just saying "Normalize spinlock usage" is insufficient. Consider:
      • Is new feature added? Is existing feature changed? (Yes, existing boot process is changed)
      • Impact on user: (Likely no, but state it explicitly)
      • Impact on build: (Probably no, but confirm)
      • Impact on hardware: (Specify which architectures/boards are affected)
      • Impact on documentation: (Does this change require documentation updates?)
      • Impact on security: (Explicitly state "NO" if none)
      • Impact on compatibility: (Crucially, does this break any existing code/configurations?)
  • Testing:

    • "local" is completely inadequate. Provide specifics:
      • Build Host(s): OS, CPU architecture, compiler version
      • Target(s): Architecture (simulated or real hardware), board and configuration
      • Testing logs before change: Empty logs are unacceptable. Demonstrate the problem the PR fixes.
      • Testing logs after change: Show that the problem is resolved and no new issues are introduced. Include relevant log output that demonstrates the fix in action.

How to Improve:

  1. Elaborate on the Problem: Clearly describe the issue with the current spinlock usage and its consequences. Provide concrete examples of how the misuse leads to problems.

  2. Detailed Impact Assessment: Address all impact categories explicitly. Even if there's no impact, state "NO" and briefly explain why (e.g., "Impact on user: NO - This change is internal to the boot process and does not affect user functionality").

  3. Thorough Testing Information: Provide specific details about your testing environment and include relevant log output before and after the change. This is crucial for reviewers to verify the fix and assess its impact. Show how you reproduced the problem before the change and how the problem is gone after the change.

By providing this missing information, the PR will be much easier to review and more likely to be accepted.

What's really need is a done signal sent from the secondary
cpu to the boot cpu, so let's simplify the logic by:
1.Change the spinlock to a bool flag
2.Set the flag to true in the secondary cpu
3.Wait the flag set in the boot cpu before continue booting

This also remove all bad usage of spinlock from the code base:
1.Lock spinlock in one thread, but unlock in a different thread
2.Lock spinlock twice in one thread, but unlock only once

Signed-off-by: Xiang Xiao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: sparc Issues related to the SPARC architecture Arch: xtensa Issues related to the Xtensa architecture 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.

[BUG] Espressif devices are not booting after PR 14578
3 participants