| Message ID | 1552457573-1354-2-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | syscalls: add tgkill test-cases | expand |
Hi! The test itself looks good, my only concern is actually that checkpoints cannot be used for the keeping the thread asleep during the test. However I can easily add one function to the futex library: TST_CHECKPOINT_SLEEP(id) That would cause the thread to wait on the checkpoint until: * we were woken up * we timeouted Which would basically loop on tst_checkpoint_wait() and retry in case of EINTR. Maybe it would be a good idea to retry on EINTR in the TST_CHECKPOINT_WAIT(), then we could easily use that one here as well. I'm not sure though if there are tests that depends on checkpoints being interrupted by signals though, I would have to check. For the second part we already have a function to wake all threads waiting on checkpoint, but we have to specify exact number of threads to wait for, which is there in order to avoid race coditions (i.e. thread was not sleeping yet at the time we tried to wake it). So we would have to count the number of threads we want to wake before the call to the TST_CHECKPOINT_WAKE2().
On Thu, 14 Mar 2019 at 17:53, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > The test itself looks good, my only concern is actually that checkpoints > cannot be used for the keeping the thread asleep during the test. > However I can easily add one function to the futex library: > > TST_CHECKPOINT_SLEEP(id) > > That would cause the thread to wait on the checkpoint until: > > * we were woken up > * we timeouted > > Which would basically loop on tst_checkpoint_wait() and retry in case of > EINTR. > I am not sure how we would manage actual "msec_timeout" in case we get EINTR and need to retry again as we may need to take care of elapsed time till we receive asynchronous signal. -Sumit > Maybe it would be a good idea to retry on EINTR in the > TST_CHECKPOINT_WAIT(), then we could easily use that one here as well. > I'm not sure though if there are tests that depends on checkpoints being > interrupted by signals though, I would have to check. > > For the second part we already have a function to wake all threads > waiting on checkpoint, but we have to specify exact number of threads to > wait for, which is there in order to avoid race coditions (i.e. thread > was not sleeping yet at the time we tried to wake it). So we would have > to count the number of threads we want to wake before the call to the > TST_CHECKPOINT_WAKE2(). > > -- > Cyril Hrubis > chrubis@suse.cz
Hi! > I am not sure how we would manage actual "msec_timeout" in case we get > EINTR and need to retry again as we may need to take care of elapsed > time till we receive asynchronous signal. I would have just restarted the timeout after we got signal, the worst case that can happen is that in an unlikely case we will send a signals fast enough so that the checkpoint will never timeout. But even then the test library will timeout and would kill the process anyways. Another option is to switch checkpoints so that they use absolute timeout and pass clock_gettime() + msec_timeout as timeout. I would go for something as simple as: diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c index 5455d0378..5e5b11496 100644 --- a/lib/tst_checkpoint.c +++ b/lib/tst_checkpoint.c @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno, int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) { struct timespec timeout; + int ret; if (id >= tst_max_futexes) { errno = EOVERFLOW; @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) timeout.tv_sec = msec_timeout/1000; timeout.tv_nsec = (msec_timeout%1000) * 1000000; - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, - tst_futexes[id], &timeout); + do { + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, + tst_futexes[id], &timeout); + } while (ret == -1 && errno == EINTR); + + return ret; }
On Thu, Mar 14, 2019 at 9:59 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > I am not sure how we would manage actual "msec_timeout" in case we get > > EINTR and need to retry again as we may need to take care of elapsed > > time till we receive asynchronous signal. > > I would have just restarted the timeout after we got signal, the worst case > that can happen is that in an unlikely case we will send a signals fast > enough > Maybe we can print something useful there at least for friendly debugging if that unlikely case happens. > so that the checkpoint will never timeout. But even then the test library > will > timeout and would kill the process anyways. > > Another option is to switch checkpoints so that they use absolute timeout > and > pass clock_gettime() + msec_timeout as timeout. > > I would go for something as simple as: > > diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c > index 5455d0378..5e5b11496 100644 > --- a/lib/tst_checkpoint.c > +++ b/lib/tst_checkpoint.c > @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int > lineno, > int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) > { > struct timespec timeout; > + int ret; > > if (id >= tst_max_futexes) { > errno = EOVERFLOW; > @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int > msec_timeout) > timeout.tv_sec = msec_timeout/1000; > timeout.tv_nsec = (msec_timeout%1000) * 1000000; > > - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > - tst_futexes[id], &timeout); > + do { > + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > + tst_futexes[id], &timeout); if (ret == -1 && errno == EINTR) tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again"); + } while (ret == -1 && errno == EINTR); > + > + return ret; > } > > -- > Cyril Hrubis > chrubis@suse.cz > -- Regards, Li Wang <div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"></div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 14, 2019 at 9:59 PM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br> > I am not sure how we would manage actual "msec_timeout" in case we get<br> > EINTR and need to retry again as we may need to take care of elapsed<br> > time till we receive asynchronous signal.<br> <br> I would have just restarted the timeout after we got signal, the worst case<br> that can happen is that in an unlikely case we will send a signals fast enough<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">Maybe we can print something useful there at least for friendly debugging if that unlikely case happens.</div><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> so that the checkpoint will never timeout. But even then the test library will<br> timeout and would kill the process anyways.<br> <br> Another option is to switch checkpoints so that they use absolute timeout and<br> pass clock_gettime() + msec_timeout as timeout.<br> <br> I would go for something as simple as:<br> <br> diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c<br> index 5455d0378..5e5b11496 100644<br> --- a/lib/tst_checkpoint.c<br> +++ b/lib/tst_checkpoint.c<br> @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno,<br> int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)<br> {<br> struct timespec timeout;<br> + int ret;<br> <br> if (id >= tst_max_futexes) {<br> errno = EOVERFLOW;<br> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)<br> timeout.tv_sec = msec_timeout/1000;<br> timeout.tv_nsec = (msec_timeout%1000) * 1000000;<br> <br> - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,<br> - tst_futexes[id], &timeout);<br> + do {<br> + ret = <span class="gmail_default" style="font-size:small"></span>syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,<br> + tst_futexes[id], &timeout);</blockquote><div><div class="gmail_default" style="font-size:small"></div></div><div class="gmail_default" style="font-size:small"> if (ret == -1 && errno == EINTR)</div><div><div class="gmail_default"> tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again");</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + } while (ret == -1 && errno == EINTR);<br> +<br> + return ret;<br> }<br> <br> -- <br> Cyril Hrubis<br> <a href="mailto:chrubis@suse.cz" target="_blank">chrubis@suse.cz</a><br> </blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div></div></div></div></div></div></div></div></div>
On Thu, 14 Mar 2019 at 19:29, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > I am not sure how we would manage actual "msec_timeout" in case we get > > EINTR and need to retry again as we may need to take care of elapsed > > time till we receive asynchronous signal. > > I would have just restarted the timeout after we got signal, the worst case > that can happen is that in an unlikely case we will send a signals fast enough > so that the checkpoint will never timeout. But even then the test library will > timeout and would kill the process anyways. > > Another option is to switch checkpoints so that they use absolute timeout and > pass clock_gettime() + msec_timeout as timeout. > > I would go for something as simple as: > > diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c > index 5455d0378..5e5b11496 100644 > --- a/lib/tst_checkpoint.c > +++ b/lib/tst_checkpoint.c > @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno, > int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) > { > struct timespec timeout; > + int ret; > > if (id >= tst_max_futexes) { > errno = EOVERFLOW; > @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) > timeout.tv_sec = msec_timeout/1000; > timeout.tv_nsec = (msec_timeout%1000) * 1000000; > > - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > - tst_futexes[id], &timeout); > + do { > + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > + tst_futexes[id], &timeout); > + } while (ret == -1 && errno == EINTR); > + > + return ret; > } > Wouldn't this loop be more appropriate in "tst_safe_checkpoint_wait()"? As at later stage we may have tests that depends on checkpoints being interrupted by signals and could directly use "tst_checkpoint_wait()". -Sumit > -- > Cyril Hrubis > chrubis@suse.cz
On Fri, 15 Mar 2019 at 13:15, Li Wang <liwang@redhat.com> wrote: > > > > On Thu, Mar 14, 2019 at 9:59 PM Cyril Hrubis <chrubis@suse.cz> wrote: >> >> Hi! >> > I am not sure how we would manage actual "msec_timeout" in case we get >> > EINTR and need to retry again as we may need to take care of elapsed >> > time till we receive asynchronous signal. >> >> I would have just restarted the timeout after we got signal, the worst case >> that can happen is that in an unlikely case we will send a signals fast enough > > > Maybe we can print something useful there at least for friendly debugging if that unlikely case happens. >> >> so that the checkpoint will never timeout. But even then the test library will >> timeout and would kill the process anyways. >> >> Another option is to switch checkpoints so that they use absolute timeout and >> pass clock_gettime() + msec_timeout as timeout. >> >> I would go for something as simple as: >> >> diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c >> index 5455d0378..5e5b11496 100644 >> --- a/lib/tst_checkpoint.c >> +++ b/lib/tst_checkpoint.c >> @@ -85,6 +85,7 @@ void tst_checkpoint_init(const char *file, const int lineno, >> int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) >> { >> struct timespec timeout; >> + int ret; >> >> if (id >= tst_max_futexes) { >> errno = EOVERFLOW; >> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) >> timeout.tv_sec = msec_timeout/1000; >> timeout.tv_nsec = (msec_timeout%1000) * 1000000; >> >> - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, >> - tst_futexes[id], &timeout); >> + do { >> + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, >> + tst_futexes[id], &timeout); > > if (ret == -1 && errno == EINTR) > tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again"); > I am not sure if this warning message is desired for test-cases which needs to wait on checkpoints irrespective of signals like this tgkill01 test-case. -Sumit >> + } while (ret == -1 && errno == EINTR); >> + >> + return ret; >> } >> >> -- >> Cyril Hrubis >> chrubis@suse.cz > > > > -- > Regards, > Li Wang
Hi! > >> int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) > >> { > >> struct timespec timeout; > >> + int ret; > >> > >> if (id >= tst_max_futexes) { > >> errno = EOVERFLOW; > >> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) > >> timeout.tv_sec = msec_timeout/1000; > >> timeout.tv_nsec = (msec_timeout%1000) * 1000000; > >> > >> - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > >> - tst_futexes[id], &timeout); > >> + do { > >> + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > >> + tst_futexes[id], &timeout); > > > > if (ret == -1 && errno == EINTR) > > tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again"); > > > > I am not sure if this warning message is desired for test-cases which > needs to wait on checkpoints irrespective of signals like this > tgkill01 test-case. Agreed, it's not an error condition, it's just a coincidence that most of the tests does not get signals when they sleep on futex, otherwise thing would crash and burn. So I would argue that retrying on EINTR is actually a bug fix rather than anything else.
Hi! > Wouldn't this loop be more appropriate in > "tst_safe_checkpoint_wait()"? As at later stage we may have tests that > depends on checkpoints being interrupted by signals and could directly > use "tst_checkpoint_wait()". The tst_checkpoint_wait() has a single use in the source tree and that is testcases/lib/tst_checkpoint.c which is binary wrapper around checkpoints so that we can use them in shell scripts as well, which is pretty cool btw. And I think that we should retry on EINTR there as well. Also there does not seem to be test relying on checkpoints being interrupted by signals and I would like to avoid this pattern ideally as asynchronous events such as signals interrupting functions is something rather counter intuitive.
On Fri, Mar 15, 2019 at 6:09 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > >> int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout) > > >> { > > >> struct timespec timeout; > > >> + int ret; > > >> > > >> if (id >= tst_max_futexes) { > > >> errno = EOVERFLOW; > > >> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned > int msec_timeout) > > >> timeout.tv_sec = msec_timeout/1000; > > >> timeout.tv_nsec = (msec_timeout%1000) * 1000000; > > >> > > >> - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > > >> - tst_futexes[id], &timeout); > > >> + do { > > >> + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT, > > >> + tst_futexes[id], &timeout); > > > > > > if (ret == -1 && errno == EINTR) > > > tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted > by a signal, retry again"); > > > > > > > I am not sure if this warning message is desired for test-cases which > > needs to wait on checkpoints irrespective of signals like this > > tgkill01 test-case. > > Agreed, it's not an error condition, it's just a coincidence that most > of the tests does not get signals when they sleep on futex, otherwise > thing would crash and burn. So I would argue that retrying on EINTR is > actually a bug fix rather than anything else. > Okay, here I'm not insist to print the warning. But it's only use for hint on that worst situation which you were mentioned. If the checkpoint got signal leads to never timeout and test eventually killed by test lib. That would hard to know what happened at that moment. My concern is the situation is hard to reproduce later so just want to print more useful messeges:). -- Regards, Li Wang <div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 15, 2019 at 6:09 PM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br> > >> int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)<br> > >> {<br> > >> struct timespec timeout;<br> > >> + int ret;<br> > >><br> > >> if (id >= tst_max_futexes) {<br> > >> errno = EOVERFLOW;<br> > >> @@ -94,8 +95,12 @@ int tst_checkpoint_wait(unsigned int id, unsigned int msec_timeout)<br> > >> timeout.tv_sec = msec_timeout/1000;<br> > >> timeout.tv_nsec = (msec_timeout%1000) * 1000000;<br> > >><br> > >> - return syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,<br> > >> - tst_futexes[id], &timeout);<br> > >> + do {<br> > >> + ret = syscall(SYS_futex, &tst_futexes[id], FUTEX_WAIT,<br> > >> + tst_futexes[id], &timeout);<br> > ><br> > > if (ret == -1 && errno == EINTR)<br> > > tst_res(TWARN | TERRNO, "FUTEX_WAIT operation was interrupted by a signal, retry again");<br> > ><br> > <br> > I am not sure if this warning message is desired for test-cases which<br> > needs to wait on checkpoints irrespective of signals like this<br> > tgkill01 test-case.<br> <br> Agreed, it's not an error condition, it's just a coincidence that most<br> of the tests does not get signals when they sleep on futex, otherwise<br> thing would crash and burn. So I would argue that retrying on EINTR is<br> actually a bug fix rather than anything else.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Okay, here I'm not insist to print the warning. But it's only use for hint on that worst situation which you were mentioned. If the checkpoint got signal leads to never timeout and test eventually killed by test lib. That would hard to know what happened at that moment. My concern is the situation is hard to reproduce later so just want to print more useful messeges:).</div></div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>
Hi! > > > I am not sure if this warning message is desired for test-cases which > > > needs to wait on checkpoints irrespective of signals like this > > > tgkill01 test-case. > > > > Agreed, it's not an error condition, it's just a coincidence that most > > of the tests does not get signals when they sleep on futex, otherwise > > thing would crash and burn. So I would argue that retrying on EINTR is > > actually a bug fix rather than anything else. > > > > Okay, here I'm not insist to print the warning. But it's only use for hint > on that worst situation which you were mentioned. If the checkpoint got > signal leads to never timeout and test eventually killed by test lib. That > would hard to know what happened at that moment. My concern is the > situation is hard to reproduce later so just want to print more useful > messeges:). As for now that's only a hypotetical corner case, someone would have to send signals to such process sleeping on the checkpoint in a loop for that to happen. The problem is that printing any messages when checkpoint was interrupted by signal would lead to even greater confusion for tests that actually have to send signals to such processes.
On Fri, 15 Mar 2019 at 15:48, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > Wouldn't this loop be more appropriate in > > "tst_safe_checkpoint_wait()"? As at later stage we may have tests that > > depends on checkpoints being interrupted by signals and could directly > > use "tst_checkpoint_wait()". > > The tst_checkpoint_wait() has a single use in the source tree and that > is testcases/lib/tst_checkpoint.c which is binary wrapper around > checkpoints so that we can use them in shell scripts as well, which is > pretty cool btw. And I think that we should retry on EINTR there as > well. Ah, I see. > > Also there does not seem to be test relying on checkpoints being > interrupted by signals and I would like to avoid this pattern ideally as > asynchronous events such as signals interrupting functions is something > rather counter intuitive. > Ok got it. Should I pick up this change as separate patch in this patch-set or would you like to commit it and then I would update patch-set to use these checkpoints? -Sumit > -- > Cyril Hrubis > chrubis@suse.cz
On Fri, Mar 15, 2019 at 7:34 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > > I am not sure if this warning message is desired for test-cases which > > > > needs to wait on checkpoints irrespective of signals like this > > > > tgkill01 test-case. > > > > > > Agreed, it's not an error condition, it's just a coincidence that most > > > of the tests does not get signals when they sleep on futex, otherwise > > > thing would crash and burn. So I would argue that retrying on EINTR is > > > actually a bug fix rather than anything else. > > > > > > > Okay, here I'm not insist to print the warning. But it's only use for > hint > > on that worst situation which you were mentioned. If the checkpoint got > > signal leads to never timeout and test eventually killed by test lib. > That > > would hard to know what happened at that moment. My concern is the > > situation is hard to reproduce later so just want to print more useful > > messeges:). > > As for now that's only a hypotetical corner case, someone would have to > send signals to such process sleeping on the checkpoint in a loop for > that to happen. The problem is that printing any messages when > checkpoint was interrupted by signal would lead to even greater > confusion for tests that actually have to send signals to such > processes. > That's true. I'm ok to withdraw my suggestion. -- Regards, Li Wang <div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 15, 2019 at 7:34 PM Cyril Hrubis <<a href="mailto:chrubis@suse.cz">chrubis@suse.cz</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi!<br> > > > I am not sure if this warning message is desired for test-cases which<br> > > > needs to wait on checkpoints irrespective of signals like this<br> > > > tgkill01 test-case.<br> > ><br> > > Agreed, it's not an error condition, it's just a coincidence that most<br> > > of the tests does not get signals when they sleep on futex, otherwise<br> > > thing would crash and burn. So I would argue that retrying on EINTR is<br> > > actually a bug fix rather than anything else.<br> > ><br> > <br> > Okay, here I'm not insist to print the warning. But it's only use for hint<br> > on that worst situation which you were mentioned. If the checkpoint got<br> > signal leads to never timeout and test eventually killed by test lib. That<br> > would hard to know what happened at that moment. My concern is the<br> > situation is hard to reproduce later so just want to print more useful<br> > messeges:).<br> <br> As for now that's only a hypotetical corner case, someone would have to<br> send signals to such process sleeping on the checkpoint in a loop for<br> that to happen. The problem is that printing any messages when<br> checkpoint was interrupted by signal would lead to even greater<br> confusion for tests that actually have to send signals to such<br> processes.<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">That's true. I'm ok to withdraw my suggestion.</div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div></div></div>
Hi! > Ok got it. Should I pick up this change as separate patch in this > patch-set or would you like to commit it and then I would update > patch-set to use these checkpoints? I will send my patch to the ML soon, you can then rebase your work on the top of that then I will commit the patches in right order.
diff --git a/runtest/syscalls b/runtest/syscalls index d752dba..8cd99e0 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1399,6 +1399,8 @@ syslog10 syslog10 syslog11 syslog11 syslog12 syslog12 +tgkill01 tgkill01 + time01 time01 time02 time02 diff --git a/testcases/kernel/syscalls/tgkill/.gitignore b/testcases/kernel/syscalls/tgkill/.gitignore new file mode 100644 index 0000000..f4566fd --- /dev/null +++ b/testcases/kernel/syscalls/tgkill/.gitignore @@ -0,0 +1 @@ +tgkill01 diff --git a/testcases/kernel/syscalls/tgkill/Makefile b/testcases/kernel/syscalls/tgkill/Makefile new file mode 100644 index 0000000..a51080c --- /dev/null +++ b/testcases/kernel/syscalls/tgkill/Makefile @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# Copyright (c) 2018 Google, Inc. + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk + +include $(top_srcdir)/include/mk/generic_leaf_target.mk + +LDLIBS += -pthread diff --git a/testcases/kernel/syscalls/tgkill/tgkill.h b/testcases/kernel/syscalls/tgkill/tgkill.h new file mode 100644 index 0000000..a7d96f4 --- /dev/null +++ b/testcases/kernel/syscalls/tgkill/tgkill.h @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2018 Google, Inc. + */ + +#ifndef TGKILL_H +#define TGKILL_H + +#include "config.h" +#include "lapi/syscalls.h" + +static inline int sys_tgkill(int tgid, int tid, int sig) +{ + return tst_syscall(__NR_tgkill, tgid, tid, sig); +} + +static inline pid_t sys_gettid(void) +{ + return tst_syscall(__NR_gettid); +} + +#endif /* TGKILL_H */ diff --git a/testcases/kernel/syscalls/tgkill/tgkill01.c b/testcases/kernel/syscalls/tgkill/tgkill01.c new file mode 100644 index 0000000..acc9977 --- /dev/null +++ b/testcases/kernel/syscalls/tgkill/tgkill01.c @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2018 Google, Inc. + * + * tgkill() delivers a signal to a specific thread. Test this by installing + * a SIGUSR1 handler which records the current pthread ID. Start a number + * of threads in parallel, then one-by-one call tgkill(..., tid, SIGUSR1) + * and check that the expected pthread ID was recorded. + */ + +#include <pthread.h> +#include <stdlib.h> + +#include "tst_safe_pthread.h" +#include "tst_test.h" +#include "tgkill.h" + +struct thread_state { + pthread_t thread; + pid_t tid; +}; + +static char *str_threads; +static int n_threads = 10; +static struct thread_state *threads; + +static pthread_t sigusr1_thread; + +static int test_running; +static pthread_cond_t test_running_cond = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t test_running_mutex = PTHREAD_MUTEX_INITIALIZER; + +static void sigusr1_handler(int signum __attribute__((unused))) +{ + sigusr1_thread = pthread_self(); +} + +static void *thread_func(void *arg) +{ + struct thread_state *thread = arg; + + /** + * There is no standard way to map pthread -> tid, so we will have the + * child stash its own tid then notify the parent that the stashed tid + * is available. + */ + thread->tid = sys_gettid(); + + TST_CHECKPOINT_WAKE(0); + + pthread_mutex_lock(&test_running_mutex); + while (test_running) + pthread_cond_wait(&test_running_cond, &test_running_mutex); + pthread_mutex_unlock(&test_running_mutex); + + return arg; +} + +static void start_thread(struct thread_state *thread) +{ + SAFE_PTHREAD_CREATE(&thread->thread, NULL, thread_func, thread); + + TST_CHECKPOINT_WAIT(0); +} + +static void stop_threads(void) +{ + int i; + + test_running = 0; + pthread_cond_broadcast(&test_running_cond); + + for (i = 0; i < n_threads; i++) { + if (threads[i].tid == -1) + continue; + + SAFE_PTHREAD_JOIN(threads[i].thread, NULL); + threads[i].tid = -1; + } + + if (threads) + free(threads); +} + +static void run(void) +{ + int i; + + for (i = 0; i < n_threads; i++) + threads[i].tid = -1; + + test_running = 1; + for (i = 0; i < n_threads; i++) + start_thread(&threads[i]); + + for (i = 0; i < n_threads; i++) { + sigusr1_thread = pthread_self(); + + TEST(sys_tgkill(getpid(), threads[i].tid, SIGUSR1)); + if (TST_RET) { + tst_res(TFAIL | TTERRNO, "tgkill() failed"); + return; + } + + while (pthread_equal(sigusr1_thread, pthread_self())) + usleep(1000); + + if (!pthread_equal(sigusr1_thread, threads[i].thread)) { + tst_res(TFAIL, "SIGUSR1 delivered to wrong thread"); + return; + } + } + + tst_res(TPASS, "SIGUSR1 delivered to correct threads"); +} + +static void setup(void) +{ + if (tst_parse_int(str_threads, &n_threads, 1, INT_MAX)) + tst_brk(TBROK, "Invalid number of threads '%s'", str_threads); + + threads = SAFE_MALLOC(sizeof(*threads) * n_threads); + + struct sigaction sigusr1 = { + .sa_handler = sigusr1_handler, + }; + SAFE_SIGACTION(SIGUSR1, &sigusr1, NULL); +} + +static struct tst_option options[] = { + {"t:", &str_threads, "-t Number of threads (default 10)"}, + {NULL, NULL, NULL}, +}; + +static struct tst_test test = { + .options = options, + .needs_checkpoints = 1, + .setup = setup, + .test_all = run, + .cleanup = stop_threads, +};