| Message ID | 6c5b161bc3bcf753cbda92954ca3f47cb268c68f.1663665637.git.jstancek@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] syscalls/futex_waitv0[23]: replace TST_THREAD_STATE_WAIT with repeated wake | expand |
Hi! On 9/20/22 11:21, Jan Stancek wrote: > TST_THREAD_STATE_WAIT isn't reliable to tell that it's safe to > call futex_wake(). futex_wake() can be called prematurely and > return 0, which leaves other thread timing out. > > Replace it with repeated futex_wake() until it fails or wakes at least 1 waiter. > Also extend timeout to 5 seconds to avoid false positives from systems with > high steal time (e.g. overloaded s390x host). > > For futex_waitv03 this replaces while loop with TST_RETRY_FUNC. > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > --- > .../kernel/syscalls/futex/futex_waitv02.c | 21 ++++++------------- > .../kernel/syscalls/futex/futex_waitv03.c | 12 +++-------- > testcases/kernel/syscalls/futex/futextest.h | 15 +++++++++++++ > 3 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/testcases/kernel/syscalls/futex/futex_waitv02.c b/testcases/kernel/syscalls/futex/futex_waitv02.c > index 0a0e2b62075c..ccea5eb5e745 100644 > --- a/testcases/kernel/syscalls/futex/futex_waitv02.c > +++ b/testcases/kernel/syscalls/futex/futex_waitv02.c > @@ -50,19 +50,13 @@ static void setup(void) > } > } > > -static void *threaded(void *arg) > +static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) > { > struct futex_test_variants tv = futex_variant(); > - int tid = *(int *)arg; > > - TST_THREAD_STATE_WAIT(tid, 'S', 0); > - > - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > - 1, FUTEX_PRIVATE_FLAG)); > - if (TST_RET < 0) { > - tst_brk(TBROK | TTERRNO, > - "futex_wake private returned: %ld", TST_RET); > - } > + TST_RETRY_FUNC(futex_wake(tv.fntype, > + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > + 1, FUTEX_PRIVATE_FLAG), futex_waked_someone); Correct way of using TST_RETRY_FUNC is the following: ret = TST_RETRY_FUNC(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, 1, FUTEX_PRIVATE_FLAG), TST_RETVAL_GE0); if (ret < 0) tst_brk(TBROK | TTERRNO, "futex_wake private returned: %ld", ret); > > return NULL; > } > @@ -70,16 +64,13 @@ static void *threaded(void *arg) > static void run(void) > { > struct timespec to; > - int tid; > pthread_t t; > > - tid = tst_syscall(__NR_gettid); > - > - SAFE_PTHREAD_CREATE(&t, NULL, threaded, (void *)&tid); > + SAFE_PTHREAD_CREATE(&t, NULL, threaded, NULL); > > /* setting absolute timeout for futex2 */ > SAFE_CLOCK_GETTIME(CLOCK_MONOTONIC, &to); > - to.tv_sec++; > + to.tv_sec += 5; > > TEST(futex_waitv(waitv, numfutex, 0, &to, CLOCK_MONOTONIC)); > if (TST_RET < 0) { > diff --git a/testcases/kernel/syscalls/futex/futex_waitv03.c b/testcases/kernel/syscalls/futex/futex_waitv03.c > index ee79728474ee..c674f52d8d4c 100644 > --- a/testcases/kernel/syscalls/futex/futex_waitv03.c > +++ b/testcases/kernel/syscalls/futex/futex_waitv03.c > @@ -74,15 +74,9 @@ static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) > { > struct futex_test_variants tv = futex_variant(); > > - do { > - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > - 1, 0)); > - if (TST_RET < 0) { > - tst_brk(TBROK | TTERRNO, > - "futex_wake private returned: %ld", TST_RET); > - } > - usleep(1000); > - } while (TST_RET < 1); > + TST_RETRY_FUNC(futex_wake(tv.fntype, > + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > + 1, 0), futex_waked_someone); > > return NULL; > } > diff --git a/testcases/kernel/syscalls/futex/futextest.h b/testcases/kernel/syscalls/futex/futextest.h > index fd10885f3205..515b5102d4fc 100644 > --- a/testcases/kernel/syscalls/futex/futextest.h > +++ b/testcases/kernel/syscalls/futex/futextest.h > @@ -277,4 +277,19 @@ futex_set(futex_t *uaddr, u_int32_t newval) > return newval; > } > > +/** > + * futex_waked_someone() - ECHCK func for TST_RETRY_FUNC > + * @ret: return value of futex_wake > + * > + * Return value drives TST_RETRY_FUNC macro. > + */ > +static inline int > +futex_waked_someone(int ret) > +{ > + if (ret < 0) > + tst_brk(TBROK | TERRNO, "futex_wake returned: %d", ret); > + > + return ret; > +} > + > #endif /* _FUTEXTEST_H */ -- Regards, Andrea Cervesato
On Tue, Sep 20, 2022 at 11:55 AM Andrea Cervesato <andrea.cervesato@suse.com> wrote: > > Hi! > > On 9/20/22 11:21, Jan Stancek wrote: > > TST_THREAD_STATE_WAIT isn't reliable to tell that it's safe to > > call futex_wake(). futex_wake() can be called prematurely and > > return 0, which leaves other thread timing out. > > > > Replace it with repeated futex_wake() until it fails or wakes at least 1 waiter. > > Also extend timeout to 5 seconds to avoid false positives from systems with > > high steal time (e.g. overloaded s390x host). > > > > For futex_waitv03 this replaces while loop with TST_RETRY_FUNC. > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > --- > > .../kernel/syscalls/futex/futex_waitv02.c | 21 ++++++------------- > > .../kernel/syscalls/futex/futex_waitv03.c | 12 +++-------- > > testcases/kernel/syscalls/futex/futextest.h | 15 +++++++++++++ > > 3 files changed, 24 insertions(+), 24 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/futex/futex_waitv02.c b/testcases/kernel/syscalls/futex/futex_waitv02.c > > index 0a0e2b62075c..ccea5eb5e745 100644 > > --- a/testcases/kernel/syscalls/futex/futex_waitv02.c > > +++ b/testcases/kernel/syscalls/futex/futex_waitv02.c > > @@ -50,19 +50,13 @@ static void setup(void) > > } > > } > > > > -static void *threaded(void *arg) > > +static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) > > { > > struct futex_test_variants tv = futex_variant(); > > - int tid = *(int *)arg; > > > > - TST_THREAD_STATE_WAIT(tid, 'S', 0); > > - > > - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > > - 1, FUTEX_PRIVATE_FLAG)); > > - if (TST_RET < 0) { > > - tst_brk(TBROK | TTERRNO, > > - "futex_wake private returned: %ld", TST_RET); > > - } > > + TST_RETRY_FUNC(futex_wake(tv.fntype, > > + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > > + 1, FUTEX_PRIVATE_FLAG), futex_waked_someone); > > Correct way of using TST_RETRY_FUNC is the following: > > ret = TST_RETRY_FUNC(futex_wake(tv.fntype, (void > *)(uintptr_t)waitv[numfutex - 1].uaddr, 1, FUTEX_PRIVATE_FLAG), > TST_RETVAL_GE0); > > if (ret < 0) > tst_brk(TBROK | TTERRNO, "futex_wake private returned: %ld", ret); This has couple problems: TST_RETVAL_GE0 aborts retry on futex_wake returning 0. It won't report a failure (-1), followed by successful call later. And if the failure (-1) is persistent, it would waste time retrying. > > > > > return NULL; > > } > > @@ -70,16 +64,13 @@ static void *threaded(void *arg) > > static void run(void) > > { > > struct timespec to; > > - int tid; > > pthread_t t; > > > > - tid = tst_syscall(__NR_gettid); > > - > > - SAFE_PTHREAD_CREATE(&t, NULL, threaded, (void *)&tid); > > + SAFE_PTHREAD_CREATE(&t, NULL, threaded, NULL); > > > > /* setting absolute timeout for futex2 */ > > SAFE_CLOCK_GETTIME(CLOCK_MONOTONIC, &to); > > - to.tv_sec++; > > + to.tv_sec += 5; > > > > TEST(futex_waitv(waitv, numfutex, 0, &to, CLOCK_MONOTONIC)); > > if (TST_RET < 0) { > > diff --git a/testcases/kernel/syscalls/futex/futex_waitv03.c b/testcases/kernel/syscalls/futex/futex_waitv03.c > > index ee79728474ee..c674f52d8d4c 100644 > > --- a/testcases/kernel/syscalls/futex/futex_waitv03.c > > +++ b/testcases/kernel/syscalls/futex/futex_waitv03.c > > @@ -74,15 +74,9 @@ static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) > > { > > struct futex_test_variants tv = futex_variant(); > > > > - do { > > - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > > - 1, 0)); > > - if (TST_RET < 0) { > > - tst_brk(TBROK | TTERRNO, > > - "futex_wake private returned: %ld", TST_RET); > > - } > > - usleep(1000); > > - } while (TST_RET < 1); > > + TST_RETRY_FUNC(futex_wake(tv.fntype, > > + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, > > + 1, 0), futex_waked_someone); > > > > return NULL; > > } > > diff --git a/testcases/kernel/syscalls/futex/futextest.h b/testcases/kernel/syscalls/futex/futextest.h > > index fd10885f3205..515b5102d4fc 100644 > > --- a/testcases/kernel/syscalls/futex/futextest.h > > +++ b/testcases/kernel/syscalls/futex/futextest.h > > @@ -277,4 +277,19 @@ futex_set(futex_t *uaddr, u_int32_t newval) > > return newval; > > } > > > > +/** > > + * futex_waked_someone() - ECHCK func for TST_RETRY_FUNC > > + * @ret: return value of futex_wake > > + * > > + * Return value drives TST_RETRY_FUNC macro. > > + */ > > +static inline int > > +futex_waked_someone(int ret) > > +{ > > + if (ret < 0) > > + tst_brk(TBROK | TERRNO, "futex_wake returned: %d", ret); > > + > > + return ret; > > +} > > + > > #endif /* _FUTEXTEST_H */ > > -- > > Regards, > Andrea Cervesato >
On 9/20/22 14:44, Jan Stancek wrote: > On Tue, Sep 20, 2022 at 11:55 AM Andrea Cervesato > <andrea.cervesato@suse.com> wrote: >> Hi! >> >> On 9/20/22 11:21, Jan Stancek wrote: >>> TST_THREAD_STATE_WAIT isn't reliable to tell that it's safe to >>> call futex_wake(). futex_wake() can be called prematurely and >>> return 0, which leaves other thread timing out. >>> >>> Replace it with repeated futex_wake() until it fails or wakes at least 1 waiter. >>> Also extend timeout to 5 seconds to avoid false positives from systems with >>> high steal time (e.g. overloaded s390x host). >>> >>> For futex_waitv03 this replaces while loop with TST_RETRY_FUNC. >>> >>> Signed-off-by: Jan Stancek <jstancek@redhat.com> >>> --- >>> .../kernel/syscalls/futex/futex_waitv02.c | 21 ++++++------------- >>> .../kernel/syscalls/futex/futex_waitv03.c | 12 +++-------- >>> testcases/kernel/syscalls/futex/futextest.h | 15 +++++++++++++ >>> 3 files changed, 24 insertions(+), 24 deletions(-) >>> >>> diff --git a/testcases/kernel/syscalls/futex/futex_waitv02.c b/testcases/kernel/syscalls/futex/futex_waitv02.c >>> index 0a0e2b62075c..ccea5eb5e745 100644 >>> --- a/testcases/kernel/syscalls/futex/futex_waitv02.c >>> +++ b/testcases/kernel/syscalls/futex/futex_waitv02.c >>> @@ -50,19 +50,13 @@ static void setup(void) >>> } >>> } >>> >>> -static void *threaded(void *arg) >>> +static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) >>> { >>> struct futex_test_variants tv = futex_variant(); >>> - int tid = *(int *)arg; >>> >>> - TST_THREAD_STATE_WAIT(tid, 'S', 0); >>> - >>> - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, >>> - 1, FUTEX_PRIVATE_FLAG)); >>> - if (TST_RET < 0) { >>> - tst_brk(TBROK | TTERRNO, >>> - "futex_wake private returned: %ld", TST_RET); >>> - } >>> + TST_RETRY_FUNC(futex_wake(tv.fntype, >>> + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, >>> + 1, FUTEX_PRIVATE_FLAG), futex_waked_someone); >> Correct way of using TST_RETRY_FUNC is the following: >> >> ret = TST_RETRY_FUNC(futex_wake(tv.fntype, (void >> *)(uintptr_t)waitv[numfutex - 1].uaddr, 1, FUTEX_PRIVATE_FLAG), >> TST_RETVAL_GE0); >> >> if (ret < 0) >> tst_brk(TBROK | TTERRNO, "futex_wake private returned: %ld", ret); > This has couple problems: > TST_RETVAL_GE0 aborts retry on futex_wake returning 0. > It won't report a failure (-1), followed by successful call later. > And if the failure (-1) is persistent, it would waste time retrying. I see your point. Ok, so LGTM even without TST_RETRY_FUNC, so we can allign to futex_waitv03 test. Acked-by: Andrea Cervesato <andrea.cervesato@suse.de> >>> return NULL; >>> } >>> @@ -70,16 +64,13 @@ static void *threaded(void *arg) >>> static void run(void) >>> { >>> struct timespec to; >>> - int tid; >>> pthread_t t; >>> >>> - tid = tst_syscall(__NR_gettid); >>> - >>> - SAFE_PTHREAD_CREATE(&t, NULL, threaded, (void *)&tid); >>> + SAFE_PTHREAD_CREATE(&t, NULL, threaded, NULL); >>> >>> /* setting absolute timeout for futex2 */ >>> SAFE_CLOCK_GETTIME(CLOCK_MONOTONIC, &to); >>> - to.tv_sec++; >>> + to.tv_sec += 5; >>> >>> TEST(futex_waitv(waitv, numfutex, 0, &to, CLOCK_MONOTONIC)); >>> if (TST_RET < 0) { >>> diff --git a/testcases/kernel/syscalls/futex/futex_waitv03.c b/testcases/kernel/syscalls/futex/futex_waitv03.c >>> index ee79728474ee..c674f52d8d4c 100644 >>> --- a/testcases/kernel/syscalls/futex/futex_waitv03.c >>> +++ b/testcases/kernel/syscalls/futex/futex_waitv03.c >>> @@ -74,15 +74,9 @@ static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) >>> { >>> struct futex_test_variants tv = futex_variant(); >>> >>> - do { >>> - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, >>> - 1, 0)); >>> - if (TST_RET < 0) { >>> - tst_brk(TBROK | TTERRNO, >>> - "futex_wake private returned: %ld", TST_RET); >>> - } >>> - usleep(1000); >>> - } while (TST_RET < 1); >>> + TST_RETRY_FUNC(futex_wake(tv.fntype, >>> + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, >>> + 1, 0), futex_waked_someone); >>> >>> return NULL; >>> } >>> diff --git a/testcases/kernel/syscalls/futex/futextest.h b/testcases/kernel/syscalls/futex/futextest.h >>> index fd10885f3205..515b5102d4fc 100644 >>> --- a/testcases/kernel/syscalls/futex/futextest.h >>> +++ b/testcases/kernel/syscalls/futex/futextest.h >>> @@ -277,4 +277,19 @@ futex_set(futex_t *uaddr, u_int32_t newval) >>> return newval; >>> } >>> >>> +/** >>> + * futex_waked_someone() - ECHCK func for TST_RETRY_FUNC >>> + * @ret: return value of futex_wake >>> + * >>> + * Return value drives TST_RETRY_FUNC macro. >>> + */ >>> +static inline int >>> +futex_waked_someone(int ret) >>> +{ >>> + if (ret < 0) >>> + tst_brk(TBROK | TERRNO, "futex_wake returned: %d", ret); >>> + >>> + return ret; >>> +} >>> + >>> #endif /* _FUTEXTEST_H */ >> -- >> >> Regards, >> Andrea Cervesato >> -- Regards, Andrea Cervesato
Hi Jan,
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
On Tue, Sep 20, 2022 at 11:09 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Jan, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> Thanks, any concerns pushing this before release? It's race, but not very frequent one, so it could wait. > > Kind regards, > Petr >
Hi! > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Thanks, any concerns pushing this before release? It's race, but not > very frequent one, so it could wait. The v2 looks good to me as well. Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
On Thu, Sep 22, 2022 at 11:56 AM Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > > > Thanks, any concerns pushing this before release? It's race, but not > > very frequent one, so it could wait. > > The v2 looks good to me as well. > > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> Pushed.
diff --git a/testcases/kernel/syscalls/futex/futex_waitv02.c b/testcases/kernel/syscalls/futex/futex_waitv02.c index 0a0e2b62075c..ccea5eb5e745 100644 --- a/testcases/kernel/syscalls/futex/futex_waitv02.c +++ b/testcases/kernel/syscalls/futex/futex_waitv02.c @@ -50,19 +50,13 @@ static void setup(void) } } -static void *threaded(void *arg) +static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) { struct futex_test_variants tv = futex_variant(); - int tid = *(int *)arg; - TST_THREAD_STATE_WAIT(tid, 'S', 0); - - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, - 1, FUTEX_PRIVATE_FLAG)); - if (TST_RET < 0) { - tst_brk(TBROK | TTERRNO, - "futex_wake private returned: %ld", TST_RET); - } + TST_RETRY_FUNC(futex_wake(tv.fntype, + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, + 1, FUTEX_PRIVATE_FLAG), futex_waked_someone); return NULL; } @@ -70,16 +64,13 @@ static void *threaded(void *arg) static void run(void) { struct timespec to; - int tid; pthread_t t; - tid = tst_syscall(__NR_gettid); - - SAFE_PTHREAD_CREATE(&t, NULL, threaded, (void *)&tid); + SAFE_PTHREAD_CREATE(&t, NULL, threaded, NULL); /* setting absolute timeout for futex2 */ SAFE_CLOCK_GETTIME(CLOCK_MONOTONIC, &to); - to.tv_sec++; + to.tv_sec += 5; TEST(futex_waitv(waitv, numfutex, 0, &to, CLOCK_MONOTONIC)); if (TST_RET < 0) { diff --git a/testcases/kernel/syscalls/futex/futex_waitv03.c b/testcases/kernel/syscalls/futex/futex_waitv03.c index ee79728474ee..c674f52d8d4c 100644 --- a/testcases/kernel/syscalls/futex/futex_waitv03.c +++ b/testcases/kernel/syscalls/futex/futex_waitv03.c @@ -74,15 +74,9 @@ static void *threaded(LTP_ATTRIBUTE_UNUSED void *arg) { struct futex_test_variants tv = futex_variant(); - do { - TEST(futex_wake(tv.fntype, (void *)(uintptr_t)waitv[numfutex - 1].uaddr, - 1, 0)); - if (TST_RET < 0) { - tst_brk(TBROK | TTERRNO, - "futex_wake private returned: %ld", TST_RET); - } - usleep(1000); - } while (TST_RET < 1); + TST_RETRY_FUNC(futex_wake(tv.fntype, + (void *)(uintptr_t)waitv[numfutex - 1].uaddr, + 1, 0), futex_waked_someone); return NULL; } diff --git a/testcases/kernel/syscalls/futex/futextest.h b/testcases/kernel/syscalls/futex/futextest.h index fd10885f3205..515b5102d4fc 100644 --- a/testcases/kernel/syscalls/futex/futextest.h +++ b/testcases/kernel/syscalls/futex/futextest.h @@ -277,4 +277,19 @@ futex_set(futex_t *uaddr, u_int32_t newval) return newval; } +/** + * futex_waked_someone() - ECHCK func for TST_RETRY_FUNC + * @ret: return value of futex_wake + * + * Return value drives TST_RETRY_FUNC macro. + */ +static inline int +futex_waked_someone(int ret) +{ + if (ret < 0) + tst_brk(TBROK | TERRNO, "futex_wake returned: %d", ret); + + return ret; +} + #endif /* _FUTEXTEST_H */
TST_THREAD_STATE_WAIT isn't reliable to tell that it's safe to call futex_wake(). futex_wake() can be called prematurely and return 0, which leaves other thread timing out. Replace it with repeated futex_wake() until it fails or wakes at least 1 waiter. Also extend timeout to 5 seconds to avoid false positives from systems with high steal time (e.g. overloaded s390x host). For futex_waitv03 this replaces while loop with TST_RETRY_FUNC. Signed-off-by: Jan Stancek <jstancek@redhat.com> --- .../kernel/syscalls/futex/futex_waitv02.c | 21 ++++++------------- .../kernel/syscalls/futex/futex_waitv03.c | 12 +++-------- testcases/kernel/syscalls/futex/futextest.h | 15 +++++++++++++ 3 files changed, 24 insertions(+), 24 deletions(-)