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

anchao · 2024-11-26T03:36:19Z

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)


remove no trace version from syscall

sched_lock_wo_note may be called in userspace

anchao · 2024-11-26T03:48:32Z

sched/sched/sched_unlock.c


-              nxsched_sporadic_lowpriority(rtcb);
+void sched_unlock(void)


void sched_unlock(void) { FAR struct tcb_s *tcb; if (!up_interrupt_context()) { return; } tcb = this_task(); if (tcb != NULL) { ... } }

anchao · 2024-11-26T04:04:36Z

sched/sched/sched_unlock.c

 ****************************************************************************/

+#if defined(CONFIG_SCHED_TICKLESS) && \
+           (CONFIG_RR_INTERVAL > 0 || defined(CONFIG_SCHED_SPORADIC))
+static int preempt_schedule_nestcount = 0;


could we remove this global variable?

nxsched_reassess_timer may also trigger sched_preempt_schedule
preempt_schedule_nestcount avoid recursive call

The logic before optimization does not have this issue? do you have the backtrace of recursive call?

Because we have placed sched_lock inside spin_lock_irqsave,
and spin_lock_irqsave is also extensively used to replace critical sections,
the usage of sched_lock has become very widespread, which makes recursion highly likely to occur.

ok, how about decrease lockcount after call sched_preemt_schedule(), This will ensure that the unlock operation is not recursively processed in context switch flow.

Due to the scheduling logic in sched_preemt_schedule that relies on checking the lockcount to determine the state,
if the lockcount is still greater than 0, we cannot distinguish whether the schedlock has been released or not.

or should we move this variable into tcb? I don't agree a global variable constantly updated in the scheduler, this is bad design.

tcb->flags should be able to solve this problem.

patacongo · 2024-11-26T18:30:38Z

[Note: my original quick email response had a few errors. I hope those are corrected here.]

The spinlock has always been a very light weight way of waiting. In SMP, sched_lock() is heavy weight and and also calls enter_critical_section() which is even heavier. This will effect performance.

General mutual exclusion is not required. All that is required is that the waiting thread no be suspended. You should consider a redesign to keep this as lightweight as possible. Consider this:

Since general mutual exclusion, is not required. It is not necessary to interact with the other CPUs at all
A single bit of information in the TCB will do the job.: If the thread holds the spinlock, that bit is set and, in that case the thread is never suspended. The bit is cleared after the spinlock is released.

This is a very simple change, similar to the way sched_lock() works in non-SMP mode: If the lockcount is greater than zero, then the thread cannot be suspended. And would return the spinlock to being a simple efficient interface. This would only affect the spinlock and scheduler. With would not affect other cpus or releases the spinlock.

anchao

Have you tested this code with SCHED_INSTRUMENTATION_CSECTION?

sched/irq/irq_csection.c

hujun260 · 2024-11-27T11:13:18Z

Have you tested this code with SCHED_INSTRUMENTATION_CSECTION?

yes

anchao · 2024-11-28T05:05:06Z

sched/irq/irq_csection.c

+
+  rtcb = this_task();
+  if (rtcb->irqcount == 1 && !up_interrupt_context())
+    {
+#  if CONFIG_SCHED_CRITMONITOR_MAXTIME_CSECTION >= 0
+      nxsched_critmon_csection(rtcb, false, return_address(0));
+#  endif
+#  ifdef CONFIG_SCHED_INSTRUMENTATION_CSECTION
+      sched_note_csection(rtcb, false);
+#  endif
+    }


Suggested change

rtcb = this_task();

if (rtcb->irqcount == 1 && !up_interrupt_context())

{

# if CONFIG_SCHED_CRITMONITOR_MAXTIME_CSECTION >= 0

nxsched_critmon_csection(rtcb, false, return_address(0));

# endif

# ifdef CONFIG_SCHED_INSTRUMENTATION_CSECTION

sched_note_csection(rtcb, false);

# endif

}

if (!up_interrupt_context())

{

rtcb = this_task();

if (rtcb->irqcount == 1)

{

# if CONFIG_SCHED_CRITMONITOR_MAXTIME_CSECTION >= 0

nxsched_critmon_csection(rtcb, false, return_address(0));

# endif

# ifdef CONFIG_SCHED_INSTRUMENTATION_CSECTION

sched_note_csection(rtcb, false);

# endif

}

}

Signed-off-by: hujun5 <[email protected]>

…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 <[email protected]>

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 <[email protected]>

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

Open

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

hujun260 force-pushed the apache_3 branch 4 times, most recently from 72f29e9 to c2c8fb5 Compare November 25, 2024 23:33

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

hujun260 force-pushed the apache_3 branch 2 times, most recently from ffa975d to 2f8fec9 Compare November 26, 2024 01:28

anchao reviewed Nov 26, 2024

View reviewed changes

hujun260 force-pushed the apache_3 branch 2 times, most recently from d5b0088 to 178dc49 Compare November 26, 2024 12:57

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

hujun260 force-pushed the apache_3 branch from 178dc49 to 017174e Compare November 27, 2024 01:54

anchao reviewed Nov 27, 2024

View reviewed changes

sched/irq/irq_csection.c Outdated Show resolved Hide resolved

sched/irq/irq_csection.c Outdated Show resolved Hide resolved

sched/irq/irq_csection.c Outdated Show resolved Hide resolved

sched/irq/irq_csection.c Outdated Show resolved Hide resolved

hujun260 force-pushed the apache_3 branch from 017174e to 96d42c5 Compare November 27, 2024 11:12

hujun260 closed this Nov 27, 2024

hujun260 reopened this Nov 27, 2024

hujun260 force-pushed the apache_3 branch from 96d42c5 to 689001f Compare November 28, 2024 02:43

anchao reviewed Nov 28, 2024

View reviewed changes

hujun260 force-pushed the apache_3 branch from 689001f to 798e25f Compare November 28, 2024 05:35

anchao approved these changes Nov 28, 2024

View reviewed changes

hujun260 force-pushed the apache_3 branch from 798e25f to a6ff2d3 Compare December 2, 2024 02:01

hujun260 added 3 commits December 11, 2024 09:00

irq: enter_critical_section_wo_note/leave_critical_section_wo_note

e10c575

Signed-off-by: hujun5 <[email protected]>

hujun260 force-pushed the apache_3 branch from a6ff2d3 to a701061 Compare December 11, 2024 01:01

This was referenced Dec 11, 2024

pthread: remove enter_critical_section in pthread_mutex #15126

Open

sched/wdog: use small lock to protect g_wdactivelist #15130

Open

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

anchao Nov 26, 2024

hujun260 Nov 26, 2024

anchao Nov 26, 2024

hujun260 Nov 26, 2024

anchao Nov 26, 2024

hujun260 Nov 26, 2024 •

edited

Loading

anchao Nov 26, 2024

hujun260 Nov 26, 2024

anchao Nov 27, 2024

hujun260 Nov 27, 2024

anchao Nov 27, 2024

hujun260 Nov 27, 2024

patacongo commented Nov 26, 2024 •

edited

Loading

anchao left a comment

hujun260 commented Nov 27, 2024

anchao Nov 28, 2024

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

hujun260 Nov 26, 2024 • edited Loading

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

patacongo commented Nov 26, 2024 • edited Loading

anchao left a comment

Choose a reason for hiding this comment

hujun260 commented Nov 27, 2024

Choose a reason for hiding this comment

hujun260 commented Oct 31, 2024 •

edited

Loading

hujun260 Nov 26, 2024 •

edited

Loading

patacongo commented Nov 26, 2024 •

edited

Loading