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

sched: add nxsched_remove_self #13418

Merged
merged 1 commit into from
Sep 24, 2024
Merged

sched: add nxsched_remove_self #13418

merged 1 commit into from
Sep 24, 2024

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Sep 13, 2024

Summary

In the scenario of active waiting, context switching is inevitable, and we can eliminate redundant judgments.

code size
before
size nuttx
text data bss dec hex filename
262848 49985 63893 376726 5bf96 nuttx

after
size nuttx
text data bss dec hex filename
263324 49985 63893 377202 5c172 nuttx

reduce code size by -476

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

Impact

Testing

@hujun260 hujun260 force-pushed the apache_1 branch 2 times, most recently from 3c4e081 to a827d2d Compare September 13, 2024 03:15
sched/sched/sched.h Outdated Show resolved Hide resolved
@hujun260
Copy link
Contributor Author

The previous function nxsched_remove_readytorun suffered from being overly complex in its logic judgments due to its comprehensive functionality,
which also made it inconsistent with its name. By splitting this function into multiple, more specific functions for different scenarios,
we can significantly reduce the complexity of the code.

For instance, the intricate implementation of the TLIST_HEAD macro was once a necessity to be invoked frequently,
but now it's only required in certain scenarios, leading to a decrease in its frequency of use.
There are similar implementations that have undergone similar optimizations.

After these optimizations, we tested nxsem_wait and observed an approximate 10% performance gain in terms of cycles.
image

@anchao
Copy link
Contributor

anchao commented Sep 14, 2024

  1. nxsched_add_readytorun()/nxsched_remove_readytorun() are paired functions. If you feel the logic is not clear enough, you could separate the requirements according to the task status and merge list, but do not expose it to kernel developers, which will reduce the readability of the code and increase the maintenance cost.
  2. About newly added interface in this PR, there are special requirements for thread status and list merging, but in nxsched_remove_readytorun(), there will only be a few more conditional branches(if). If you want to optimize, we could add parameters to nxsched_remove_readytorun instead of using inexplicable interfaces to confuse developers.
  3. nxsched_remove()? did not you think the function naming is weird?

Kernel interfaces are as reusable as possible. We need to balance some performance (if), which does improve code readability and avoids many rookie errors. I don't want to see all interfaces in the future like freertos (****_FromISR)

@hujun260
Copy link
Contributor Author

  1. nxsched_add_readytorun()/nxsched_remove_readytorun() are paired functions. If you feel the logic is not clear enough, you could separate the requirements according to the task status and merge list, but do not expose it to kernel developers, which will reduce the readability of the code and increase the maintenance cost.
  2. About newly added interface in this PR, there are special requirements for thread status and list merging, but in nxsched_remove_readytorun(), there will only be a few more conditional branches(if). If you want to optimize, we could add parameters to nxsched_remove_readytorun instead of using inexplicable interfaces to confuse developers.
  3. nxsched_remove()? did not you think the function naming is weird?

Kernel interfaces are as reusable as possible. We need to balance some performance (if), which does improve code readability and avoids many rookie errors. I don't want to see all interfaces in the future like freertos (****_FromISR)

i update patch just add one interface nxsched_remove_self

@hujun260 hujun260 changed the title sched: rm nxsched_remove_readytorun sched: add nxsched_remove_self Sep 16, 2024
sched/sched/sched_removereadytorun.c Outdated Show resolved Hide resolved
sched/sched/sched_removereadytorun.c Outdated Show resolved Hide resolved
sched/sched/sched_removereadytorun.c Outdated Show resolved Hide resolved
@hujun260 hujun260 force-pushed the apache_1 branch 2 times, most recently from 871bac7 to 693b07c Compare September 19, 2024 15:03
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Sep 19, 2024
reason:
1In the scenario of active waiting, context switching is inevitable, and we can eliminate redundant judgments.

code size
before
hujun5@hujun5-OptiPlex-7070:~/downloads1/vela_sim/nuttx$ size nuttx
   text    data     bss     dec     hex filename
 262848   49985   63893  376726   5bf96 nuttx

after
hujun5@hujun5-OptiPlex-7070:~/downloads1/vela_sim/nuttx$ size nuttx
   text    data     bss     dec     hex filename
 263324   49985   63893  377202   5c172 nuttx

reduce code size by  -476

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 <hujun5@xiaomi.com>
Copy link
Contributor

@anchao anchao left a comment

Choose a reason for hiding this comment

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

LGTM

@anchao anchao merged commit e13d255 into apache:master Sep 24, 2024
29 checks passed
@hujun260 hujun260 deleted the apache_1 branch September 29, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants