Skip to content

semaphore: change semaphore count to atomic operation#15075

Merged
xiaoxiang781216 merged 4 commits intoapache:masterfrom
zyfeier:sem_atomic_3
Dec 10, 2024
Merged

semaphore: change semaphore count to atomic operation#15075
xiaoxiang781216 merged 4 commits intoapache:masterfrom
zyfeier:sem_atomic_3

Conversation

@zyfeier
Copy link
Copy Markdown
Contributor

@zyfeier zyfeier commented Dec 6, 2024

Summary

Add sem_wait fast operations, use atomic to ensure atomicity of semcount operations, and do not depend on critical section.

Test with robot board:
before modify:
nxmutex_lock cost: 78 ns
nxmutex_unlock cost: 82 ns

after modify:
nxmutex_lock cost: 28 ns
nxmutex_unlock cost: 14 ns

Impact

semaphore

Testing

bes board with monkey test pass
sabre-6quad:smp with ostest pass
sim:citest pass

@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 Dec 6, 2024
@nuttxpr
Copy link
Copy Markdown

nuttxpr commented Dec 6, 2024

[Experimental Bot, please feedback here]

No. The PR description lacks several key details required by the NuttX guidelines.

Here's a breakdown:

  • Insufficient Summary: While the summary mentions what changed, it lacks crucial why information. Why was this change necessary? What problem does it solve? What are the benefits of using atomic operations and avoiding the critical section?
  • Missing Impact Details: Simply stating "semaphore" is insufficient. The impact section needs specific YES/NO answers for all listed questions. For any "YES" answers, a detailed explanation is required. For example, even if the user impact is NO, explicitly state "Impact on user: NO." Consider these points for your impact assessment:
    • Performance: The summary mentions performance improvements. This should be explicitly stated in the Impact section, quantifying the improvement.
    • Behavior Change: Does this change the behavior of semaphores in any way beyond performance? Even subtle changes can affect existing applications.
    • Portability: Does this change affect portability to other architectures? Does it rely on specific atomic operations that might not be available everywhere?
  • Incomplete Testing Information: While testing environments are mentioned, it lacks details required by the template. The provided information is too vague.
    • Build Host: Specify OS version, compiler version (e.g., GCC 12.2), etc.
    • Target: Be specific. Instead of "sim," specify the simulator being used (e.g., qemu-system-arm). For boards, specify the configuration used (e.g., sabre-6quad:smp).
  • Missing "Before" and "After" Logs: The provided timing information is helpful, but insufficient. The template specifically requests logs, not just summarized timings. Show relevant log excerpts demonstrating the semaphore behavior before and after the change. Include more than just the timings - show the context of the semaphore usage in the test.
  • Missing Issue References: Are there any related NuttX or NuttX Apps issues that prompted this change? If so, include links.

In short: The PR needs to be significantly expanded to address these missing details before it can be considered complete according to the NuttX requirements. Focus on providing specific, detailed answers to all questions in the template.

@zyfeier zyfeier force-pushed the sem_atomic_3 branch 2 times, most recently from 22e0542 to 4b89636 Compare December 9, 2024 14:21
@xiaoxiang781216 xiaoxiang781216 marked this pull request as ready for review December 9, 2024 17:29
xiaoxiang781216
xiaoxiang781216 reviewed Dec 9, 2024
View reviewed changes
Comment thread sched/semaphore/sem_wait.c
Comment thread sched/semaphore/sem_trywait.c
Comment thread sched/semaphore/sem_wait.c
Comment thread sched/semaphore/sem_trywait.c
Comment thread sched/semaphore/sem_trywait.c Outdated
Comment thread sched/semaphore/sem_reset.c Outdated
Comment thread sched/semaphore/sem_post.c
Comment thread sched/semaphore/sem_post.c Outdated
Comment thread sched/semaphore/sem_destroy.c
Comment thread sched/semaphore/sem_destroy.c Outdated
@zyfeier zyfeier force-pushed the sem_atomic_3 branch 3 times, most recently from bd9ba91 to fd34884 Compare December 10, 2024 05:37
xiaoxiang781216
xiaoxiang781216 reviewed Dec 10, 2024
View reviewed changes
Comment thread sched/semaphore/sem_trywait.c Outdated
@zyfeier
atomic: Unify the compare_exchange functions of nx and stdatomic
020913a 
define nx_atomic_compare_exchange_weak and nx_atomic_compare_exchange_strong function

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
@zyfeier
sem: change sem wait to atomic operation
6813737 
Add sem_wait fast operations, use atomic to ensure
atomicity of semcount operations, and do not depend
on critical section.

Test with robot:
before modify:
nxmutex_lock cost: 78 ns
nxmutex_unlock cost: 82 ns

