| Message ID | 20190924105801.7616-1-liwang@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v3] fzsync: revoke thread_b if parent hits an accidental break | expand |
Hello, Li Wang <liwang@redhat.com> writes: > We shouldn't rely entirely on the pair->exit flag in tst_fzsync_pair_cleanup() > since there is possible to call tst_brk() at anyplace of thread_a, that will > lead to timeout eventually because of thread_b(tst_fzsync_wait_b) fall into > an infinite(no explicit condition to exit) loop. > > Thread_a path trace: > tst_brk() > cleanup() > tst_fzsync_pair_cleanup() > SAFE_PTHREAD_JOIN(pair->thread_b, NULL) > #Or pthread_cancel(pair->thread_b) > > We fix the problem via a way to kill thread_b with pthread_cancel. Good idea, however not an easy one to implement. > > Work-around: [commit 2e74d996: Check recvmmsg exists before entering fuzzy loop] > Signed-off-by: Li Wang <liwang@redhat.com> > Cc: Richard Palethorpe <rpalethorpe@suse.com> > Cc: Cyril Hrubis <chrubis@suse.cz> > --- > > Notes: > Patch V2: http://lists.linux.it/pipermail/ltp/2019-January/010489.html > Patch V1: http://lists.linux.it/pipermail/ltp/2019-January/010438.html > > include/tst_fuzzy_sync.h | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h > index f9a1947c7..152f779cb 100644 > --- a/include/tst_fuzzy_sync.h > +++ b/include/tst_fuzzy_sync.h > @@ -63,6 +63,7 @@ > #include <time.h> > #include <math.h> > #include <stdlib.h> > +#include <pthread.h> > #include "tst_atomic.h" > #include "tst_timer.h" > #include "tst_safe_pthread.h" > @@ -218,9 +219,13 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) > { > if (pair->thread_b) { > - tst_atomic_store(1, &pair->exit); > - SAFE_PTHREAD_JOIN(pair->thread_b, NULL); > - pair->thread_b = 0; > + if (pair->exit == 1) { It can just be if (!pair->exit) { ... } We want to join the thread and set the func pointer to zero regardless of how we exit. > + SAFE_PTHREAD_JOIN(pair->thread_b, NULL); > + pair->thread_b = 0; > + } else { I suggest still setting pair->exit here and maybe sleeping for 100ms. This gives thread B chance to exit gracefully. It is possible that if thread B is in a spin loop then the thread won't be cancelled as asynchronous cancellation is not guaranteed by POSIX. > + pthread_cancel(pair->thread_b); > + pair->thread_b = 0; > + } > } > } > > @@ -271,8 +276,11 @@ static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, > pair->a_cntr = 0; > pair->b_cntr = 0; > pair->exit = 0; > - if (run_b) > + if (run_b) { > + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); > + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); These need to go inside thread B unless I am mistaken. Which means you must wrap the user supplied function. You can create a function which accepts a pointer to some contiguous memory containing the user supplied function pointer and the user supplied arg pointer. This can then set the threads cancel type before calling the user func with the arg. > SAFE_PTHREAD_CREATE(&pair->thread_b, 0, run_b, 0); > + } > > pair->exec_time_start = (float)tst_timeout_remaining(); > } -- Thank you, Richard.
Hi Richard, On Tue, Sep 24, 2019 at 8:42 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > ... > It can just be > > if (!pair->exit) { > ... > } > > We want to join the thread and set the func pointer to zero regardless > of how we exit. > OK. > > > + SAFE_PTHREAD_JOIN(pair->thread_b, NULL); > > + pair->thread_b = 0; > > + } else { > > I suggest still setting pair->exit here and maybe sleeping for > 100ms. This gives thread B chance to exit gracefully. It is possible > that if thread B is in a spin loop then the thread won't be cancelled as > asynchronous cancellation is not guaranteed by POSIX. > Good suggestion. That'd be better to give one more time for thread B exiting gracefully. > > + pthread_cancel(pair->thread_b); > > + pair->thread_b = 0; > > + } > > } > > } > > > > @@ -271,8 +276,11 @@ static void tst_fzsync_pair_reset(struct > tst_fzsync_pair *pair, > > pair->a_cntr = 0; > > pair->b_cntr = 0; > > pair->exit = 0; > > - if (run_b) > > + if (run_b) { > > + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); > > + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); > > These need to go inside thread B unless I am mistaken. Which means you > Right. > must wrap the user supplied function. You can create a function which > accepts a pointer to some contiguous memory containing the user supplied > function > pointer and the user supplied arg pointer. > Since you have fixed the function format of thread B as void *(*run_b)(void *) in tst_fzsync_pair_reset(), which means we have no need to take care of the function arg pointer anymore. So just like what I did in V2, the wrapper function could steal the real run_b address from pthread_create(..., wrap_run_b, run_b) parameter.
Hello, Li Wang <liwang@redhat.com> writes: > Hi Richard, > > On Tue, Sep 24, 2019 at 8:42 PM Richard Palethorpe <rpalethorpe@suse.de> > wrote: > >> ... >> It can just be >> >> if (!pair->exit) { >> ... >> } >> >> We want to join the thread and set the func pointer to zero regardless >> of how we exit. >> > > OK. > > >> >> > + SAFE_PTHREAD_JOIN(pair->thread_b, NULL); >> > + pair->thread_b = 0; >> > + } else { >> >> I suggest still setting pair->exit here and maybe sleeping for >> 100ms. This gives thread B chance to exit gracefully. It is possible >> that if thread B is in a spin loop then the thread won't be cancelled as >> asynchronous cancellation is not guaranteed by POSIX. >> > > Good suggestion. That'd be better to give one more time for thread B > exiting gracefully. > > >> > + pthread_cancel(pair->thread_b); >> > + pair->thread_b = 0; >> > + } >> > } >> > } >> > >> > @@ -271,8 +276,11 @@ static void tst_fzsync_pair_reset(struct >> tst_fzsync_pair *pair, >> > pair->a_cntr = 0; >> > pair->b_cntr = 0; >> > pair->exit = 0; >> > - if (run_b) >> > + if (run_b) { >> > + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); >> > + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); >> >> These need to go inside thread B unless I am mistaken. Which means you >> > > Right. > > >> must wrap the user supplied function. You can create a function which >> accepts a pointer to some contiguous memory containing the user supplied >> function >> pointer and the user supplied arg pointer. >> > > Since you have fixed the function format of thread B as void *(*run_b)(void > *) in tst_fzsync_pair_reset(), which means we have no need to take care of > the function arg pointer anymore. I think the function pointer signature would be 'void *(*run_b)(void)' not 'void *(*run_b)(void *)'. I doubt any test would need the arg though, because we only use one thread and can store parameters in global variables. So you could remove it and update the tests. The user might need that arg if they are starting many threads, but for now we don't have explicit support for that in the library. > > So just like what I did in V2, the wrapper function could steal the real > run_b address from pthread_create(..., wrap_run_b, run_b) parameter. -- Thank you, Richard.
Richard Palethorpe <rpalethorpe@suse.de> wrote: >> must wrap the user supplied function. You can create a function which > >> accepts a pointer to some contiguous memory containing the user supplied > >> function > >> pointer and the user supplied arg pointer. > >> > > > > Since you have fixed the function format of thread B as void > *(*run_b)(void > > *) in tst_fzsync_pair_reset(), which means we have no need to take care > of > > the function arg pointer anymore. > > I think the function pointer signature would be 'void *(*run_b)(void)' > not 'void *(*run_b)(void *)'. > Hmm, but at least we should respect the pthread_create()? It requires the function prototype is ''void *(*func)(void *)'. int pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*start_routine) (void *), void *arg); We could unuse the arg in thread B, but declare the function prototype with parameter "void *" is no harm. > I doubt any test would need the arg though, because we only use one > thread and can store parameters in global variables. So you could remove > it and update the tests. > > The user might need that arg if they are starting many threads, but for > now we don't have explicit support for that in the library. > Maybe we just need to note that in the lib document. > > > > So just like what I did in V2, the wrapper function could steal the real > > run_b address from pthread_create(..., wrap_run_b, run_b) parameter. > > > -- > Thank you, > Richard. >
Hello, Li Wang <liwang@redhat.com> writes: > Richard Palethorpe <rpalethorpe@suse.de> wrote: > >>> must wrap the user supplied function. You can create a function which >> >> accepts a pointer to some contiguous memory containing the user supplied >> >> function >> >> pointer and the user supplied arg pointer. >> >> >> > >> > Since you have fixed the function format of thread B as void >> *(*run_b)(void >> > *) in tst_fzsync_pair_reset(), which means we have no need to take care >> of >> > the function arg pointer anymore. >> >> I think the function pointer signature would be 'void *(*run_b)(void)' >> not 'void *(*run_b)(void *)'. >> > > Hmm, but at least we should respect the pthread_create()? It requires the > function prototype is ''void *(*func)(void *)'. > > int pthread_create(pthread_t *thread, const pthread_attr_t *attr, > void *(*start_routine) (void *), void *arg); > > We could unuse the arg in thread B, but declare the function prototype with > parameter "void *" is no harm. I'm not sure what you are saying. However you could do something like this (I haven't tested it): struct tst_fzsync_run_thread { void *(*run)(void *); void *arg; }; static void tst_fzsync_thread_wrapper(void *arg) { struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread *)arg; pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS); pthread_setcancelstate(PTHREAD_CANCEL_ENABLE); t.run(t.arg); } static void tst_fzsync_pair_reset(..., struct tst_fzsync_run_thread *run_b) { ... if (run_b) SAFE_PTHREAD_fCREATE(..., tst_fzsync_thread_wrapper, (void *)run_b); ... } Note that in any case you can't reliably cast a function pointer to a void pointer without some magic. I am guessing wrapping it in a struct is the clearest way to do it. You can remove the arg altogether, but I kept it because we have a struct anyway to wrap the function pointer. > > >> I doubt any test would need the arg though, because we only use one >> thread and can store parameters in global variables. So you could remove >> it and update the tests. >> >> The user might need that arg if they are starting many threads, but for >> now we don't have explicit support for that in the library. >> > > Maybe we just need to note that in the lib document. > > >> > >> > So just like what I did in V2, the wrapper function could steal the real >> > run_b address from pthread_create(..., wrap_run_b, run_b) parameter. >> >> >> -- >> Thank you, >> Richard. >> -- Thank you, Richard.
On Wed, Sep 25, 2019 at 8:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > ... > I'm not sure what you are saying. However you could do something like > this (I haven't tested it): > I misunderstood your words in the last mail, sorry about that. > > struct tst_fzsync_run_thread > { > void *(*run)(void *); > void *arg; > }; > > static void tst_fzsync_thread_wrapper(void *arg) > { > struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread > *)arg; > > pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS); > pthread_setcancelstate(PTHREAD_CANCEL_ENABLE); > > t.run(t.arg); > } > > static void tst_fzsync_pair_reset(..., struct tst_fzsync_run_thread *run_b) > I'd like to keep the tst_fzync_pair_reset() API no change to user. The patch v4.1 like below, is there any improper place in usage? @@ -218,12 +219,36 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) { if (pair->thread_b) { - tst_atomic_store(1, &pair->exit); + /* Revoke thread B if parent hits accidental break */ + if (!pair->exit) { + tst_atomic_store(1, &pair->exit); + usleep(100000); + pthread_cancel(pair->thread_b); + } SAFE_PTHREAD_JOIN(pair->thread_b, NULL); pair->thread_b = 0; } } +/** To store the run_b pointer and pass to tst_fzsync_thread_wrapper */ +struct tst_fzsync_run_thread { + void *(*func)(void *); + void *arg; +}; + +/** + * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel + * at the start of the thread B. + */ +static void *tst_fzsync_thread_wrapper(void *run_thread) +{ + struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread *)run_thread; + + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); + return t.func(t.arg); +} + /** * Zero some stat fields * @@ -271,8 +296,10 @@ static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, pair->a_cntr = 0; pair->b_cntr = 0; pair->exit = 0; - if (run_b) - SAFE_PTHREAD_CREATE(&pair->thread_b, 0, run_b, 0); + if (run_b) { + struct tst_fzsync_run_thread wrap_run_b = {.func = run_b, .arg = NULL}; + SAFE_PTHREAD_CREATE(&pair->thread_b, 0, tst_fzsync_thread_wrapper, &wrap_run_b); + } pair->exec_time_start = (float)tst_timeout_remaining(); } > Note that in any case you can't reliably cast a function pointer to a > void pointer without some magic. I am guessing wrapping it in a struct > is the clearest way to do it. > Good to know this, I searched on google and confirmed that the C standard does not allow to cast function pointer to void*, thanks! > > You can remove the arg altogether, but I kept it because we have a > struct anyway to wrap the function pointer. > Yes, to keep it make the wrapper struct is clear to understand.
Li Wang <liwang@redhat.com> writes: > On Wed, Sep 25, 2019 at 8:13 PM Richard Palethorpe <rpalethorpe@suse.de> > wrote: > >> ... >> I'm not sure what you are saying. However you could do something like >> this (I haven't tested it): >> > > I misunderstood your words in the last mail, sorry about that. No problem. This is also my fault. > > >> >> struct tst_fzsync_run_thread >> { >> void *(*run)(void *); >> void *arg; >> }; >> >> static void tst_fzsync_thread_wrapper(void *arg) >> { >> struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread >> *)arg; >> >> pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS); >> pthread_setcancelstate(PTHREAD_CANCEL_ENABLE); >> >> t.run(t.arg); >> } >> >> static void tst_fzsync_pair_reset(..., struct tst_fzsync_run_thread *run_b) >> > > I'd like to keep the tst_fzync_pair_reset() API no change to user. > > The patch v4.1 like below, is there any improper place in usage? LGTM! > > @@ -218,12 +219,36 @@ static void tst_fzsync_pair_init(struct > tst_fzsync_pair *pair) > static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) > { > if (pair->thread_b) { > - tst_atomic_store(1, &pair->exit); > + /* Revoke thread B if parent hits accidental break */ > + if (!pair->exit) { > + tst_atomic_store(1, &pair->exit); > + usleep(100000); > + pthread_cancel(pair->thread_b); > + } > SAFE_PTHREAD_JOIN(pair->thread_b, NULL); > pair->thread_b = 0; > } > } > > +/** To store the run_b pointer and pass to tst_fzsync_thread_wrapper */ > +struct tst_fzsync_run_thread { > + void *(*func)(void *); > + void *arg; > +}; > + > +/** > + * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel > + * at the start of the thread B. > + */ > +static void *tst_fzsync_thread_wrapper(void *run_thread) > +{ > + struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread > *)run_thread; > + > + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); > + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); > + return t.func(t.arg); > +} > + > /** > * Zero some stat fields > * > @@ -271,8 +296,10 @@ static void tst_fzsync_pair_reset(struct > tst_fzsync_pair *pair, > pair->a_cntr = 0; > pair->b_cntr = 0; > pair->exit = 0; > - if (run_b) > - SAFE_PTHREAD_CREATE(&pair->thread_b, 0, run_b, 0); > + if (run_b) { > + struct tst_fzsync_run_thread wrap_run_b = {.func = run_b, > .arg = NULL}; > + SAFE_PTHREAD_CREATE(&pair->thread_b, 0, > tst_fzsync_thread_wrapper, &wrap_run_b); > + } > > pair->exec_time_start = (float)tst_timeout_remaining(); > } > > >> Note that in any case you can't reliably cast a function pointer to a >> void pointer without some magic. I am guessing wrapping it in a struct >> is the clearest way to do it. >> > > Good to know this, I searched on google and confirmed that the C standard > does not allow to cast function pointer to void*, thanks! > > >> >> You can remove the arg altogether, but I kept it because we have a >> struct anyway to wrap the function pointer. >> > > Yes, to keep it make the wrapper struct is clear to understand.
diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h index f9a1947c7..152f779cb 100644 --- a/include/tst_fuzzy_sync.h +++ b/include/tst_fuzzy_sync.h @@ -63,6 +63,7 @@ #include <time.h> #include <math.h> #include <stdlib.h> +#include <pthread.h> #include "tst_atomic.h" #include "tst_timer.h" #include "tst_safe_pthread.h" @@ -218,9 +219,13 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) { if (pair->thread_b) { - tst_atomic_store(1, &pair->exit); - SAFE_PTHREAD_JOIN(pair->thread_b, NULL); - pair->thread_b = 0; + if (pair->exit == 1) { + SAFE_PTHREAD_JOIN(pair->thread_b, NULL); + pair->thread_b = 0; + } else { + pthread_cancel(pair->thread_b); + pair->thread_b = 0; + } } } @@ -271,8 +276,11 @@ static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, pair->a_cntr = 0; pair->b_cntr = 0; pair->exit = 0; - if (run_b) + if (run_b) { + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL); SAFE_PTHREAD_CREATE(&pair->thread_b, 0, run_b, 0); + } pair->exec_time_start = (float)tst_timeout_remaining(); }
We shouldn't rely entirely on the pair->exit flag in tst_fzsync_pair_cleanup() since there is possible to call tst_brk() at anyplace of thread_a, that will lead to timeout eventually because of thread_b(tst_fzsync_wait_b) fall into an infinite(no explicit condition to exit) loop. Thread_a path trace: tst_brk() cleanup() tst_fzsync_pair_cleanup() SAFE_PTHREAD_JOIN(pair->thread_b, NULL) #Or pthread_cancel(pair->thread_b) We fix the problem via a way to kill thread_b with pthread_cancel. Work-around: [commit 2e74d996: Check recvmmsg exists before entering fuzzy loop] Signed-off-by: Li Wang <liwang@redhat.com> Cc: Richard Palethorpe <rpalethorpe@suse.com> Cc: Cyril Hrubis <chrubis@suse.cz> --- Notes: Patch V2: http://lists.linux.it/pipermail/ltp/2019-January/010489.html Patch V1: http://lists.linux.it/pipermail/ltp/2019-January/010438.html include/tst_fuzzy_sync.h | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)