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

Because sched_lock is now called in spin_lock_irqsave, there is no ne… #15665

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

wangzhi-art
Copy link
Contributor

@wangzhi-art wangzhi-art commented Jan 23, 2025

Summary

Because sched_lock is now called in spin_lock_irqsave, there is no need to call it explicitly.
please see #14578 for more info.

Impact

All places that call the spin_lock_irqsave interface and also call sched_lock in arch directory.

Testing

CI

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: mips Issues related to the MIPS architecture Arch: renesas Issues related to the Renesas chips Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Jan 23, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 23, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it attempts to address some points, it lacks critical details. Here's a breakdown:

  • Summary: While it states why (redundant call), it's vague on what and how. Which function(s) were modified? Exactly how was the code changed (e.g., "removed the call to sched_lock within the following functions: ...")? It also lacks issue/PR references.
  • Impact: The impact section is poorly formatted and lacks the required YES/NO responses. It mentions impacted code, but not the effects of the change. Will user applications need to change? Build systems? Documentation? Security considerations are completely omitted.
  • Testing: CI passing is good, but insufficient. The template specifically requests logs before and after the change on a local setup, including details about that setup (host OS, target architecture, etc.). Simply saying "CI" doesn't provide enough evidence for proper review. What specific tests were run? What were the observed results demonstrating the fix?

To make this PR acceptable, it needs significant improvement in providing specific details for each section, especially regarding the impact and testing sections. Example improvements:

Summary:

This PR removes redundant calls to sched_lock within the spin_lock_irqsave function in the architecture-specific code. Since spin_lock_irqsave now implicitly calls sched_lock, explicit calls are unnecessary and potentially lead to unexpected behavior. This addresses [NuttX Issue # (if applicable)]. The change removes the explicit sched_lock call from the following functions: function1(), function2(), etc. (within arch/xxxxx/src/...).

Impact:

  • Is new feature added? NO
  • Is existing feature changed? YES - Removed redundant calls that could potentially cause issues.
  • Impact on user (will user need to adapt to change)? NO
  • Impact on build (will build process change)? NO
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES - This change affects the arch/xxxx directory, specifically the handling of spinlocks. (List affected architectures)
  • Impact on documentation (is update required / provided)? YES - The documentation for spin_lock_irqsave should be updated to reflect that it now implicitly calls sched_lock. (Indicate if the documentation update is included in this PR)
  • Impact on security (any sort of implications)? NO (or YES if there are any, explain)
  • Impact on compatibility (backward/forward/interoperability)? NO (or YES if there are any, explain)
  • Anything else to consider? NO

Testing:

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:qemu-x86_64, ARM:stm32f4discovery

Testing logs before change:

(Show output demonstrating the redundant calls, if possible. At minimum, show the test commands used)

Testing logs after change:

(Show output demonstrating the issue is resolved, e.g., absence of redundant calls or improved performance. At minimum, show the test commands used)

@xiaoxiang781216 xiaoxiang781216 merged commit b0af946 into apache:master Jan 23, 2025
39 checks passed
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: arm64 Issues related to ARM64 (64-bit) architecture Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: mips Issues related to the MIPS architecture Arch: renesas Issues related to the Renesas chips Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: x86_64 Issues related to the x86_64 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.

3 participants