Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

use small lock in following files

arch/risc-v/src/litex/litex_emac.c
arch/risc-v/src/mpfs/mpfs_coremmc.c
arch/risc-v/src/mpfs/mpfs_usb.c
arch/xtensa/src/esp32/esp32_idle.c
arch/xtensa/src/esp32/esp32_rtc_lowerhalf.c
arch/xtensa/src/esp32s2/esp32s2_idle.c
arch/xtensa/src/esp32s2/esp32s2_lowputc.c
arch/xtensa/src/esp32s2/esp32s2_lowputc.h
arch/xtensa/src/esp32s2/esp32s2_rtc_lowerhalf.c
arch/xtensa/src/esp32s3/esp32s3_idle.c
arch/xtensa/src/esp32s3/esp32s3_rtc_lowerhalf.c

Impact

arch/risc-v/src/litex/litex_emac.c
arch/risc-v/src/mpfs/mpfs_coremmc.c
arch/risc-v/src/mpfs/mpfs_usb.c
arch/xtensa/src/esp32/esp32_idle.c
arch/xtensa/src/esp32/esp32_rtc_lowerhalf.c
arch/xtensa/src/esp32s2/esp32s2_idle.c
arch/xtensa/src/esp32s2/esp32s2_lowputc.c
arch/xtensa/src/esp32s2/esp32s2_lowputc.h
arch/xtensa/src/esp32s2/esp32s2_rtc_lowerhalf.c
arch/xtensa/src/esp32s3/esp32s3_idle.c
arch/xtensa/src/esp32s3/esp32s3_rtc_lowerhalf.c

Testing

ci

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Dec 20, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 20, 2024

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is the change necessary? Simply stating "use small lock" is insufficient. Explain the problem the larger lock was causing (e.g., performance bottleneck, deadlock potential, excessive memory usage). What benefits does using a smaller lock provide?
  • What functional part of the code is being changed? Be more specific than just listing files. Mention the specific functions or code sections within those files where the locks are being changed. For example, "Interrupt handlers in the LiteX EMAC driver," or "Data transfer routines in the MPFS CoreMMC driver."
  • How does the change exactly work? What type of lock was used before? What type is being used now? Was any other code modified besides just the lock type (e.g., critical sections, interrupt masking)?
  • Related Issues: Link any related NuttX issues. Even if there isn't a directly related issue, creating one before the PR and linking it is good practice.

Missing Information in Impact:

  • Is new feature added? Is existing feature changed? This is likely an optimization, so clarify that. State that it's a change to existing functionality.
  • All other Impact sections: All are marked as changed, but none provide descriptions. This is crucial. For example:
    • Impact on user: "Potentially improved performance in network/storage/peripheral operations on affected platforms." (If no noticeable user impact, state that explicitly).
    • Impact on build: Likely "NO," but confirm.
    • Impact on hardware: Specify the affected architectures (RISC-V, Xtensa) and boards (LiteX, MPFS, ESP32, ESP32S2, ESP32S3).
    • Impact on documentation: If no changes are needed, explicitly state "NO." If there are changes, describe them or say "YES, documentation updated."
    • Impact on security: Carefully consider this. Changing locking mechanisms can sometimes introduce vulnerabilities. If none, explicitly state "NO." If there are concerns, describe them and how they've been mitigated.
    • Impact on compatibility: Likely "NO," but confirm.
  • Anything else to consider: This is a good place to mention any potential caveats or limitations of the change.

Missing Information in Testing:

  • Build Host(s): Provide details about your build environment.
  • Target(s): Be specific about the target platforms tested. For example: sim:qemu-rv, board:esp32-devkitc.
  • Testing logs before change: These are essential to demonstrate the problem being solved. Show the performance bottleneck, the deadlock, or whatever issue the larger lock was causing.
  • Testing logs after change: Show how the change improves the situation. Provide performance metrics, demonstrate the absence of deadlocks, etc. "ci" is not sufficient. Link to CI logs if relevant, but still include representative snippets in the PR description itself.

Example of an improved Summary:

This PR optimizes locking mechanisms in several drivers for RISC-V and Xtensa platforms to improve performance. The existing use of larger locks caused contention and limited throughput, particularly noticeable in network and storage operations. This change replaces the larger locks with smaller, more fine-grained locks in specific code sections within the LiteX EMAC, MPFS CoreMMC, MPFS USB, and ESP32/ESP32S2/ESP32S3 idle and RTC drivers. This change addresses NuttX Issue #XXXX.

By providing this level of detail, reviewers can understand the context of your changes, assess their impact, and verify their correctness. This leads to faster reviews and higher-quality contributions to the NuttX project.

@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Size: M The size of the change in this PR is medium labels Dec 20, 2024
arch/risc-v/src/litex/litex_emac.c
arch/risc-v/src/mpfs/mpfs_coremmc.c
arch/risc-v/src/mpfs/mpfs_usb.c
arch/xtensa/src/esp32/esp32_idle.c
arch/xtensa/src/esp32/esp32_rtc_lowerhalf.c
arch/xtensa/src/esp32s2/esp32s2_idle.c
arch/xtensa/src/esp32s2/esp32s2_lowputc.c
arch/xtensa/src/esp32s2/esp32s2_lowputc.h
arch/xtensa/src/esp32s2/esp32s2_rtc_lowerhalf.c
arch/xtensa/src/esp32s3/esp32s3_idle.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit f25e3d2 into apache:master Dec 22, 2024
27 checks passed
@lupyuen
Copy link
Member

lupyuen commented Dec 23, 2024

Hi @hujun260: esp32s2-kaluga-1/audio is failing to build after this PR. Could you check? Thanks!
https://github.com/NuttX/nuttx/actions/runs/12453859478/job/34764458075#step:7:221

Configuration/Tool: esp32s2-kaluga-1/audio
Error: chip/esp32s2_lowputc.c:102:22: error: expected '}' before ';' token
  102 |   .lock = SP_UNLOCKED;
      |                      ^
chip/esp32s2_lowputc.c:62:1: note: to match this '{'
   62 | {

More errors at nuttx-dashboard.org
Screenshot 2024-12-23 at 10 42 51 AM

@hujun260
Copy link
Contributor Author

Configuration/Tool: esp32s2-kaluga-1/audio
Error: chip/esp32s2_lowputc.c:102:22: error: expected '}' before ';' token
102 | .lock = SP_UNLOCKED;
| ^
chip/esp32s2_lowputc.c:62:1: note: to match this '{'
62 | {

#15313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture 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.

4 participants