mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Waiman Long <llong@redhat.com>
To: "André Almeida" <andrealmeid@igalia.com>,
	"Sebastian Andrzej Siewior" <bigeasy@linutronix.de>,
	linux-kernel@vger.kernel.org
Cc: Darren Hart <dvhart@infradead.org>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Ingo Molnar <mingo@redhat.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Valentin Schneider <vschneid@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	kernel-dev@igalia.com
Subject: Re: [PATCH 1/2] selftest/futex: Make the error check more precise for futex_numa_mpol
Date: Wed, 3 Sep 2025 13:53:41 -0400	[thread overview]
Message-ID: <1d6a0c1e-5fa2-4c21-b3c1-7bfb2f9dd669@redhat.com> (raw)
In-Reply-To: <20250901203327.53887-1-andrealmeid@igalia.com>

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?


>   		}
>   		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.

Cheers,
Longman


  parent reply	other threads:[~2025-09-03 17:53 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-01 20:33 André Almeida
2025-09-01 20:33 ` [PATCH 2/2] selftest/futex: Reintroduce "Memory out of range" numa_mpol's subtest André Almeida
2025-09-03 17:57   ` Waiman Long
2025-09-03 17:53 ` Waiman Long [this message]
2025-09-04 15:23   ` [PATCH 1/2] selftest/futex: Make the error check more precise for futex_numa_mpol André Almeida
2025-09-04 15:59     ` Waiman Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1d6a0c1e-5fa2-4c21-b3c1-7bfb2f9dd669@redhat.com \
    --to=llong@redhat.com \
    --cc=andrealmeid@igalia.com \
    --cc=bigeasy@linutronix.de \
    --cc=bp@alien8.de \
    --cc=dave@stgolabs.net \
    --cc=dvhart@infradead.org \
    --cc=juri.lelli@redhat.com \
    --cc=kernel-dev@igalia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=vschneid@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®