selftests: membarrier: fix test by checking supported commands
diff mbox series

Message ID 20180730160543.19056-1-rafael.tinoco@linaro.org
State New
Headers show
Series
  • selftests: membarrier: fix test by checking supported commands
Related show

Commit Message

Rafael David Tinoco July 30, 2018, 4:05 p.m. UTC
Makes membarrier_test compatible with older kernels (LTS) by checking if
the membarrier features exist before running the tests.

Link: https://bugs.linaro.org/show_bug.cgi?id=3771
Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
Cc: <stable@vger.kernel.org> #v4.17
---
 .../selftests/membarrier/membarrier_test.c    | 69 +++++++++++--------
 1 file changed, 41 insertions(+), 28 deletions(-)

Comments

Mathieu Desnoyers July 30, 2018, 4:13 p.m. UTC | #1
----- On Jul 30, 2018, at 12:05 PM, Rafael David Tinoco rafael.tinoco@linaro.org wrote:

> Makes membarrier_test compatible with older kernels (LTS) by checking if
> the membarrier features exist before running the tests.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
> Cc: <stable@vger.kernel.org> #v4.17

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks!

Mathieu

> ---
> .../selftests/membarrier/membarrier_test.c    | 69 +++++++++++--------
> 1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c
> b/tools/testing/selftests/membarrier/membarrier_test.c
> index 6793f8ecc8e7..b96caa096e2f 100644
> --- a/tools/testing/selftests/membarrier/membarrier_test.c
> +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> @@ -225,7 +225,14 @@ static int test_membarrier_global_expedited_success(void)
> 
> static int test_membarrier(void)
> {
> -	int status;
> +	int supported, status;
> +
> +	supported = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
> +	if (supported < 0) {
> +		ksft_test_result_fail(
> +			"sys_membarrier() failed to query supported cmds\n");
> +		return supported;
> +	}
> 
> 	status = test_membarrier_cmd_fail();
> 	if (status)
> @@ -236,21 +243,22 @@ static int test_membarrier(void)
> 	status = test_membarrier_global_success();
> 	if (status)
> 		return status;
> -	status = test_membarrier_private_expedited_fail();
> -	if (status)
> -		return status;
> -	status = test_membarrier_register_private_expedited_success();
> -	if (status)
> -		return status;
> -	status = test_membarrier_private_expedited_success();
> -	if (status)
> -		return status;
> -	status = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
> -	if (status < 0) {
> -		ksft_test_result_fail("sys_membarrier() failed\n");
> -		return status;
> +
> +	/* commit 22e4ebb975822833b083533035233d128b30e98f added this feature */
> +	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED) {
> +		status = test_membarrier_private_expedited_fail();
> +		if (status)
> +			return status;
> +		status = test_membarrier_register_private_expedited_success();
> +		if (status)
> +			return status;
> +		status = test_membarrier_private_expedited_success();
> +		if (status)
> +			return status;
> 	}
> -	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
> +
> +	/* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */
> +	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
> 		status = test_membarrier_private_expedited_sync_core_fail();
> 		if (status)
> 			return status;
> @@ -261,19 +269,24 @@ static int test_membarrier(void)
> 		if (status)
> 			return status;
> 	}
> -	/*
> -	 * It is valid to send a global membarrier from a non-registered
> -	 * process.
> -	 */
> -	status = test_membarrier_global_expedited_success();
> -	if (status)
> -		return status;
> -	status = test_membarrier_register_global_expedited_success();
> -	if (status)
> -		return status;
> -	status = test_membarrier_global_expedited_success();
> -	if (status)
> -		return status;
> +
> +	/* commit c5f58bd58f432be5d92df33c5458e0bcbee3aadf added this feature */
> +	if (supported & MEMBARRIER_CMD_GLOBAL_EXPEDITED) {
> +		/*
> +		 * It is valid to send a global membarrier from a non-registered
> +		 * process.
> +		 */
> +		status = test_membarrier_global_expedited_success();
> +		if (status)
> +			return status;
> +		status = test_membarrier_register_global_expedited_success();
> +		if (status)
> +			return status;
> +		status = test_membarrier_global_expedited_success();
> +		if (status)
> +			return status;
> +	}
> +
> 	return 0;
> }
> 
> --
> 2.18.0
shuah July 30, 2018, 11:32 p.m. UTC | #2
Hi Rafael,

