use small lock in following file #15303

hujun260 · 2024-12-20T13:51:09Z

Summary

use small lock in following file

arch/or1k/src/mor1kx/mor1kx_serial.c
arch/risc-v/src/bl602/bl602_serial.c
arch/risc-v/src/common/espressif/esp_lowputc.c
arch/risc-v/src/common/espressif/esp_lowputc.h
arch/risc-v/src/common/espressif/esp_tickless.c
arch/risc-v/src/esp32c3-legacy/esp32c3_idle.c
arch/risc-v/src/esp32c3-legacy/esp32c3_lowputc.c
arch/risc-v/src/esp32c3-legacy/esp32c3_lowputc.h
arch/risc-v/src/esp32c3-legacy/esp32c3_rtc_lowerhalf.c arch/risc-v/src/fe310/fe310_gpio.c

Impact

arch/or1k/src/mor1kx/mor1kx_serial.c
arch/risc-v/src/bl602/bl602_serial.c
arch/risc-v/src/common/espressif/esp_lowputc.c
arch/risc-v/src/common/espressif/esp_lowputc.h
arch/risc-v/src/common/espressif/esp_tickless.c
arch/risc-v/src/esp32c3-legacy/esp32c3_idle.c
arch/risc-v/src/esp32c3-legacy/esp32c3_lowputc.c
arch/risc-v/src/esp32c3-legacy/esp32c3_lowputc.h
arch/risc-v/src/esp32c3-legacy/esp32c3_rtc_lowerhalf.c arch/risc-v/src/fe310/fe310_gpio.c

Testing

ci

nuttxpr · 2024-12-20T14:04:45Z

[Experimental Bot, please feedback here]

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

Summary Deficiencies:

Missing "Why": It states what was changed (using smaller locks) but not why. Was this for performance? Deadlock avoidance? Reduced memory usage? This is critical information.
Missing "How exactly": Saying "use small lock" is vague. What kind of lock was used before? What kind of lock is used now? Were spinlocks replaced with mutexes? Was a custom lock implementation introduced? Provide specifics.
Missing Related Issues: If this addresses a reported bug or implements a requested feature, link the relevant issue(s).

Impact Deficiencies:

Repeating file list is unhelpful: Listing the affected files again doesn't explain the impact of the changes. Address each point individually:
- New Feature? Existing Feature Changed?: Specify which. Likely an existing feature changed (serial driver, tickless operation, GPIO).
- Impact on User: Will applications behave differently? Likely no, but state that explicitly.
- Impact on Build: Are there new configuration options? Probably not, but confirm.
- Impact on Hardware: Does this affect specific architectures, boards, or drivers? Yes, specifically the listed architectures. Mention them clearly.
- Impact on Documentation: Does the documentation need updating to reflect these changes? If not, say so explicitly. If yes, has the documentation been updated in this PR?
- Impact on Security: Are there any potential security implications from changing the locking mechanisms? (e.g., potential for race conditions if not done correctly). Carefully consider this.
- Impact on Compatibility: Does this break backward compatibility? Does it affect compatibility between different NuttX configurations?
"Anything else to consider?": This is for unusual edge cases. If none, you can omit this or state "None."

Testing Deficiencies:

"ci" is insufficient: Just stating "ci" doesn't demonstrate testing. Provide:
- Detailed build host information: OS version, compiler version (including minor version), etc.
- Detailed target information: Specific architectures and boards tested (e.g., sim:nsh, riscv:bl602-iot-sdk, etc.).
- Relevant log snippets: Don't just dump entire logs. Show specific before-and-after logs that demonstrate the issue being fixed or the new functionality working. Focus on the relevant parts. What was the problem before? How is the behavior different now?

Example of an Improved Summary:

