Re: [PR] Signal dispatch cleanups [nuttx]
jlaitine commented on code in PR #16333: URL: https://github.com/apache/nuttx/pull/16333#discussion_r2077160868 ## sched/signal/sig_dispatch.c: ## @@ -435,6 +473,14 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info) flags = enter_critical_section(); + /* Make sure that there is always at least one sigpednq and sigq structure + * available, in case one needs to be queued later. Note that this breaks + * the critical section if it needs to allocate any new structures. So it + * needs to be done here before using the task state or sigprocmask. + */ + + flags = nxsig_alloc_dyn_pending(flags); Review Comment: We need to keep the critical section after adding the items to the free queues; otherwise it is in theory possible that other signal dispatcher thread in the same process steals it in between. And this one proceeds with 0 spare pending structures left. This is perhaps not the most common case, but still a race in theory ;) -- 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]
Re: [PR] Signal dispatch cleanups [nuttx]
jlaitine commented on PR #16333: URL: https://github.com/apache/nuttx/pull/16333#issuecomment-2857780485 Had to push again, since spell checked complained on some old comments. -- 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]
Re: [PR] Signal dispatch cleanups [nuttx]
xiaoxiang781216 commented on code in PR #16333: URL: https://github.com/apache/nuttx/pull/16333#discussion_r2077179740 ## sched/signal/sig_dispatch.c: ## @@ -435,6 +473,14 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info) flags = enter_critical_section(); + /* Make sure that there is always at least one sigpednq and sigq structure + * available, in case one needs to be queued later. Note that this breaks + * the critical section if it needs to allocate any new structures. So it + * needs to be done here before using the task state or sigprocmask. + */ + + flags = nxsig_alloc_dyn_pending(flags); Review Comment: Ok -- 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]
Re: [PR] Signal dispatch cleanups [nuttx]
xiaoxiang781216 commented on code in PR #16333: URL: https://github.com/apache/nuttx/pull/16333#discussion_r2077002464 ## sched/signal/sig_dispatch.c: ## @@ -110,18 +110,15 @@ static int sig_handler(FAR void *cookie) * / -static int nxsig_queue_action(FAR struct tcb_s *stcb, siginfo_t *info) +static int nxsig_queue_action(FAR struct tcb_s *stcb, + FAR sigactq_t *sigact, + siginfo_t *info) Review Comment: add FAR ## sched/signal/sig_dispatch.c: ## @@ -435,6 +473,14 @@ int nxsig_tcbdispatch(FAR struct tcb_s *stcb, siginfo_t *info) flags = enter_critical_section(); + /* Make sure that there is always at least one sigpednq and sigq structure + * available, in case one needs to be queued later. Note that this breaks + * the critical section if it needs to allocate any new structures. So it + * needs to be done here before using the task state or sigprocmask. + */ + + flags = nxsig_alloc_dyn_pending(flags); Review Comment: move not move before line 474 and let nxsig_alloc_dyn_pending do the protection by self? -- 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]
[PR] Signal dispatch cleanups [nuttx]
jlaitine opened a new pull request, #16333: URL: https://github.com/apache/nuttx/pull/16333 ## Summary The critical section scope was widened before to avoid race conditions between signal dispatch and signal delivery, which were especially harmful for SMP systems (but exist also for single CPU). This PR makes the scope a bit smaller by removing thigs which don't need to be inside the critical section, to more appropriate places. The nxsig_find_action can be done before entering the protected section, and the found action can be just passed in. The memory allocations which are done as a backup if one runs out of preallocated pending structures can be done beforehand, and the dynamically allocated structures simply injected into the free structure lists. This has the downside that it may leave a dynamically allocated pending structure in the free list for some period of time, in case it is not needed after all. But this is not an issue, it is freed eventually in case it gets used. ## Impact This is unlikely to have any impact in testing, since typically the preallocated pending structures never run out. But this makes the critical section a bit smaller again by re-structuring things which are not needed to be in the critical section outside. In the case of really running out of pre-allocated pending structures, this removes a rare, but potential race condition between dispatch and delivery, if the dispatch leaves critical section in the middle of the logic by sleeping on heap mutex. ## Testing Tested on qemu rv-virt:smp, and on real HW (MPFS). -- 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]
