-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support disable signal #17357
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
base: master
Are you sure you want to change the base?
Support disable signal #17357
Conversation
2148d7d to
c904e1f
Compare
anchao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please refer to #17352 as the primary commit; @wangchdo submitted this change first.
- cc NuttX PMC and commiterrs, Please do not merge this PR, especially @xiaoxiang781216
c904e1f to
6b7125c
Compare
The previous submission at #17352 had an overly broad scope when disabling signals, as it also disabled functionalities like slee, kill etc. The current patchset narrows this scope, ensuring that APIs such as sleep and kill remain available while only certain signal functionalities are disabled. Alternatively, we could introduce a concept like "disable levels" to differentiate the extent of signal functionality being disabled. |
For sleep()/usleep(), compatibility with the CONFIG_DISABLE_SIGNALS scenario should be considered in the implementation. For other APIs that have mandatory dependencies on signals (such as kill()/pkill()), scenario-based optimization can be performed—even the interfaces can be retained with error code returns supported. |
Basically, signal has two reaction:
The major memory/code size contribution come from the first item, the second item consume the small code/memory, but could keep many frequent used function(e.g. sleep/usleep/wait etc.). So, the best approach is disable signal by level to make it more useful in the different case.
The detailed implementation is different, both approach could benefit in the different usage. It's better to introduce the disable level concept, so both pr could be merged. |
3188735 to
7ec86dc
Compare
Why do we need two PRs for this? |
It's fine to use one pr to do the work, but someone need help to merge them. For example, for example, this pr contain more clean than #17352: Bascially, we can arrange the pr into three part:
Who volunteers to complete this work? @wangchdo or @extinguish. |
I don't think this PR is better because if signal support is disabled, then the related APIs should also be disabled. |
I can take care of all the works related to disabling signals , and I'm happy to do so. By the way, since this is still under VOTING on the mailing list, I think this work is not very urgent. |
I think |
wherer do I say this patch is better? I just claim that both approach can be used in the different case. I just said that some dead fields forget to remove from #17352, need merge the change in this pr to make it complete.
Could you review the patch carefully before making the comment? In this patch, you can still interrupt the target thread from sleep, but you can't trigger the signal handle. |
I just think this PR isn't perfect enough, because the sleep process still uses the implementation of nxsig_clockwait() |
This patch try to keep the signal wake up functionality, from this context, the change is consistent and perfect. |
7ec86dc to
746966d
Compare
Well, the initial split is now complete:
The "One patch skip the signal dispatch in the common code" part may need clarification regarding the disable level. |
Upon reconsideration, I prefer to only allow disabling all signal-related functionality, and re-implement the signal-dependent functions such as sleep()/usleep() using the newly added scheduler-based sleep APIs.
|
1. add the DISABLE_SIGNALS build option 2. make the sleep/usleep/nanosleep function can work with signal function disabled 3. make the driver/fs/sched/libc kernel module can decouple from signal module Signed-off-by: guoshichao <[email protected]>
make the following arch can work when signal is disabled: 1. arch/arm 2. arm/arm64 3. arch/avr 4. arch/z80 5. arch/mips 6. arch/x86 7. arch/misoc 8. arch/tricore 9. arch/risc-v 10. arch/sim 11. arch/z16 12. arch/xtensa 13. arch/or1k 14. archrenesas 15. arch/sparc Signed-off-by: guoshichao <[email protected]>
746966d to
652167c
Compare
Summary
Added support for signal decoupling. In scenarios with strict requirements for runtime memory and code size, this feature can be enabled by setting CONFIG_DISABLE_SIGNALS=y, which will disable the core functionality of signals.
It should be noted that CONFIG_DISABLE_SIGNALS=y does not completely disable all signal functionalities, as some operations such as kill and sleep remain essential in certain contexts. Therefore, only the core queue component of signals is disabled, while basic operations like sleep and kill continue to be supported.
Impact
This feature does not modify the functional logic of existing capabilities. It only decouples the signal functionality.
When
CONFIG_DISABLE_SIGNALS=y, it can reduce runtime memory usage and decrease code size.Testing
When
CONFIG_DISABLE_SIGNALS=n:Binary size = 1,295,424 bytes, Used RAM = 37,980 bytes
When
CONFIG_DISABLE_SIGNALS=y:Binary size = 1,262,624 bytes, Used RAM = 37,852 bytes
This shows a reduction of 32,800 bytes in binary size and 128 bytes in RAM usage.