after modify:
nxmutex_lock cost: 28 ns
nxmutex_unlock cost: 14 ns

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
@zyfeier
arm/lpc17xx: disable mqueue sysv
fc91b5a 
mqueue sysv not used, remove to reduce sram usage

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
@zyfeier
tools/ci: change sem_init_6_1 to support skip result
2887e90 
After SEM_VALUE_MAX change to INT_MAX, need to support
skip result.

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
xiaoxiang781216
xiaoxiang781216 reviewed Dec 10, 2024
View reviewed changes
Comment thread tools/ci/testrun/script/test_open_posix/test_openposix_.py
@xiaoxiang781216 xiaoxiang781216 merged commit fb4a246 into apache:master Dec 10, 2024
@fdcavalcanti fdcavalcanti mentioned this pull request Dec 11, 2024
Closed
1 task
tmedicci added a commit to tmedicci/nuttx that referenced this pull request Dec 11, 2024
@tmedicci
xtensa/esp32s3: Update the reserved size for struct __lock
74d060c 
After apache#15075, the static
assertion at `nuttx/arch/xtensa/src/esp32s3/esp32s3_libc_stubs.c`
was being triggered when building any of the ESP32-S3's defconfigs.
This commit updates the reserved size to reflect the changes
introduced by the related PR.
@tmedicci tmedicci mentioned this pull request Dec 11, 2024
Merged
xiaoxiang781216 pushed a commit that referenced this pull request Dec 11, 2024
@tmedicci @xiaoxiang781216
xtensa/esp32s3: Update the reserved size for struct __lock
f3ec1bd 
After #15075, the static
assertion at `nuttx/arch/xtensa/src/esp32s3/esp32s3_libc_stubs.c`
was being triggered when building any of the ESP32-S3's defconfigs.
This commit updates the reserved size to reflect the changes
introduced by the related PR.
jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Dec 12, 2024
@tmedicci @jerpelea
xtensa/esp32s3: Update the reserved size for struct __lock
c770c4c 
After apache#15075, the static
assertion at `nuttx/arch/xtensa/src/esp32s3/esp32s3_libc_stubs.c`
was being triggered when building any of the ESP32-S3's defconfigs.
This commit updates the reserved size to reflect the changes
introduced by the related PR.
@jerpelea jerpelea mentioned this pull request Dec 12, 2024
Merged
xiaoxiang781216 pushed a commit that referenced this pull request Dec 12, 2024
@tmedicci @xiaoxiang781216
xtensa/esp32s3: Update the reserved size for struct __lock
6eb9b09 
After #15075, the static
assertion at `nuttx/arch/xtensa/src/esp32s3/esp32s3_libc_stubs.c`
was being triggered when building any of the ESP32-S3's defconfigs.
This commit updates the reserved size to reflect the changes
introduced by the related PR.
@tmedicci tmedicci mentioned this pull request Dec 12, 2024
Closed
1 task
tmedicci added a commit to tmedicci/nuttx that referenced this pull request Dec 12, 2024
@tmedicci
rv-virt/citest: Increase init task stack size to 3072
ecd20b9 
After apache#15075, the size of the
stack size has decreased 8 bytes and the init stack size for the
rv-virt:citest defconfig was near its full capacity, which lead to
crashing the `ps` command. The init stack size for this defconfig
was increased from 2048 to 3072 to avoid stack overflow.
@tmedicci tmedicci mentioned this pull request Dec 12, 2024
Merged
tmedicci added a commit to tmedicci/nuttx that referenced this pull request Dec 12, 2024
@tmedicci
rv-virt/citest: Increase init task stack size to 4096
cae8d20 
After apache#15075, the size of the
stack size has decreased 8 bytes and the init stack size for the
rv-virt:citest defconfig was near its full capacity, which lead to
crashing the `ps` command. The init stack size for this defconfig
was increased from 2048 to 4096 to avoid stack overflow.
xiaoxiang781216 pushed a commit that referenced this pull request Dec 12, 2024
@tmedicci @xiaoxiang781216
rv-virt/citest: Increase init task stack size to 3072
38b41e5 
After #15075, the size of the
stack size has decreased 8 bytes and the init stack size for the
rv-virt:citest defconfig was near its full capacity, which lead to
crashing the `ps` command. The init stack size for this defconfig
was increased from 2048 to 3072 to avoid stack overflow.
jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Dec 13, 2024
@tmedicci @jerpelea
rv-virt/citest: Increase init task stack size to 3072
f082cce 
After apache#15075, the size of the
stack size has decreased 8 bytes and the init stack size for the
rv-virt:citest defconfig was near its full capacity, which lead to
crashing the `ps` command. The init stack size for this defconfig
was increased from 2048 to 3072 to avoid stack overflow.
@jerpelea jerpelea mentioned this pull request Dec 13, 2024
Merged
xiaoxiang781216 pushed a commit that referenced this pull request Dec 13, 2024
@tmedicci @xiaoxiang781216
rv-virt/citest: Increase init task stack size to 3072
0da4e44 
After #15075, the size of the
stack size has decreased 8 bytes and the init stack size for the
rv-virt:citest defconfig was near its full capacity, which lead to
crashing the `ps` command. The init stack size for this defconfig
was increased from 2048 to 3072 to avoid stack overflow.
linguini1 pushed a commit to CarletonURocketry/nuttx that referenced this pull request Jan 15, 2025
@tmedicci @linguini1
xtensa/esp32s3: Update the reserved size for struct __lock
2c61f13 
After apache#15075, the static
assertion at `nuttx/arch/xtensa/src/esp32s3/esp32s3_libc_stubs.c`
was being triggered when building any of the ESP32-S3's defconfigs.
This commit updates the reserved size to reflect the changes
introduced by the related PR.
linguini1 pushed a commit to CarletonURocketry/nuttx that referenced this pull request Jan 15, 2025
@tmedicci @linguini1
rv-virt/citest: Increase init task stack size to 3072
00095ca 
After apache#15075, the size of the
stack size has decreased 8 bytes and the init stack size for the
rv-virt:citest defconfig was near its full capacity, which lead to
crashing the `ps` command. The init stack size for this defconfig
was increased from 2048 to 3072 to avoid stack overflow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

Assignees

No one assigned

Labels

Area: CI Area: OS Components OS Components issues Area: Tooling Board: arm Size: M The size of the change in this PR is medium

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zyfeier @nuttxpr @xiaoxiang781216