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

replace spin_lock_irqrestore with raw_spin_lock_irqrestore #15695

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hujun260
Copy link
Contributor

Summary

replace spin_lock_irqrestore with raw_spin_lock_irqrestore

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

reason:
raw_spin_lock_irqrestore has better performance.

Signed-off-by: hujun5 <[email protected]>
reason:
raw_spin_lock_irqrestore has better performance.

Signed-off-by: hujun5 <[email protected]>
reason:
raw_spin_lock_irqrestore has better performance.

Signed-off-by: hujun5 <[email protected]>
@github-actions github-actions bot added Area: File System File System issues Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Jan 26, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 26, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as presented. The Impact and Testing sections are incomplete placeholders. While the Summary section identifies what changed, it lacks crucial context:

  • WHY: Why is this change necessary? What problem does it solve? Is spin_lock_irqrestore buggy? Is this a performance optimization? The justification is missing.
  • HOW: A more detailed explanation of how the change works is needed. What are the implications of switching to raw_spin_lock_irqrestore? What are the differences between the two functions? Does this change affect interrupt handling?

To make this PR compliant, the following needs to be addressed:

  • Summary: Expand on the why and how of the change. Link a relevant NuttX issue if this addresses a reported bug or implements a requested feature.
  • Impact: Fill out all relevant sections. Consider impacts on interrupt latency, real-time performance, and potential race conditions. Even if the answer is "NO" for most, explicitly stating so is helpful.
  • Testing: Provide specific details about the testing environment (Host OS, compiler version, target architecture, board, configuration). Include actual logs demonstrating the behavior before and after the change. Simply stating that it "works as intended" is insufficient. Show evidence. What tests were run? Were there any performance benchmarks?

Example of an improved Summary:

The current implementation using spin_lock_irqrestore can lead to excessive interrupt latency in certain scenarios [link to NuttX issue if applicable]. This change replaces it with raw_spin_lock_irqrestore, which directly manipulates the interrupt state without additional overhead. This is expected to reduce interrupt latency and improve real-time performance. The difference between the two functions is that spin_lock_irqrestore [explain the difference, e.g., handles nesting, etc.], whereas raw_spin_lock_irqrestore [explain its behavior].

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

@anchao
Copy link
Contributor

anchao commented Jan 26, 2025

Please do not make such changes. The meaning of raw_ in linux is to disable the preemption. We need to discuss API naming further.

#15684 (comment)

@linguini1
Copy link
Contributor

Could you update the impact and testing sections? What is the purpose of the change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues Area: Memory Management Memory Management issues 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.

5 participants