This PR replaces mutexes with spinlocks in the serial driver, tickless timer, and GPIO driver for the OR1K, BL602, FE310, and ESP32C3 architectures. This change aims to improve performance by reducing the overhead of mutex acquisition and release in these frequently accessed code paths. This addresses the performance regression reported in [Issue #XYZ].

By providing specific details, you make it much easier for reviewers to understand and evaluate your changes, which leads to faster reviews and a higher likelihood of your PR being merged.

acassis · 2024-12-20T14:45:09Z

@hujun260 please verify this error:

Configuration/Tool: esp32c3-devkit/ble
2024-12-20 14:23:20
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
chip/esp32c3_lowputc.c: In function 'esp32c3_lowputc_disable_all_uart_int':
Error: chip/esp32c3_lowputc.c:718:29: error: passing argument 1 of 'spin_lock_irqsave' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  718 |   flags = spin_lock_irqsave(&priv->lock);
      |                             ^~~~~~~~~~~
In file included from chip/esp32c3_lowputc.c:30:
/github/workspace/sources/nuttx/include/nuttx/spinlock.h:511:55: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
  511 | irqstate_t spin_lock_irqsave(FAR volatile spinlock_t *lock)
      |                                  ~~~~~~~~~~~~~~~~~~~~~^~~~
Error: chip/esp32c3_lowputc.c:735:26: error: passing argument 1 of 'spin_unlock_irqrestore' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  735 |   spin_unlock_irqrestore(&priv->lock, flags);
      |                          ^~~~~~~~~~~
/github/workspace/sources/nuttx/include/nuttx/spinlock.h:674:54: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
  674 | void spin_unlock_irqrestore(FAR volatile spinlock_t *lock,
      |                                 ~~~~~~~~~~~~~~~~~~~~~^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:144: esp32c3_lowputc.o] Error 1
make[1]: Target 'libarch.a' not remade because of errors.
make: *** [tools/LibTargets.mk:170: arch/risc-v/src/libarch.a] Error 2
make: Target 'all' not remade because of errors.

acassis · 2024-12-21T14:19:09Z

@hujun260 your recent modifications broken the CI (stm32f4discovery), see LVGL PR from @JorgeGzm

I think RISC-V also was affected:

Configuration/Tool: esp32c3-generic/tickless
2024-12-20 13:56:24
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
common/espressif/esp_tickless.c: In function 'up_timer_start':
Error: common/espressif/esp_tickless.c:359:23: error: passing argument 1 of 'up_timer_cancel' from incompatible pointer type [-Werror=incompatible-pointer-types]
  359 |       up_timer_cancel(&g_esp_tickless_lock);
      |                       ^~~~~~~~~~~~~~~~~~~~
      |                       |
      |                       spinlock_t * {aka unsigned char *}
common/espressif/esp_tickless.c:268:48: note: expected 'struct timespec *' but argument is of type 'spinlock_t *' {aka 'unsigned char *'}
  268 | int IRAM_ATTR up_timer_cancel(struct timespec *ts)
      |                               ~~~~~~~~~~~~~~~~~^~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:144: esp_tickless.o] Error 1
make[1]: Target 'libarch.a' not remade because of errors.
make: *** [tools/LibTargets.mk:170: arch/risc-v/src/libarch.a] Error 2
make: Target 'all' not remade because of errors.
/github/workspace/sources/nuttx/tools/testbuild.sh: line 385: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
  Normalize esp32c3-generic/tickless

hujun260 · 2024-12-22T02:04:11Z

@hujun260 please verify this error:

Configuration/Tool: esp32c3-devkit/ble
2024-12-20 14:23:20
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
chip/esp32c3_lowputc.c: In function 'esp32c3_lowputc_disable_all_uart_int':
Error: chip/esp32c3_lowputc.c:718:29: error: passing argument 1 of 'spin_lock_irqsave' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  718 |   flags = spin_lock_irqsave(&priv->lock);
      |                             ^~~~~~~~~~~
In file included from chip/esp32c3_lowputc.c:30:
/github/workspace/sources/nuttx/include/nuttx/spinlock.h:511:55: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
  511 | irqstate_t spin_lock_irqsave(FAR volatile spinlock_t *lock)
      |                                  ~~~~~~~~~~~~~~~~~~~~~^~~~
Error: chip/esp32c3_lowputc.c:735:26: error: passing argument 1 of 'spin_unlock_irqrestore' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  735 |   spin_unlock_irqrestore(&priv->lock, flags);
      |                          ^~~~~~~~~~~
/github/workspace/sources/nuttx/include/nuttx/spinlock.h:674:54: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
  674 | void spin_unlock_irqrestore(FAR volatile spinlock_t *lock,
      |                                 ~~~~~~~~~~~~~~~~~~~~~^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:144: esp32c3_lowputc.o] Error 1
make[1]: Target 'libarch.a' not remade because of errors.
make: *** [tools/LibTargets.mk:170: arch/risc-v/src/libarch.a] Error 2
make: Target 'all' not remade because of errors.

done

arch/or1k/src/mor1kx/mor1kx_serial.c arch/risc-v/src/bl602/bl602_serial.c arch/risc-v/src/common/espressif/esp_lowputc.c arch/risc-v/src/common/espressif/esp_lowputc.h arch/risc-v/src/common/espressif/esp_tickless.c arch/risc-v/src/esp32c3-legacy/esp32c3_idle.c arch/risc-v/src/esp32c3-legacy/esp32c3_lowputc.c arch/risc-v/src/esp32c3-legacy/esp32c3_lowputc.h arch/risc-v/src/esp32c3-legacy/esp32c3_rtc_lowerhalf.c arch/risc-v/src/fe310/fe310_gpio.c Signed-off-by: hujun5 <[email protected]>

fdcavalcanti · 2024-12-26T13:12:42Z

@hujun260 I'm having similar issue on esp32c3-generic and esp32h2-devkit on this commit.
Could you please take a look?

./tools/configure.sh esp32c3-generic:nsh or ./tools/configure.sh esp32h2-devkit:nsh
Set CONFIG_SPINLOCK
Make

