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

[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