Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

spinlock: remove recursive locks with write_lock_irqsave/read_lock_irqsave
reason:
1 There is a similar PR, #14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

Impact

spinlock

Testing

ci

…qsave

reason:
1 There is a similar PR, apache#14079,
2 Currently, no one is using recursive locks with write_lock_irqsave/read_lock_irqsave.
3 Nested spinlock is harmful, prone to abuse and leading to a decline in code quality and performance
4 Nested spinlock is also not available in Linux.
5 In our future plans, nested usage of enter_critical_section and spin_lock_irqsave will also be removed.

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 Nov 25, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 25, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing with CI, it lacks crucial details.

Here's what's missing:

  • Summary: Lacks specifics on how the change works. Removing recursive locks is the what, but the how (code modifications) is absent. While referencing a similar PR is helpful, the summary should stand alone.
  • Impact: The single word "spinlock" is insufficient. It needs to address all the impact points with YES/NO and descriptions for any YES answers. Consider the ripple effects of this change. Will any drivers or subsystems need modification?
  • Testing: CI passing is not enough detail. Provide specific build hosts and targets tested. "Testing logs before change" and "Testing logs after change" sections are empty. Show what was tested and the results demonstrating the fix. Ideally, show a test case that failed before and passes after the change.

Without these details, reviewers cannot effectively assess the PR.

@xiaoxiang781216 xiaoxiang781216 merged commit f5136b2 into apache:master Nov 26, 2024
@hujun260 hujun260 deleted the apache_29 branch December 1, 2024 02:54
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