spin_lock_irqsave+sched_lock #14578

hujun260 · 2024-10-31T09:02:03Z

Summary

1 Accelerated the implementation of sched_lock, remove enter_critical_section in sched_lock and
only enter_critical_section when task scheduling is required.
2 we add sched_lock_wo_note/sched_unlock_wo_note and it does not perform instrumentation logic
3 We aim to replace big locks with smaller ones. So we will use spin_lock_irqsave extensively to
replace enter_critical_section in the subsequent process. We imitate the implementation of Linux
by adding sched_lock to spin_lock_irqsave in order to address scenarios where sem_post occurs
within spin_lock_irqsave, which can lead to spinlock failures and deadlocks.

The entire implementation process includes:

1 spin_lock_irqsave + sched_lock
2 spin_lock/rw/spin_trylock + sched_lock
3 enter_critical_section + sched_lock
We are currently implementing the first step.

Impact

spinlock and sched_lock

Testing

Build Host:

OS: Ubuntu 20.04
CPU: x86_64
Compiler: GCC 9.4.0

Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic
-machine virt,virtualization=on,gic-version=3
-net none -chardev stdio,id=con,mux=on -serial chardev:con
-mon chardev=con,mode=readline -kernel ./nuttx

arch/xtensa/src/esp32/esp32_ble_adapter.c

include/nuttx/spinlock.h

sched/sched/sched.h

include/sched.h

sched/sched/sched.h

sched/sched/sched_lock.c

sched/sched/sched_unlock.c

include/nuttx/spinlock.h

xiaoxiang781216 · 2024-11-18T07:25:38Z

@patacongo please review this patch which fix the long issue about sched lock.

sched/sched/sched_lock.c

xiaoxiang781216 · 2025-01-18T17:32:37Z

include/sys/syscall_lookup.h

@@ -41,10 +41,12 @@ SYSCALL_LOOKUP(sched_getparam,             2)
 SYSCALL_LOOKUP(sched_getscheduler,         1)
 SYSCALL_LOOKUP(sched_lock,                 0)
 SYSCALL_LOOKUP(sched_lockcount,            0)
+SYSCALL_LOOKUP(sched_lock_wo_note,         0)


but the implement of lib_get_tempbuffer switch to atomic operation.

sched/sched/sched_lock.c

sched/sched/sched_unlock.c

xiaoxiang781216 · 2025-01-18T17:46:37Z

sched/sched/sched_unlock.c

-  /* This operation is safe because the scheduler is locked and no context
-   * switch may occur.
-   */
+  if (list_pendingtasks()->head != NULL)


do we need hold critical section before check the pending task list?

I think we just need to add a memory synchronization here.
If list_pendingtasks()->head is modified to (not NULL) by another CPU,
but the current CPU doesn't detect it, it's okay. The other core will continue to check list_pendingtasks()->head when sched_unlock is called.
Additionally, we will perform a double-check in nxsched_merge_pending, which is within the critical section.

include/nuttx/spinlock.h

arch/risc-v/src/common/espressif/esp_pcnt.c

xiaoxiang781216 · 2025-01-18T18:05:30Z