common/espressif/esp_lowputc.c: In function 'esp_lowputc_disable_all_uart_int':
common/espressif/esp_lowputc.c:215:29: error: passing argument 1 of 'spin_lock_irqsave' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  215 |   flags = spin_lock_irqsave(&priv->lock);
      |                             ^~~~~~~~~~~
In file included from common/espressif/esp_lowputc.c:40:
/home/fdcavalcanti/nuttxspace1/nuttx/include/nuttx/spinlock.h:511:55: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
  511 | irqstate_t spin_lock_irqsave(FAR volatile spinlock_t *lock)
      |                                  ~~~~~~~~~~~~~~~~~~~~~^~~~
common/espressif/esp_lowputc.c:232:26: error: passing argument 1 of 'spin_unlock_irqrestore' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
  232 |   spin_unlock_irqrestore(&priv->lock, flags);
      |                          ^~~~~~~~~~~
/home/fdcavalcanti/nuttxspace1/nuttx/include/nuttx/spinlock.h:674:54: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
  674 | void spin_unlock_irqrestore(FAR volatile spinlock_t *lock,
      |                                 ~~~~~~~~~~~~~~~~~~~~~^~~~
IN: boards/libboards.a -> staging/libboards.a cc1: all warnings being treated as errors
make[1]: *** [Makefile:144: esp_lowputc.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC:  misc/reboot_notifier.c make: *** [tools/LibTargets.mk:170: arch/risc-v/src/libarch.a] Error 2
make: *** Waiting for unfinished jobs....

hujun260 · 2024-12-26T13:30:30Z

common/espressif/esp_lowputc.c: In function 'esp_lowputc_disable_all_uart_int':
common/espressif/esp_lowputc.c:215:29: error: passing argument 1 of 'spin_lock_irqsave' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
215 | flags = spin_lock_irqsave(&priv->lock);
| ^~~~~~~~~~~
In file included from common/espressif/esp_lowputc.c:40:
/home/fdcavalcanti/nuttxspace1/nuttx/include/nuttx/spinlock.h:511:55: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
511 | irqstate_t spin_lock_irqsave(FAR volatile spinlock_t *lock)
| ~~~~~~~~~~~~~~~~~~~~~^~~~
common/espressif/esp_lowputc.c:232:26: error: passing argument 1 of 'spin_unlock_irqrestore' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
232 | spin_unlock_irqrestore(&priv->lock, flags);
| ^~~~~~~~~~~
/home/fdcavalcanti/nuttxspace1/nuttx/include/nuttx/spinlock.h:674:54: note: expected 'volatile spinlock_t *' {aka 'volatile unsigned int *'} but argument is of type 'const spinlock_t *' {aka 'const unsigned int *'}
674 | void spin_unlock_irqrestore(FAR volatile spinlock_t *lock,
| ~~~~~~~~~~~~~~~~~~~~~^~~~
IN: boards/libboards.a -> staging/libboards.a cc1: all warnings being treated as errors
make[1]: *** [Makefile:144: esp_lowputc.o] Error 1
make[1]: *** Waiting for unfinished jobs....
CC: misc/reboot_notifier.c make: *** [tools/LibTargets.mk:170: arch/risc-v/src/libarch.a] Error 2
make: *** Waiting for unfinished jobs....

#15350

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

acassis approved these changes Dec 20, 2024

View reviewed changes

hujun260 force-pushed the apache_45 branch from a0763b8 to 8fb6829 Compare December 22, 2024 02:03

hujun260 force-pushed the apache_45 branch from 8fb6829 to 71a15bc Compare December 22, 2024 03:33

acassis approved these changes Dec 22, 2024

View reviewed changes

xiaoxiang781216 approved these changes Dec 22, 2024

View reviewed changes

xiaoxiang781216 merged commit 97eef35 into apache:master Dec 22, 2024
27 checks passed

jerpelea mentioned this pull request Dec 27, 2024

{bp-15318} use small lock in following files: #15355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use small lock in following file #15303

use small lock in following file #15303

hujun260 commented Dec 20, 2024

nuttxpr commented Dec 20, 2024

acassis commented Dec 20, 2024

acassis commented Dec 21, 2024

hujun260 commented Dec 22, 2024

fdcavalcanti commented Dec 26, 2024 •

edited

Loading

hujun260 commented Dec 26, 2024

use small lock in following file #15303

use small lock in following file #15303

Conversation

hujun260 commented Dec 20, 2024

Summary

Impact

Testing

nuttxpr commented Dec 20, 2024

acassis commented Dec 20, 2024

acassis commented Dec 21, 2024

hujun260 commented Dec 22, 2024

fdcavalcanti commented Dec 26, 2024 • edited Loading

hujun260 commented Dec 26, 2024

fdcavalcanti commented Dec 26, 2024 •

edited

Loading