On 07/30/2018 10:05 AM, Rafael David Tinoco wrote:
> Makes membarrier_test compatible with older kernels (LTS) by checking if
> the membarrier features exist before running the tests.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
> Cc: <stable@vger.kernel.org> #v4.17
> ---
>  .../selftests/membarrier/membarrier_test.c    | 69 +++++++++++--------
>  1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> index 6793f8ecc8e7..b96caa096e2f 100644
> --- a/tools/testing/selftests/membarrier/membarrier_test.c
> +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> @@ -225,7 +225,14 @@ static int test_membarrier_global_expedited_success(void)
>  
>  static int test_membarrier(void)
>  {
> -	int status;
> +	int supported, status;
> +
> +	supported = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
> +	if (supported < 0) {
> +		ksft_test_result_fail(
> +			"sys_membarrier() failed to query supported cmds\n");
> +		return supported;
> +	}
>  

ksft_exit_skip() is the right interface to use here. If feature isn't supported,
it should exit skip as opposed fail.

>  	status = test_membarrier_cmd_fail();
>  	if (status)
> @@ -236,21 +243,22 @@ static int test_membarrier(void)
>  	status = test_membarrier_global_success();
>  	if (status)
>  		return status;
> -	status = test_membarrier_private_expedited_fail();
> -	if (status)
> -		return status;
> -	status = test_membarrier_register_private_expedited_success();
> -	if (status)
> -		return status;
> -	status = test_membarrier_private_expedited_success();
> -	if (status)
> -		return status;
> -	status = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
> -	if (status < 0) {
> -		ksft_test_result_fail("sys_membarrier() failed\n");
> -		return status;
> +
> +	/* commit 22e4ebb975822833b083533035233d128b30e98f added this feature */
> +	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED) {
> +		status = test_membarrier_private_expedited_fail();
> +		if (status)
> +			return status;
> +		status = test_membarrier_register_private_expedited_success();
> +		if (status)
> +			return status;
> +		status = test_membarrier_private_expedited_success();
> +		if (status)
> +			return status;
>  	}
> -	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
> +
> +	/* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */
> +	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
>  		status = test_membarrier_private_expedited_sync_core_fail();
>  		if (status)
>  			return status;
> @@ -261,19 +269,24 @@ static int test_membarrier(void)
>  		if (status)
>  			return status;
>  	}
> -	/*
> -	 * It is valid to send a global membarrier from a non-registered
> -	 * process.
> -	 */
> -	status = test_membarrier_global_expedited_success();
> -	if (status)
> -		return status;
> -	status = test_membarrier_register_global_expedited_success();
> -	if (status)
> -		return status;
> -	status = test_membarrier_global_expedited_success();
> -	if (status)
> -		return status;
> +
> +	/* commit c5f58bd58f432be5d92df33c5458e0bcbee3aadf added this feature */
> +	if (supported & MEMBARRIER_CMD_GLOBAL_EXPEDITED) {
> +		/*
> +		 * It is valid to send a global membarrier from a non-registered
> +		 * process.
> +		 */
> +		status = test_membarrier_global_expedited_success();
> +		if (status)
> +			return status;
> +		status = test_membarrier_register_global_expedited_success();
> +		if (status)
> +			return status;
> +		status = test_membarrier_global_expedited_success();
> +		if (status)
> +			return status;
> +	}
> +
>  	return 0;
>  }
>  
> 

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael David Tinoco July 31, 2018, 3:15 a.m. UTC | #3
Hello Shuah,

