Re: [PATCH 1/2] selftest/futex: Make the error check more precise for futex_numa_mpol

["André Almeida" <andrealmeid@igalia.com>]

Hi Waiman,

Thanks for the feedback!

Em 03/09/2025 14:53, Waiman Long escreveu:
> On 9/1/25 4:33 PM, André Almeida wrote:
>> Instead of just checking if the syscall failed as expected, check as
>> well what is the error code returned, to check if it's match the
>> expectation and it's failing in the correct error path inside the
>> kernel.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---
>> This patch is aimed for 6.18
>> ---
>>   .../futex/functional/futex_numa_mpol.c        | 36 +++++++++++--------
>>   1 file changed, 21 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/testing/selftests/futex/functional/ 
>> futex_numa_mpol.c b/tools/testing/selftests/futex/functional/ 
>> futex_numa_mpol.c
>> index 802c15c82190..c84441751235 100644
>> --- a/tools/testing/selftests/futex/functional/futex_numa_mpol.c
>> +++ b/tools/testing/selftests/futex/functional/futex_numa_mpol.c
>> @@ -77,7 +77,7 @@ static void join_max_threads(void)
>>       }
>>   }
>> -static void __test_futex(void *futex_ptr, int must_fail, unsigned int 
>> futex_flags)
>> +static void __test_futex(void *futex_ptr, int err_value, unsigned int 
>> futex_flags)
>>   {
>>       int to_wake, ret, i, need_exit = 0;
>> @@ -88,11 +88,17 @@ static void __test_futex(void *futex_ptr, int 
>> must_fail, unsigned int futex_flag
>>       do {
>>           ret = futex2_wake(futex_ptr, to_wake, futex_flags);
>> -        if (must_fail) {
>> -            if (ret < 0)
>> -                break;
>> -            ksft_exit_fail_msg("futex2_wake(%d, 0x%x) should fail, 
>> but didn't\n",
>> -                       to_wake, futex_flags);
>> +
>> +        if (err_value) {
>> +            if (ret >= 0)
>> +                ksft_exit_fail_msg("futex2_wake(%d, 0x%x) should 
>> fail, but didn't\n",
>> +                           to_wake, futex_flags);
>> +
>> +            if (errno != err_value)
>> +                ksft_exit_fail_msg("futex2_wake(%d, 0x%x) expected 
>> error was %d, but returned %d (%s)\n",
>> +                           to_wake, futex_flags, err_value, errno, 
>> strerror(errno));
>> +
>> +            break;
> 
> If (ret >= 0), the 2nd (errno != err_value) failure message will likely 
> be printed too. Should we use "else if" so that only one error message 
> will be printed?
> 
> 

ksft_exit_fail_msg() calls exit(), so the code will exit before 
executing the second failure message.

If this was a  ksft_test_result_error() call, then the message would be 
printed twice.

>>           }
>>           if (ret < 0) {
>>               ksft_exit_fail_msg("Failed futex2_wake(%d, 0x%x): %m\n",
>> @@ -106,12 +112,12 @@ static void __test_futex(void *futex_ptr, int 
>> must_fail, unsigned int futex_flag
>>       join_max_threads();
>>       for (i = 0; i < MAX_THREADS; i++) {
>> -        if (must_fail && thread_args[i].result != -1) {
>> +        if (err_value && thread_args[i].result != -1) {
>>               ksft_print_msg("Thread %d should fail but succeeded 
>> (%d)\n",
>>                          i, thread_args[i].result);
>>               need_exit = 1;
>>           }
>> -        if (!must_fail && thread_args[i].result != 0) {
>> +        if (!err_value && thread_args[i].result != 0) {
>>               ksft_print_msg("Thread %d failed (%d)\n", i, 
>> thread_args[i].result);
>>               need_exit = 1;
>>           }
>> @@ -120,14 +126,14 @@ static void __test_futex(void *futex_ptr, int 
>> must_fail, unsigned int futex_flag
>>           ksft_exit_fail_msg("Aborting due to earlier errors.\n");
>>   }
>> -static void test_futex(void *futex_ptr, int must_fail)
>> +static void test_futex(void *futex_ptr, int err_value)
>>   {
>> -    __test_futex(futex_ptr, must_fail, FUTEX2_SIZE_U32 | 
>> FUTEX_PRIVATE_FLAG | FUTEX2_NUMA);
>> +    __test_futex(futex_ptr, err_value, FUTEX2_SIZE_U32 | 
>> FUTEX_PRIVATE_FLAG | FUTEX2_NUMA);
>>   }
>> -static void test_futex_mpol(void *futex_ptr, int must_fail)
>> +static void test_futex_mpol(void *futex_ptr, int err_value)
>>   {
>> -    __test_futex(futex_ptr, must_fail, FUTEX2_SIZE_U32 | 
>> FUTEX_PRIVATE_FLAG | FUTEX2_NUMA | FUTEX2_MPOL);
>> +    __test_futex(futex_ptr, err_value, FUTEX2_SIZE_U32 | 
>> FUTEX_PRIVATE_FLAG | FUTEX2_NUMA | FUTEX2_MPOL);
>>   }
>>   static void usage(char *prog)
>> @@ -184,16 +190,16 @@ int main(int argc, char *argv[])
>>       /* FUTEX2_NUMA futex must be 8-byte aligned */
>>       ksft_print_msg("Mis-aligned futex\n");
>> -    test_futex(futex_ptr + mem_size - 4, 1);
>> +    test_futex(futex_ptr + mem_size - 4, 22);
>>       futex_numa->numa = FUTEX_NO_NODE;
>>       mprotect(futex_ptr, mem_size, PROT_READ);
>>       ksft_print_msg("Memory, RO\n");
>> -    test_futex(futex_ptr, 1);
>> +    test_futex(futex_ptr, 14);
>>       mprotect(futex_ptr, mem_size, PROT_NONE);
>>       ksft_print_msg("Memory, no access\n");
>> -    test_futex(futex_ptr, 1);
>> +    test_futex(futex_ptr, 14);
>>       mprotect(futex_ptr, mem_size, PROT_READ | PROT_WRITE);
>>       ksft_print_msg("Memory back to RW\n");
> 
> I believe it is better to use the error number mnemonic (EINVAL & 
> EFAULT) instead of 22 and 14 as argument to make the code easier to read.
> 

Good call, applied.

Thanks!
André

> Cheers,
> Longman
>