xiaoxiang781216 commented on PR #17352: URL: https://github.com/apache/nuttx/pull/17352#issuecomment-3569567608
> > > > > When signals are disabled, the related POSIX APIs—including sleep, usleep, kill, pkill, and pthread—will be disabled as well. > > > > > > > > > > > > It's too limit that sleep/usleep can't be called when CONFIG_DISABLE_SIGNALS equals true, so I would suggest that this feature should be done by level: > > > > > > > > 1. disable all signal related functionality like this pr > > > > 2. disable signal function related to signal handler(callback), but keep other simple but frequnctly used function(e.g. wait/sigwait/ppoll). > > > > 3. enable all signal functionality like before > > > > > > > > > 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. > > > > > > not only sleep/usleep, my real concern is the following functions: > > ``` > > pid_t wait(FAR int *stat_loc); > > int waitid(idtype_t idtype, id_t id, FAR siginfo_t *info, int options); > > pid_t waitpid(pid_t pid, FAR int *stat_loc, int options); > > ``` > > > > > > > > > > > > > > > > > > > > > > > > These functions is important to make nsh work correctly. > > > 1. This approach is clearer and safer. If we allow disabling only part of the signal subsystem, we would need to modify the implementations of the remaining signal functions. > > > > > > But we just need skip the signal dispatch, no other change. > > > At the same time, we cannot guarantee that those functions will continue to behave exactly as before. Even worse, it becomes harder for users to understand the actual impact of partially disabling signal features. > > > > > > The rule is simple, signal action doesn't work, but all other signal related feature work as before. > > OK. If we disable only the signal action feature while keeping the rest of the signal subsystem intact, we still need to retain `sigset_t sigprocmask, sigset_t sigwaitmask, and siginfo_t *sigunbinfo in struct tcb_s`, and only remove `sq_queue_t sigpendactionq and sq_queue_t sigpostedq`. The remaining fields `(sigprocmask, sigwaitmask, and sigunbinfo`) occupy about 20 bytes in total, whereas` sigpendactionq and sigpostedq` together take about 16 bytes. > 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. > 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. > #17357 already finish the work, it isn't complex than the full disable: https://github.com/apache/nuttx/pull/17357/commits/a5678552cc2bfec8113e7f51b621b860b1b8d9d7 > 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. > here is the code save: ``` 1. has passed ostest on CONFIG_DISABLE_SIGNALS=y 2. Here are the test results from our ARMv7-A platform: 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. ``` > I will look into the details and evaluate the pros and cons. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
