Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spinlock+sched_lock #14578

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

spinlock+sched_lock #14578

wants to merge 7 commits into from

Conversation

hujun260
Copy link
Contributor

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.

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

@github-actions 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
@github-actions github-actions bot added the Arch: xtensa Issues related to the Xtensa architecture label Oct 31, 2024
include/nuttx/spinlock.h Outdated Show resolved Hide resolved
sched/sched/sched.h Outdated Show resolved Hide resolved
include/sched.h Show resolved Hide resolved
sched/sched/sched.h Outdated Show resolved Hide resolved
sched/sched/sched_lock.c Outdated Show resolved Hide resolved
sched/sched/sched_lock.c Outdated Show resolved Hide resolved
sched/sched/sched_lock.c Outdated Show resolved Hide resolved
sched/sched/sched_unlock.c Outdated Show resolved Hide resolved
sched/sched/sched_unlock.c Show resolved Hide resolved
sched/sched/sched_unlock.c Show resolved Hide resolved
include/nuttx/spinlock.h Outdated Show resolved Hide resolved
@xiaoxiang781216 xiaoxiang781216 linked an issue Nov 18, 2024 that may be closed by this pull request
@xiaoxiang781216
Copy link
Contributor

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

sched/sched/sched_lock.c Outdated Show resolved Hide resolved
sched/sched/sched_lock.c Outdated Show resolved Hide resolved
sched/sched/sched_lock.c Outdated Show resolved Hide resolved
@hujun260 hujun260 force-pushed the apache_3 branch 2 times, most recently from 87f9b88 to cde599d Compare November 20, 2024 01:38
@github-actions github-actions bot added the Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture label Nov 20, 2024
@xiaoxiang781216
Copy link
Contributor

please fix conflict

@hujun260
Copy link
Contributor Author

please fix conflict

done

sched/irq/irq_csection.c Show resolved Hide resolved
drivers/note/noterpmsg_driver.c Outdated Show resolved Hide resolved
drivers/note/noterpmsg_driver.c Outdated Show resolved Hide resolved
drivers/note/noterpmsg_driver.c Outdated Show resolved Hide resolved
include/nuttx/spinlock.h Outdated Show resolved Hide resolved
include/nuttx/spinlock.h Show resolved Hide resolved
…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:
allow the spin_lock_irqsave_wo_note to respond interrupts during the test set loop and improve RT performance

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>
…id recursive calls.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
reason:
spin_lock_wo_note/spin_unlock_wo_note  should be called in matching pairs.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
reason:
1: spin_lock_init and spin_initialize have similar functionalities.
2: spin_lock and spin_unlock should be called in matching pairs.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
reason:
we.avoid.use gcc express statement extension in spinlock, to enhance compatibility

Signed-off-by: hujun5 <hujun5@xiaomi.com>
sched/irq/irq_csection.c Show resolved Hide resolved
@@ -85,6 +86,8 @@ struct noterpmsg_driver_s g_noterpmsg_driver =
},
};

static spinlock_t g_note_driver_lock = SP_UNLOCKED;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move into noterpmsg_driver_s

@@ -291,6 +291,7 @@ static inline_function void spin_lock(FAR volatile spinlock_t *lock)
/* Lock without trace note */

spin_lock_wo_note(lock);
sched_lock_wo_note();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spin_lock_wo_note need schedlock too

@@ -353,6 +354,8 @@ spin_trylock_wo_note(FAR volatile spinlock_t *lock)
}

SP_DMB();
sched_lock_wo_note();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lose sched lock note in spin_trylock

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spin_trylock will call spin_trylock_wo_note
spin_trylock_wo_note will call sched_lock_wo_note

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spin_trylock should call sched_lock to trace the preemption state.

@@ -721,6 +725,7 @@ void spin_unlock_irqrestore_wo_note(FAR volatile spinlock_t *lock,
}

up_irq_restore(flags);
sched_unlock_wo_note();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rwlock need update too

@@ -721,6 +725,7 @@ void spin_unlock_irqrestore_wo_note(FAR volatile spinlock_t *lock,
}

up_irq_restore(flags);
sched_unlock_wo_note();
}
#else
# define spin_unlock_irqrestore_wo_note(l, f) ((void)(l), up_irq_restore(f))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up case need lock/unlock sched lock too

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 Area: Drivers Drivers issues Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread holding spinlock can be blocked. Thread can be unexpectedly suspended within a critical section.
4 participants