-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Refine the signal handling in sim #1663
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
Conversation
|
@sebastianene07 please review this patch. To make #1602 enter the mainline, I would suggest you don't put all code into one patch, it's better to split that patch into two: |
22a0164 to
206c14c
Compare
|
|
||
| void up_disable_irq(int irq) | ||
| { | ||
| signal(irq, SIG_IGN); |
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.
MAYBE: We can use the same mechanism from up_enable_irq to specify a blocking signal mask with pthread_sigmask as signal(..) specifies the behaviour for the entire process(is like blocking the interrupts on all CPUs).
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.
pthread_sigmask can only block the current thread, it's hard to block all threads directly with signal API. That's why I simply change the signal handle to ignore. BTW, up_disable_irq just make the linker happy, nobody really call it.
| * | ||
| ****************************************************************************/ | ||
|
|
||
| void up_enable_irq(int irq) |
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.
Q: This function will enable interrupts for a CPU. The behaviour is that child threads will copy the signal mask upon creation. Do you think it's better for us to block all the signals in sim_idle_trampoline except the SIGUSR1 ?
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.
It's good to unmask the signal on all threads because we can simulate the hardware behaviour more closely.
| * Private Types | ||
| ****************************************************************************/ | ||
|
|
||
| union sigset_u |
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.
GG ! This looks good
I think this is a good idea to split the patch as it makes the review more efficient. Thanks ! |
206c14c to
3254ff5
Compare
I will rebase my patches on top of this, once this enters master |
to avoid the host signal process interrupt the execution of NuttX critical section Signed-off-by: Sebastian Ene <nuttx@fitbit.com>
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
3254ff5 to
d8604c5
Compare
Summary
Model the host signal handling as NuttX's irq. The next step is:
1.Save/restore context with ucontext API
2.trigger the context switch by signal
Impact
Testing