Skip to content

Conversation

@hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 7, 2025

Summary

sched: replace all spin_lock_irqsave to spin_lock_irqsave_wo_note in sched dir
reason:
avoid potential deadlock situations

Impact

sched

Testing

ci

…sched dir

reason:
avoid potential deadlock situations

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@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 Jan 7, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 7, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the what, it lacks sufficient detail on the why and how. The impact section is insufficiently detailed, and the testing section is far too brief to be useful.

Here's a breakdown of the missing information:

  • Summary:

    • Why is this change necessary? "Avoid potential deadlock situations" is vague. Which deadlock situations? How frequently do they occur? What are the consequences of these deadlocks? Provide concrete examples or link to relevant issues.
    • How does the change work? Simply stating "replace all spin_lock_irqsave to spin_lock_irqsave_wo_note" doesn't explain the mechanism. What is the difference between these two functions? Why is spin_lock_irqsave_wo_note preferable in this context?
  • Impact: "sched" is not an acceptable answer. Address all the impact points individually with "YES" or "NO" and provide details for any "YES" answers. For example:

    • Impact on user: Likely NO, but explain why the user shouldn't notice a difference.
    • Impact on build: Likely NO, but confirm.
    • Impact on hardware: Specify the affected architectures. Even if it's all architectures, state that explicitly.
    • Impact on documentation: Does this change require documentation updates?
    • Impact on security: Does this change have any security implications (positive or negative)?
    • Impact on compatibility: Are there any backward compatibility concerns?
  • Testing: "ci" is unacceptable. Provide specific details about the testing environment:

    • Build Host(s): List the operating system, CPU architecture, and compiler version used for the build.
    • Target(s): List the target architecture(s) and board configurations tested.
    • Testing Logs: Include actual logs demonstrating the issue before the change and the improved behavior after the change. Simply stating that it "works as intended" is not sufficient proof. Show evidence of the deadlock being avoided.

In short, the PR needs significantly more detail to be acceptable. It needs to clearly explain the problem being solved, the chosen solution, the rationale behind the solution, and provide concrete evidence that the solution works as intended.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 11, 2025

but only the part which interact with sched_note need swtich to _wo_note version, using _wo_note blinkly will lose the insight into these components. @hujun260

@hujun260
Copy link
Contributor Author

but only the part which interact with sched_note need swtich to _wo_note version, using _wo_note blinkly will lose the insight into these components. @hujun260

ok

@hujun260 hujun260 closed this Jan 12, 2025
@hujun260 hujun260 deleted the apache_58 branch February 5, 2025 02:58
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