Re: [PR] sched/signal: Fix signal delivered to a wrong thread [nuttx]
xiaoxiang781216 merged PR #16334: URL: https://github.com/apache/nuttx/pull/16334 -- 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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2079012684
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
Ok. that change to raise was just wrong. I reverted it. Found other bugs and
fixed those, I had made mistakes in signal pending set handling. Perhaps the
patch is now ok, at least passes tests on my PC (although I had to revert
#16231 , because with those changes more or less nothing works for me, I filed
an issue on that).
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078910326
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
Having said all that, I don't think this part of the PR is correct either,
the "raise" should send the signal specifically to the calling thread. I still
need to tinker with this.
I am currently having some issues with tests on the master, something has
happened yesterday or the day before, and for example sim:citest is just
segfaulting for me now.
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078906467
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
Having said all that, I don't think that this PR is correct either. "raise"
should send the signal explicitly only to the calling thread. I need to still
tinker with this.
I am currently having some issues with the master branch, there is some
instability after yesterday or the day before. for example sim:citest sometimes
just segfaults aready at the start
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078906467
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
Having said all that, I don't think that this PR is correct either. "raise"
should send the signal explicitly only to the calling thread. I need to still
tinker with this.
I am currently having some issues with the master branch, there is some
instability after yesterday or the day before. for example sim:citest sometimes
just segfaults aready at the start
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078895532
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
Or change the interface of tgkill, I don't see any use for PID in there. It
should be enough to pass any TID and just a boolean of whether the signal is to
be delivered to whole group or specifically to the TID.
I don't even understand why the (linux) tkill and tgkill have been added to
NuttX, this is completely redundant to nxsig_kill, only that "sig_kill" would
have needed the group/thread information as well.
So I'd suggest removing either nxsig_tgkill or nxsig_kill if we start to
optimize these interfaces
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078895532
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
Or change the interface of tgkill, I don't see any use for PID in there. It
should be enough to pass any TID and just a boolean of whether the signal is to
be delivered to whole group or specifically to the TID.
I don't even understand why the (linux) tkill and tgkill have been added to
NuttX, this is completely redundant to sig_kill and nxsig_kill, only that
sig_kill would have needed the group/thread information as well.
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
pussuw commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078865703
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
The calls are to getpid and gettid, not 2x getpid
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
xiaoxiang781216 commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078653224
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
yes, but it isn't harmful to add a local variable here to avoid call
_SCHED_GETPID twice.
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
pussuw commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078253692
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
That is one option, another could be to stash tcb->group->tg_pid into TLS as
well. Perhaps put it into struct task_info_s?
https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/sched/tls/tls_initinfo.c#L75
Anyway, it should be a separate topic (separate patch and PR).
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
pussuw commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078253692
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
That is one option, another could be to stash tcb->group->tg_pid into TLS as
well. Perhaps put it into struct task_info_s?
https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/sched/tls/tls_initinfo.c#L75
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
xiaoxiang781216 commented on code in PR #16334:
URL: https://github.com/apache/nuttx/pull/16334#discussion_r2078074826
##
libs/libc/signal/sig_raise.c:
##
@@ -45,5 +45,5 @@
int raise(int signo)
{
- return tkill(_SCHED_GETTID(), signo);
+ return tgkill(_SCHED_GETPID(), _SCHED_GETTID(), signo);
Review Comment:
can we save _SCHED_GETPID to local variable? getpid is very time consume in
protected and kernel build
--
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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on PR #16334: URL: https://github.com/apache/nuttx/pull/16334#issuecomment-2858578108 Hm. I need to still work on this later, it is not 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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on PR #16334: URL: https://github.com/apache/nuttx/pull/16334#issuecomment-2858527710 Rebased and pushed to start CI again... -- 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] sched/signal: Fix signal delivered to a wrong thread [nuttx]
jlaitine commented on PR #16334: URL: https://github.com/apache/nuttx/pull/16334#issuecomment-2858411760 Oops. Looks like, sig_tgkill doesn't handle sending to group/thread and also "raise" only sends to a single thread (using tkill). So now this didn't pass the sim:citest 's conformance test. Fixed sig_tgkill to just call nxsig_dispatch, and "raise" to use tgkill instead of tkill. Let's see if all the posix conformance tests pass now. I re-tested that this still fixes the original issue on real HW -- 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]
