-
Notifications
You must be signed in to change notification settings - Fork 1.4k
sched/signal: Add support to disable signals #17352
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?
Conversation
cce68d3 to
387e274
Compare
raiden00pl
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.
you can't disable signals under POSIX. As much as I like this change and minimizing the footprint of NuttX, this change is against INVIOLABLES.md and it certainly can't be merged without more discussion in the community.
| cp15_cacheops.c) | ||
|
|
||
| if(NOT CONFIG_DISABLE_SIGNALS) | ||
| list(APPEND SRCS arm_schedulesigaction.c arm_sigdeliver.c) |
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.
need change irq.h too:
746966d#diff-9e6b14f89a1f89e46914da0086d8e74e4d511ce0907333ca592dfda73368207fR132-R138
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.
not fix this comment yet.
OK. If we disable only the signal action feature while keeping the rest of the signal subsystem intact, we still need to retain In addition, selectively disabling only the signal action functionality would require refactoring the implementation to clearly separate signal-action logic from the rest of the signal subsystem, which increases overall complexity. But it may still be worthwhile, since disabling signal actions only can also improve task initialization/creation/switch/exit performance and reduce task stack usage. I will look into the details and evaluate the pros and cons. |
Yes, I plan to implement sleep alternatives with #17368
I think the version without signals are better
I will show these data later |
|
I guess apache/nuttx-apps#3217 by @extinguish is a prototype on @xiaoxiang781216 idea? :-) |
Yes, this is a compromise if we want to make the most POSIX application/driver work as before. In very simple case, the full disable may work well, but the partial disable may work better in many normal case.
#17357 already finish the work, it isn't complex than the full disable:
here is the code save:
|
Yes, these points your raised makes total sense. |
|
I already point out the key problem of #17065, which must be fixed before I can approve it.
Do you review #17352 and #17357 carefully? The detail is totally different as I already mention many time, so I don't repeat the difference here again.
I review @wangchdo 's patch carefully and point out the place which need improve, once the problem get fixed, I always approve and his change. could you point out which patch I block it after the comment get addressed? |
Of course, I am very familiar with the implementation of BTW I dare say that if wangcd hadn't submitted this commit, you guys wouldn't have submitted it either, right? |
|
To reiterate, the current implementation of signals is highly unsafe because it borrows the context of the interrupted thread in its delivery logic. If a lock is held in the signal context, a serious bug will occur, which is why we prohibit the use of signals. |
|
As @raiden00pl mentioned, many POSIX capabilities actually rely on the MMU implementation; for example, fork/signal cannot be fully replicated on devices without virtual addresses. |
|
@xiaoxiang781216 I think we should have a Roadmap from Xiaomi of commits submission, to avoid issues like this. There are my useful things that Xiaomi people did and most still not updated to mainline yet, this causes people to waste their time re-implementing something that you guys already implemented (CMUX support is an example, fortunately the submitted implemented was for userspace and will not conflict with yours and you explained that didn't send it early because it had some issues). Having this Roadmap will avoid:
I think it also will be useful to bring more awareness of what is coming next and community devs could engage on this process to improve that solution before that it submitted here. |
Signal just like a software interrupt, which has many limitation similar with you can't do many thing in hardware interrupt. |
But our system does not follow this standard, especially VFS. On our real device, there was a bug caused by a POSIX library using read/write interfaces in the signal context, resulting in a deadlock, I merely wish to state that there is still a long way to go, which, of course, is irrelevant to the current topic. |
Who come to first isn't important, we should select the implementation which is better and complete. If both implementation is valuable like cmux and signal, both should be merged. If some work need be done to merge both and the contributor doens't have time to do it, we can take time to do it and keep their credit.
yes, it will be better to have a place to share the roadmap. |
If we have a better implementation that retains the individual developer's contribution, that's the best solution. |
I suggest using the Projects here in github to maps the Roadmap features. I should include the supposed date (Weeks of Year 1-52 or Q1-Q3 for distant/complex features). It could contains reference to existing open Issues, to explain what could be implemented in the coming PR. |
|
cde0bbb to
16259f2
Compare
Currently nuttx is more and more used on small embedded systems with limited resources. Some of these systems do not require signal handling functionality or posix apis. This patch adds a configuration option to disable signal handling in order to save code size and memory. Signed-off-by: Chengdong Wang <[email protected]>
Update architecture-level signal implementations to support optional signal disabling. Signed-off-by: Chengdong Wang <[email protected]>
16259f2 to
776072e
Compare
As @anchao mentioned — and I completely agree — signals are disabled not only to reduce footprint, but also to improve safety.
By the way, I work for Li Auto, a well-known Chinese new energy vehicle manufacturer that has sold more than 1.4 million cars. I am responsible for using NuttX to develop platform software for our vehicle control systems. We place great emphasis on both footprint and safety, and of course, safety is always our top priority. We have also open-sourced our project, which is available at: https://gitee.com/haloos |
Again I don't against the full disable signal, but the full disable can't be enabled in many cases. The disable signal by level is more flexiable and useful, and let the end user enable the full or partial signal disable case by case.
Do you review @extinguish's patch? the partial signal disable remove all these unsafe part you mention, but keep the useful and safe component to improve the POSIX compliant.
It's great that more and more company develop the different solution on top of NuttX.
But, we need consider other application(e.g. IoT, embeded, AI..) too since many people use NuttX on these fields. Actually, we also use NuttX on many products including car, that's why I suggest to accept both approach. |
|
Thank you guys! I have sent the update on the mailing list with this PR and apache/nuttx-apps#3217 (applications update signal disable). What is the Signal Disable impact on the Drivers? |
With the partial signal disable solution, driver which notify the event to userspace through signal still work if it uses sigwaitinfo(not signal handler) to receive the event, for example, button driver notification example work as before: |
@xiaoxiang781216 thank you! :-) So it looks like we have two general options and approaches, is this correct?
Maybe we can consider both options as possible or only one that offers biggest benefit at the lowest cost?
|
Yes.
sigaction/signal and sigpending can't be used anymore, all othe sigxxx function work as before https://github.com/apache/nuttx/pull/17357/files#diff-b2638ab8551b8ccdc2caf1d064e4dd07ea1bcd1405a562591213d6ed10969594R175
here is the number from: #17357
disable signal is just an option, just like many DISABLE_XXX option here:
It's just a simple change(exclude the signal related functions from build), and the wrong config will report the link error, not runtime error. I can't understand why do we spend so much time to consider the meaningless case. If your driver or apps use signal, don't enable CONFIG_DISABLE_SIGNALS, that's it. |
I think that is a good feature if we could still getting working the buttons, zerocross and other drivers that depends on signal. @xiaoxiang781216 @wangchdo @anchao I think most of people that will decide it doesn't have the complete understanding how signals works on NuttX. I suggest creating a Documentation explains how do signals work on NuttX. It could be an overview, but should explain details as @anchao commented about signals on NuttX using the ISR context, etc. Also it should explain how the partial disable works. @xiaoxiang781216 @wangchdo is it possible to have more two disable levels: partial (where buttons, etc, works) and full (not signal at all, buttons will not work) ? |
Summary
Depends-on: apache/nuttx-apps#3217 (apps updated to disabled signals).
Currently, NuttX is being adopted increasingly on small embedded systems with tight resource constraints. Many of these systems do not require signal handling functionality or POSIX-related APIs.
With #17200 introducing scheduled sleep support, and #17204 replacing all signal-based sleep implementations in drivers and the filesystem with scheduled sleep, the dependency on signals has been significantly reduced.
Therefore, it is now a good time to provide an option for users to disable signal support entirely, allowing them to reduce code size and memory usage when signal functionality is not needed.
Impact
Add configuration support that allows users to disable signal functionality.
When signals are disabled, the related POSIX APIs—including sleep, usleep, kill, pkill, and pthread—will be disabled as well.
Testing
ostest is heavily dependent on POSIX APIs—including sleep, usleep, kill, pkill, and pthread. Therefore, to disable signal support, ostest must also be disabled until it is refactored to reduce its reliance on these APIs.
I have tested the signal-disable option on the fvp-armv8r-aarch32 board with ostest disabled, and the build succeeds and runs correctly.