On Mon, Jul 30, 2018 at 05:32:30PM -0600, Shuah Khan wrote:
> Hi Rafael,
>
> On 07/30/2018 10:05 AM, Rafael David Tinoco wrote:
> > Makes membarrier_test compatible with older kernels (LTS) by checking if
> > the membarrier features exist before running the tests.
> >
> > Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
> > Cc: <stable@vger.kernel.org> #v4.17
> > ---
> >  .../selftests/membarrier/membarrier_test.c    | 69 +++++++++++--------
> >  1 file changed, 41 insertions(+), 28 deletions(-)
> >
> > diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> > index 6793f8ecc8e7..b96caa096e2f 100644
> > --- a/tools/testing/selftests/membarrier/membarrier_test.c
> > +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> > @@ -225,7 +225,14 @@ static int test_membarrier_global_expedited_success(void)
> >
> >  static int test_membarrier(void)
> >  {
> > -	int status;
> > +	int supported, status;
> > +
> > +	supported = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
> > +	if (supported < 0) {
> > +		ksft_test_result_fail(
> > +			"sys_membarrier() failed to query supported cmds\n");
> > +		return supported;
> > +	}
> >
>
> ksft_exit_skip() is the right interface to use here. If feature isn't supported,
> it should exit skip as opposed fail.
>

Not sure this is the case here. This part was just a positional change.

This check is extending an existing logic (for MEMBARRIER_CMD_PRIVATE_
EXPEDITED_SYNC_CORE tests). Calling membarrier with MEMBARRIER_CMD_QUERY
will return us MEMBARRIER_CMD_BITMASK, telling us which features are
enabled for the running kernel (thus which tests can be executed).

The query command was added in v4.3 and should (could ?) be considered a
fundament for a working test by now, I suppose, no ?

It is used to decide which further tests to run. Not receiving anything
back from this call would mean something is broken (since at least
MEMBARRIER_CMD_GLOBAL should have always existed as a membarrier
feature/command).

I think your concern is addressed in the beginning of the test.
test_membarrier_query() tests for ENOSYS and calls ksft_exit_skip() if
CONFIG_MEMBARRIER is disabled.

This part is not about checking if the test can run, but which one can.
What do you think ? Tks for reviewing!

-- Rafael D. Tinoco
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael David Tinoco Aug. 8, 2018, 2:09 p.m. UTC | #4
On Tue, Jul 31, 2018 at 12:15:37AM -0300, Rafael David Tinoco wrote:
> Hello Shuah,
> 
> On Mon, Jul 30, 2018 at 05:32:30PM -0600, Shuah Khan wrote:
> > Hi Rafael,
> >
> > On 07/30/2018 10:05 AM, Rafael David Tinoco wrote:
> > > Makes membarrier_test compatible with older kernels (LTS) by checking if
> > > the membarrier features exist before running the tests.
> > >
> > > Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org>
> > > Cc: <stable@vger.kernel.org> #v4.17
> > > ---
> > >  .../selftests/membarrier/membarrier_test.c    | 69 +++++++++++--------
> > >  1 file changed, 41 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> > > index 6793f8ecc8e7..b96caa096e2f 100644
> > > --- a/tools/testing/selftests/membarrier/membarrier_test.c
> > > +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> > > @@ -225,7 +225,14 @@ static int test_membarrier_global_expedited_success(void)
> > >
> > >  static int test_membarrier(void)
> > >  {
> > > -	int status;
> > > +	int supported, status;
> > > +
> > > +	supported = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
> > > +	if (supported < 0) {
> > > +		ksft_test_result_fail(
> > > +			"sys_membarrier() failed to query supported cmds\n");
> > > +		return supported;
> > > +	}
> > >
> >
> > ksft_exit_skip() is the right interface to use here. If feature isn't supported,
> > it should exit skip as opposed fail.
> >
> 
> Not sure this is the case here. This part was just a positional change.
> 
> This check is extending an existing logic (for MEMBARRIER_CMD_PRIVATE_
> EXPEDITED_SYNC_CORE tests). Calling membarrier with MEMBARRIER_CMD_QUERY
> will return us MEMBARRIER_CMD_BITMASK, telling us which features are
> enabled for the running kernel (thus which tests can be executed).
> 
> The query command was added in v4.3 and should (could ?) be considered a
> fundament for a working test by now, I suppose, no ?
> 
> It is used to decide which further tests to run. Not receiving anything
> back from this call would mean something is broken (since at least
> MEMBARRIER_CMD_GLOBAL should have always existed as a membarrier
> feature/command).
> 
> I think your concern is addressed in the beginning of the test.
> test_membarrier_query() tests for ENOSYS and calls ksft_exit_skip() if
> CONFIG_MEMBARRIER is disabled.
> 
> This part is not about checking if the test can run, but which one can.
> What do you think ? Tks for reviewing!

Shuah,

Never mind, I'll remove the 2nd MEMBARRIER_CMD_QUERY call, and cache the
first call results into a global status. This way, the function
test_membarrier_query() will test for availability, and initial issues
(like not having MEMBARRIER_CMD_GLOBAL), and skip or return error
approprietly like you said. No need to call it twice, just use cached
status. Tks for the review.

I'll send a v2.

Thank you
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox series

diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
index 6793f8ecc8e7..b96caa096e2f 100644
--- a/tools/testing/selftests/membarrier/membarrier_test.c
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -225,7 +225,14 @@  static int test_membarrier_global_expedited_success(void)
 
 static int test_membarrier(void)
 {
-	int status;
+	int supported, status;
+
+	supported = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
+	if (supported < 0) {
+		ksft_test_result_fail(
+			"sys_membarrier() failed to query supported cmds\n");
+		return supported;
+	}
 
 	status = test_membarrier_cmd_fail();
 	if (status)
@@ -236,21 +243,22 @@  static int test_membarrier(void)
 	status = test_membarrier_global_success();
 	if (status)
 		return status;
-	status = test_membarrier_private_expedited_fail();
-	if (status)
-		return status;
-	status = test_membarrier_register_private_expedited_success();
-	if (status)
-		return status;
-	status = test_membarrier_private_expedited_success();
-	if (status)
-		return status;
-	status = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
-	if (status < 0) {
-		ksft_test_result_fail("sys_membarrier() failed\n");
-		return status;
+
+	/* commit 22e4ebb975822833b083533035233d128b30e98f added this feature */
+	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED) {
+		status = test_membarrier_private_expedited_fail();
+		if (status)
+			return status;
+		status = test_membarrier_register_private_expedited_success();
+		if (status)
+			return status;
+		status = test_membarrier_private_expedited_success();
+		if (status)
+			return status;
 	}
-	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
+
+	/* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */
+	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
 		status = test_membarrier_private_expedited_sync_core_fail();
 		if (status)
 			return status;
@@ -261,19 +269,24 @@  static int test_membarrier(void)
 		if (status)
 			return status;
 	}
-	/*
-	 * It is valid to send a global membarrier from a non-registered
-	 * process.
-	 */
-	status = test_membarrier_global_expedited_success();
-	if (status)
-		return status;
-	status = test_membarrier_register_global_expedited_success();
-	if (status)
-		return status;
-	status = test_membarrier_global_expedited_success();
-	if (status)
-		return status;
+
+	/* commit c5f58bd58f432be5d92df33c5458e0bcbee3aadf added this feature */
+	if (supported & MEMBARRIER_CMD_GLOBAL_EXPEDITED) {
+		/*
+		 * It is valid to send a global membarrier from a non-registered
+		 * process.
+		 */
+		status = test_membarrier_global_expedited_success();
+		if (status)
+			return status;
+		status = test_membarrier_register_global_expedited_success();
+		if (status)
+			return status;
+		status = test_membarrier_global_expedited_success();
+		if (status)
+			return status;
+	}
+
 	return 0;
 }