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

change nxsched_islocked_global to nxsched_islocked_tcb #13716

Merged
merged 2 commits into from
Oct 5, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Sep 29, 2024

Summary

1 To improve efficiency, we mimic Linux's behavior where preemption disabling is
only applicable to the current CPU and does not affect other CPUs.
2 In the future, we will implement "spinlock+sched_lock", and use it extensively.
Under such circumstances, if preemption is still globally disabled, it will seriously impact
the scheduling efficiency.
3 We have removed g_cpu_lockset and used irqcount in order to eliminate the dependency of
schedlock on critical sections in the future, simplify the logic, and further enhance the
performance of sched_lock.
4 We set lockcount to 1 in order to lock scheduling on all CPUs during startup, without
the need to provide additional functions to disable scheduling on other CPUs.
5 Cpu (1-n) must wait for cpu0 to enter the idle state before enabling scheduling because
it prevents CPUs1~n from competing with cpu0 for the memory manager mutex, which could
cause the cpu0 idle task to enter a wait state and trigger an assert.

size nuttx
before:
   text    data     bss     dec     hex filename
 265396   51057   63646  380099   5ccc3 nuttx
after:
   text    data     bss     dec     hex filename
 265184   51057   63642  379883   5cbeb nuttx

size -216

Impact

sched_lock/sched_unlock

Testing

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

ostest

@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 Sep 29, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

NuttX PR Requirements Review

While this PR summary provides context and reasoning for the changes, it partially meets NuttX's PR requirements.

Here's what's missing or needs improvement:

  • Summary:
    • Functional part: Be more specific about which functional part of the code is changed (e.g., scheduler, interrupt handling, specific driver).
    • How it works: The explanation is high-level. Provide concrete details on the mechanism used to achieve per-CPU preemption disabling and how irqcount replaces g_cpu_lockset.
    • Issue references: Include links to relevant NuttX and/or NuttX Apps issues.
  • Impact:
    • User impact: While it mentions scheduling changes, clarify if any user-facing behavior is modified.
    • Build impact: Does this PR change any build configurations, dependencies, or options?
    • Hardware impact: Specify the affected architectures (ARMv8-A is implied but state it explicitly).
    • Documentation: Clearly state if documentation updates are required and whether the PR includes them.
    • Security, Compatibility: Address these even if there's no impact (e.g., "NO impact on security").
  • Testing:
    • Build Hosts: List all build environments used for testing (OS, CPU architecture, compiler).
    • Targets: Specify the exact board configurations within qemu-armv8a (e.g., qemu-armv8a:nsh_smp).
    • Testing logs: The logs are placeholder comments. Include actual logs demonstrating the issue before the change and the correct behavior after.

Recommendations:

  • Expand the summary to provide a more technically detailed explanation.
  • Address all impact categories explicitly, even if it's "NO impact."
  • Provide comprehensive testing information with specific build/target details and actual logs.

By addressing these points, your PR will be more informative and easier for reviewers to assess.

reason:
1 To improve efficiency, we mimic Linux's behavior where preemption disabling is only applicable to the current CPU and does not affect other CPUs.
2 In the future, we will implement "spinlock+sched_lock", and use it extensively. Under such circumstances, if preemption is still globally disabled, it will seriously impact the scheduling efficiency.
3 We have removed g_cpu_lockset and used irqcount in order to eliminate the dependency of schedlock on critical sections in the future, simplify the logic, and further enhance the performance of sched_lock.
4 We set lockcount to 1 in order to lock scheduling on all CPUs during startup, without the need to provide additional functions to disable scheduling on other CPUs.
5 Cpu1~n must wait for cpu0 to enter the idle state before enabling scheduling because it prevents CPUs1~n from competing with cpu0 for the memory manager mutex, which could cause the cpu0 idle task to enter a wait state and trigger an assert.

size nuttx
before:
   text    data     bss     dec     hex filename
 265396   51057   63646  380099   5ccc3 nuttx
after:
   text    data     bss     dec     hex filename
 265184   51057   63642  379883   5cbeb nuttx

size -216

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

Signed-off-by: hujun5 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 6a13e4a into apache:master Oct 5, 2024
29 checks passed
hujun260 added a commit to hujun260/nuttx that referenced this pull request Oct 8, 2024
This commit fixes the regression from apache#13716
Signed-off-by: hujun5 <[email protected]>
hujun260 added a commit to hujun260/nuttx that referenced this pull request Oct 9, 2024
This commit fixes the regression from apache#13716
Signed-off-by: hujun5 <[email protected]>
hujun260 added a commit to hujun260/nuttx that referenced this pull request Oct 9, 2024
This commit fixes the regression from apache#13716

Signed-off-by: hujun5 <[email protected]>
xiaoxiang781216 pushed a commit that referenced this pull request Oct 9, 2024
This commit fixes the regression from #13716

Signed-off-by: hujun5 <[email protected]>
@hujun260 hujun260 deleted the apache_1 branch October 10, 2024 05:32
jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Oct 10, 2024
This commit fixes the regression from apache#13716

Signed-off-by: hujun5 <[email protected]>
xiaoxiang781216 pushed a commit that referenced this pull request Oct 10, 2024
This commit fixes the regression from #13716

Signed-off-by: hujun5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants