Skip to content

Conversation

@anchao
Copy link
Contributor

@anchao anchao commented Oct 13, 2024

Summary

group/exitinof: replace spin_lock to critical_section avoid deadlock

holding the irqsaved spinlock first and then take critical section may cause deadlock in SMP case,
replace spin_lock to critical_section avoid deadlock

Impact

SMP

Testing

sim/smp

@github-actions github-actions bot added Area: OS Components OS Components issues Size: XS The size of the change in this PR is very small labels Oct 13, 2024
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Size: XS The size of the change in this PR is very small labels Oct 13, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 13, 2024

[Experimental Bot, please feedback here]

The PR summary is somewhat vague and doesn't fully adhere to the NuttX requirements. Here's a breakdown and suggestions for improvement:

What's good:

  • Identifies a deadlock issue: The summary clearly states that the PR aims to fix a deadlock problem.
  • Mentions SMP: It correctly highlights that the issue pertains to Symmetric Multiprocessing (SMP) systems.
  • Provides a solution: Replacing spin_lock with critical_section is a reasonable approach to address potential deadlocks.

What needs improvement:

  • Insufficient detail in the summary:
    • Why is the change necessary? While mentioning "deadlock" is a start, it lacks a clear explanation of the scenario leading to the deadlock. How does "holding the irqsaved spinlock first and then taking the critical section" specifically cause a deadlock in an SMP environment?
    • What functional part of the code is being changed? Be more specific than just "group/exitinof." Which functions or data structures within that part are modified?
    • How does the change exactly work? Provide a slightly more detailed explanation of why replacing spin_lock with critical_section resolves the deadlock.
  • Missing information:
    • Related issues: Are there any related NuttX or NuttX Apps issues that this PR addresses? If so, provide links.
  • Incomplete testing section:
    • Build Host(s): Specify the operating system, CPU architecture, and compiler used for building and testing.
    • Target(s): "sim/smp" is too broad. Mention the specific simulator and its configuration used.
    • Testing logs: You must include actual testing logs (both before and after the change) to demonstrate the issue and the fix.

How to improve the PR:

  1. Elaborate on the deadlock: Describe the specific sequence of events and conditions that lead to the deadlock when using spin_lock.
  2. Explain the solution's mechanism: Briefly explain why critical_section is a safer choice in this context and how it prevents the deadlock.
  3. Add specifics to the "Impact" section:
    • Is new feature added? Is existing feature changed? - This is a bug fix, so clarify if any existing functionality is modified as a side effect.
    • Impact on other aspects: Address the remaining "Impact" questions (build process, hardware, documentation, security, compatibility). Even if the answer is "NO", state it explicitly.
  4. Provide detailed testing information: Include the build host details, specific simulator configuration, and, most importantly, the actual testing logs from before and after your changes.

@xiaoxiang781216
Copy link
Contributor

should we revert #14190 instead?

holding the irqsaved spinlock first and then take critical section may cause deadlock in SMP case
replace spin_lock to critical_section avoid deadlock

Signed-off-by: chao an <anchao@lixiang.com>
@github-actions github-actions bot added the Arch: arm Issues related to ARM (32-bit) architecture label Oct 13, 2024
@anchao anchao closed this Oct 14, 2024
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 Area: OS Components OS Components 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.

3 participants