[LTP] [PATCH v2] syscalls/futex_waitv0[23]: replace TST_THREAD_STATE_WAIT with repeated wake

[Jan Stancek]

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
>