sched/sched/sched_unlock.c

        {
-          /* Note that we no longer have pre-emption disabled. */
-
-#if CONFIG_SCHED_CRITMONITOR_MAXTIME_PREEMPTION >= 0


let's try best to reduce the change

sched/sched/sched_lock.c

include/nuttx/spinlock.h

include/nuttx/irq.h

xiaoxiang781216 · 2025-01-20T12:46:44Z

include/nuttx/irq.h

@@ -80,7 +80,7 @@
  do \


move to sched/irq/irq.h

done in next patch

xiaoxiang781216 · 2025-01-20T12:49:06Z

include/sys/syscall_lookup.h

 SYSCALL_LOOKUP(sched_rr_get_interval,      2)
 SYSCALL_LOOKUP(sched_setparam,             2)
 SYSCALL_LOOKUP(sched_setscheduler,         3)
 SYSCALL_LOOKUP(sched_unlock,               0)
+SYSCALL_LOOKUP(sched_unlock_wo_note,       0)


does usespace really call sched_unlock_wo_note

xiaoxiang781216 · 2025-01-20T12:49:29Z

sched/sched/sched_lock.c

 {
+#if (CONFIG_SCHED_CRITMONITOR_MAXTIME_PREEMPTION >= 0) ||\


#if (CONFIG_SCHED_CRITMONITOR_MAXTIME_PREEMPTION >= 0) || \

xiaoxiang781216 · 2025-01-20T12:50:27Z

sched/sched/sched_lock.c

-
-      if (rtcb->lockcount == 1)
+      rtcb = this_task();
+      if (rtcb != NULL)


why need check != NULL

xiaoxiang781216 · 2025-01-20T12:52:03Z

sched/sched/sched_lock.c

+  if (!up_interrupt_context())
+    {
+      rtcb = this_task();
+      if (rtcb != NULL)


don't need check?

xiaoxiang781216 · 2025-01-20T12:58:54Z

sched/sched/sched_lock.c

-#ifdef CONFIG_SCHED_INSTRUMENTATION_PREEMPTION
-          sched_note_preemption(rtcb, true);
+#else
+  sched_lock_wo_note();


remove, could share the same path

xiaoxiang781216 · 2025-01-20T12:59:54Z

sched/sched/sched_unlock.c

+  if (!up_interrupt_context())
+    {
+      rtcb = this_task();
+      if (rtcb != NULL)


remove the check

xiaoxiang781216 · 2025-01-20T13:00:19Z

sched/sched/sched_unlock.c

-
-      if (rtcb->lockcount <= 0)
+      tcb = this_task();
+      if (tcb != NULL)


remove the check

xiaoxiang781216 · 2025-01-20T13:00:59Z

sched/sched/sched_unlock.c

-              rtcb->timeslice == 0)
+          leave_critical_section_wo_note(flags);
+#else
+          tcb->lockcount--;


let's share the same code path

xiaoxiang781216 · 2025-01-20T13:01:27Z

sched/sched/sched_unlock.c

-          /* Note that we no longer have pre-emption disabled. */
+#if (CONFIG_SCHED_CRITMONITOR_MAXTIME_PREEMPTION >= 0) ||\
+    defined(CONFIG_SCHED_INSTRUMENTATION_PREEMPTION)
+          irqstate_t flags = enter_critical_section_wo_note();


move after line 240

… with the _wo_note suffix. Signed-off-by: hujun5 <hujun5@xiaomi.com>

Signed-off-by: hujun5 <hujun5@xiaomi.com>

…/irq.h Signed-off-by: hujun5 <hujun5@xiaomi.com>

…ed_[un]lock reason: 1 Accelerated the implementation of sched_lock, remove enter_critical_section in sched_lock and only enter_critical_section when task scheduling is required. 2 we add sched_lock_wo_note/sched_unlock_wo_note and it does not perform instrumentation logic Signed-off-by: hujun5 <hujun5@xiaomi.com>

reason: We aim to replace big locks with smaller ones. So we will use spin_lock_irqsave extensively to replace enter_critical_section in the subsequent process. We imitate the implementation of Linux by adding sched_lock to spin_lock_irqsave in order to address scenarios where sem_post occurs within spin_lock_irqsave, which can lead to spinlock failures and deadlocks. Signed-off-by: hujun5 <hujun5@xiaomi.com>

github-actions bot added Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Oct 31, 2024

xiaoxiang781216 requested a review from anchao October 31, 2024 09:14

hujun260 force-pushed the apache_3 branch from 61fdaf6 to e5d9392 Compare October 31, 2024 10:58

github-actions bot added the Arch: xtensa Issues related to the Xtensa architecture label Oct 31, 2024

hujun260 force-pushed the apache_3 branch from e5d9392 to 2d48282 Compare October 31, 2024 11:22

acassis reviewed Oct 31, 2024

View reviewed changes

arch/xtensa/src/esp32/esp32_ble_adapter.c Show resolved Hide resolved

anchao requested changes Nov 1, 2024

View reviewed changes

include/nuttx/spinlock.h Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_3 branch 7 times, most recently from 157736b to 175819e Compare November 4, 2024 11:26

hujun260 requested a review from xiaoxiang781216 November 6, 2024 11:32

hujun260 mentioned this pull request Nov 6, 2024

wqueue remove enter_critical_section #14623

Merged

xiaoxiang781216 requested a review from anchao November 15, 2024 06:26

xiaoxiang781216 reviewed Nov 18, 2024

View reviewed changes

xiaoxiang781216 linked an issue Nov 18, 2024 that may be closed by this pull request

Thread holding spinlock can be blocked. #9531

Open

xiaoxiang781216 requested a review from patacongo November 18, 2024 07:25

xiaoxiang781216 linked an issue Nov 18, 2024 that may be closed by this pull request

Thread can be unexpectedly suspended within a critical section. #1138

Open

anchao reviewed Nov 19, 2024

View reviewed changes

sched/sched/sched_lock.c Outdated Show resolved Hide resolved

sched/sched/sched_lock.c Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_3 branch from 175819e to 815cf96 Compare November 19, 2024 03:39

github-actions bot added the Area: Drivers Drivers issues label Nov 19, 2024

anchao reviewed Nov 19, 2024

View reviewed changes

sched/sched/sched_lock.c Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_3 branch 2 times, most recently from 87f9b88 to cde599d Compare November 20, 2024 01:38

github-actions bot added the Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture label Nov 20, 2024

github-actions bot added the Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture label Jan 15, 2025

hujun260 force-pushed the apache_3 branch 2 times, most recently from edd4876 to 21ddf45 Compare January 15, 2025 06:17

xiaoxiang781216 mentioned this pull request Jan 17, 2025

sim: fix regression from /~https://github.com/apache/nuttx/pull/14623 #15586

Merged

xiaoxiang781216 reviewed Jan 18, 2025

View reviewed changes

arch/risc-v/src/common/espressif/esp_pcnt.c Outdated Show resolved Hide resolved

xiaoxiang781216 reviewed Jan 18, 2025

View reviewed changes

hujun260 force-pushed the apache_3 branch from 21ddf45 to 95c8080 Compare January 19, 2025 07:55

github-actions bot removed the Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture label Jan 19, 2025

hujun260 force-pushed the apache_3 branch 2 times, most recently from 8ab2f7f to fe48a33 Compare January 19, 2025 09:28

xiaoxiang781216 reviewed Jan 19, 2025

View reviewed changes

hujun260 force-pushed the apache_3 branch 3 times, most recently from 09a3313 to 2e11d88 Compare January 20, 2025 04:55

xiaoxiang781216 reviewed Jan 20, 2025

View reviewed changes

include/nuttx/irq.h Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_3 branch from 2e11d88 to f3d786d Compare January 20, 2025 07:36

xiaoxiang781216 reviewed Jan 20, 2025

View reviewed changes

hujun260 added 3 commits January 21, 2025 11:04

spinlock: Due to semantic inconsistency, remove/rename some functions…

13b1538

… with the _wo_note suffix. Signed-off-by: hujun5 <hujun5@xiaomi.com>

atomic: use raw_spin_lock_irqsave replace spin_lock_irqsave_wo_note

474e7fe

Signed-off-by: hujun5 <hujun5@xiaomi.com>

irq: move restore_critical_section and cpu_irqlock_clear to sched/irq…

8d426c2

…/irq.h Signed-off-by: hujun5 <hujun5@xiaomi.com>

hujun260 force-pushed the apache_3 branch from f3d786d to 6c7b056 Compare January 21, 2025 03:06

hujun260 added 2 commits January 21, 2025 11:10

hujun260 force-pushed the apache_3 branch from 6c7b056 to b6fc1e0 Compare January 21, 2025 03:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spin_lock_irqsave+sched_lock #14578

spin_lock_irqsave+sched_lock #14578

hujun260 commented Oct 31, 2024 •

edited

Loading

xiaoxiang781216 commented Nov 18, 2024

xiaoxiang781216 Jan 18, 2025

xiaoxiang781216 Jan 18, 2025

hujun260 Jan 19, 2025

xiaoxiang781216 Jan 18, 2025

xiaoxiang781216 Jan 20, 2025

hujun260 Jan 21, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

xiaoxiang781216 Jan 20, 2025

spin_lock_irqsave+sched_lock #14578

Are you sure you want to change the base?

spin_lock_irqsave+sched_lock #14578

Conversation

hujun260 commented Oct 31, 2024 • edited Loading

Summary

Impact

Testing

xiaoxiang781216 commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hujun260 commented Oct 31, 2024 •

edited

Loading