[PATCH] selftests: membarrier: fix test by checking supported commands

[PATCH] selftests: membarrier: fix test by checking supported commands

[rafael.tinoco]

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 at linaro.org>
Cc: <stable at 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;
+	}
 
 	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

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

[PATCH] selftests: membarrier: fix test by checking supported commands

[Rafael David Tinoco]

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 at linaro.org>
Cc: <stable at 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;
+	}
 
 	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

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

[PATCH] selftests: membarrier: fix test by checking supported commands

[mathieu.desnoyers]

----- On Jul 30, 2018, at 12:05 PM, Rafael David Tinoco rafael.tinoco at 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 at linaro.org>
> Cc: <stable at vger.kernel.org> #v4.17

Acked-by: Mathieu Desnoyers <mathieu.desnoyers at 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

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[Mathieu Desnoyers]

----- On Jul 30, 2018,@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 at linaro.org>
> Cc: <stable at vger.kernel.org> #v4.17

Acked-by: Mathieu Desnoyers <mathieu.desnoyers at 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

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[shuah]

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 at linaro.org>
> Cc: <stable at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[Shuah Khan]

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 at linaro.org>
> Cc: <stable at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[rafael.tinoco]

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 at linaro.org>
> > Cc: <stable at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[Rafael David Tinoco]

Hello Shuah,

On Mon, Jul 30, 2018@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 at linaro.org>
> > Cc: <stable at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[rafael.tinoco]

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 at linaro.org>
> > > Cc: <stable at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] selftests: membarrier: fix test by checking supported commands

[Rafael David Tinoco]

On Tue, Jul 31, 2018@12:15:37AM -0300, Rafael David Tinoco wrote:
> Hello Shuah,
> 
> On Mon, Jul 30, 2018@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 at linaro.org>
> > > Cc: <stable at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] selftests: membarrier: fix test by checking supported commands

[rafael.tinoco]

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 at linaro.org>
Cc: <stable at vger.kernel.org> #v4.17
---
 .../selftests/membarrier/membarrier_test.c    | 71 +++++++++++--------
 1 file changed, 40 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
index 6793f8ecc8e7..4dc263824bda 100644
--- a/tools/testing/selftests/membarrier/membarrier_test.c
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -223,7 +223,7 @@ static int test_membarrier_global_expedited_success(void)
 	return 0;
 }
 
-static int test_membarrier(void)
+static int test_membarrier(int supported)
 {
 	int status;
 
@@ -236,21 +236,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,23 +262,28 @@ 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;
 }
 
-static int test_membarrier_query(void)
+static int test_membarrier_query(int *supported)
 {
 	int flags = 0, ret;
 
@@ -297,16 +303,19 @@ static int test_membarrier_query(void)
 		ksft_exit_skip(
 			"sys_membarrier unsupported: CMD_GLOBAL not found.\n");
 
+	*supported = ret;
 	ksft_test_result_pass("sys_membarrier available\n");
 	return 0;
 }
 
 int main(int argc, char **argv)
 {
+	int supported;
+
 	ksft_print_header();
 
-	test_membarrier_query();
-	test_membarrier();
+	test_membarrier_query(&supported);
+	test_membarrier(supported);
 
 	return ksft_exit_pass();
 }
-- 
2.18.0

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

[PATCH v2] selftests: membarrier: fix test by checking supported commands

[Rafael David Tinoco]

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 at linaro.org>
Cc: <stable at vger.kernel.org> #v4.17
---
 .../selftests/membarrier/membarrier_test.c    | 71 +++++++++++--------
 1 file changed, 40 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
index 6793f8ecc8e7..4dc263824bda 100644
--- a/tools/testing/selftests/membarrier/membarrier_test.c
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -223,7 +223,7 @@ static int test_membarrier_global_expedited_success(void)
 	return 0;
 }
 
-static int test_membarrier(void)
+static int test_membarrier(int supported)
 {
 	int status;
 
@@ -236,21 +236,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,23 +262,28 @@ 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;
 }
 
-static int test_membarrier_query(void)
+static int test_membarrier_query(int *supported)
 {
 	int flags = 0, ret;
 
@@ -297,16 +303,19 @@ static int test_membarrier_query(void)
 		ksft_exit_skip(
 			"sys_membarrier unsupported: CMD_GLOBAL not found.\n");
 
+	*supported = ret;
 	ksft_test_result_pass("sys_membarrier available\n");
 	return 0;
 }
 
 int main(int argc, char **argv)
 {
+	int supported;
+
 	ksft_print_header();
 
-	test_membarrier_query();
-	test_membarrier();
+	test_membarrier_query(&supported);
+	test_membarrier(supported);
 
 	return ksft_exit_pass();
 }
-- 
2.18.0

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

[PATCH v2] selftests: membarrier: fix test by checking supported commands

[shuah]

Hi Rafael,

Thanks for the ping.

On 08/09/2018 02:21 PM, 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 at linaro.org>
> Cc: <stable at vger.kernel.org> #v4.17
> ---
>  .../selftests/membarrier/membarrier_test.c    | 71 +++++++++++--------
>  1 file changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> index 6793f8ecc8e7..4dc263824bda 100644
> --- a/tools/testing/selftests/membarrier/membarrier_test.c
> +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> @@ -223,7 +223,7 @@ static int test_membarrier_global_expedited_success(void)
>  	return 0;
>  }
>  
> -static int test_membarrier(void)
> +static int test_membarrier(int supported)
>  {
>  	int status;
>  
> @@ -236,21 +236,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 */

Get rid of this comment.

> +	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;
>  	}

This change moves several tests under this check. These should run to test
the case when MEMBARRIER_CMD_PRIVATE_EXPEDITED isn't supported. This change
reduces coverage.

> -	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
> +
> +	/* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */

Get rid of the above comment.

> +	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
>  		status = test_membarrier_private_expedited_sync_core_fail();
>  		if (status)
>  			return status;
> @@ -261,23 +262,28 @@ 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 */

Get rid of the above comment.

> +	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;
> +	}
> +

There skip handling missing here. Without this the test result reports
pass which is incorrect.

If feature isn't supported, test should report that the feature test is
skipped not passed.

What I would like to see here is a skip for each individual test not one
skip for all 3 tests.

This applies to the if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE)
case above.

When I run this test on 4.19 I see, 

TAP version 13
selftests: membarrier: membarrier_test
========================================
ok 1 sys_membarrier available
ok 2 sys membarrier invalid command test: command = -1, flags = 0, errno = 22. Failed as expected
ok 3 sys membarrier MEMBARRIER_CMD_QUERY invalid flags test: flags = 1, errno = 22. Failed as expected
ok 4 sys membarrier MEMBARRIER_CMD_GLOBAL test: flags = 0
ok 5 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED not registered failure test: flags = 0, errno = 1
ok 6 sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED test: flags = 0
ok 7 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED test: flags = 0
ok 8 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE not registered failure test: flags = 0, errno = 1
ok 9 sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE test: flags = 0
ok 10 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE test: flags = 0
ok 11 sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED test: flags = 0
ok 12 sys membarrier MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED test: flags = 0
ok 13 sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED test: flags = 0
Pass 13 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0
1..13
ok 1..1 selftests: membarrier: membarrier_test [PASS]

A total of 13 tests out of which, maybe 6 should be skips on 4.14. For example,
if MEMBARRIER_CMD_GLOBAL_EXPEDITED isn't supported, it should report that test
as a skip.

>  	return 0;
>  }
>  
> -static int test_membarrier_query(void)
> +static int test_membarrier_query(int *supported)
>  {
>  	int flags = 0, ret;
>  
> @@ -297,16 +303,19 @@ static int test_membarrier_query(void)
>  		ksft_exit_skip(
>  			"sys_membarrier unsupported: CMD_GLOBAL not found.\n");
>  
> +	*supported = ret;
>  	ksft_test_result_pass("sys_membarrier available\n");
>  	return 0;
>  }
>  
>  int main(int argc, char **argv)
>  {
> +	int supported;
> +
>  	ksft_print_header();
>  
> -	test_membarrier_query();
> -	test_membarrier();
> +	test_membarrier_query(&supported);
> +	test_membarrier(supported);
>  
>  	return ksft_exit_pass();
>  }
> 

thanks,
-- Shuah

[PATCH v2] selftests: membarrier: fix test by checking supported commands

[Shuah Khan]

Hi Rafael,

Thanks for the ping.

On 08/09/2018 02:21 PM, 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 at linaro.org>
> Cc: <stable at vger.kernel.org> #v4.17
> ---
>  .../selftests/membarrier/membarrier_test.c    | 71 +++++++++++--------
>  1 file changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> index 6793f8ecc8e7..4dc263824bda 100644
> --- a/tools/testing/selftests/membarrier/membarrier_test.c
> +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> @@ -223,7 +223,7 @@ static int test_membarrier_global_expedited_success(void)
>  	return 0;
>  }
>  
> -static int test_membarrier(void)
> +static int test_membarrier(int supported)
>  {
>  	int status;
>  
> @@ -236,21 +236,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 */

Get rid of this comment.

> +	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;
>  	}

This change moves several tests under this check. These should run to test
the case when MEMBARRIER_CMD_PRIVATE_EXPEDITED isn't supported. This change
reduces coverage.

> -	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
> +
> +	/* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */

Get rid of the above comment.

> +	if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
>  		status = test_membarrier_private_expedited_sync_core_fail();
>  		if (status)
>  			return status;
> @@ -261,23 +262,28 @@ 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 */

Get rid of the above comment.

> +	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;
> +	}
> +

There skip handling missing here. Without this the test result reports
pass which is incorrect.

If feature isn't supported, test should report that the feature test is
skipped not passed.

What I would like to see here is a skip for each individual test not one
skip for all 3 tests.

This applies to the if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE)
case above.

When I run this test on 4.19 I see, 

TAP version 13
selftests: membarrier: membarrier_test
========================================
ok 1 sys_membarrier available
ok 2 sys membarrier invalid command test: command = -1, flags = 0, errno = 22. Failed as expected
ok 3 sys membarrier MEMBARRIER_CMD_QUERY invalid flags test: flags = 1, errno = 22. Failed as expected
ok 4 sys membarrier MEMBARRIER_CMD_GLOBAL test: flags = 0
ok 5 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED not registered failure test: flags = 0, errno = 1
ok 6 sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED test: flags = 0
ok 7 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED test: flags = 0
ok 8 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE not registered failure test: flags = 0, errno = 1
ok 9 sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE test: flags = 0
ok 10 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE test: flags = 0
ok 11 sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED test: flags = 0
ok 12 sys membarrier MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED test: flags = 0
ok 13 sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED test: flags = 0
Pass 13 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0
1..13
ok 1..1 selftests: membarrier: membarrier_test [PASS]

A total of 13 tests out of which, maybe 6 should be skips on 4.14. For example,
if MEMBARRIER_CMD_GLOBAL_EXPEDITED isn't supported, it should report that test
as a skip.

>  	return 0;
>  }
>  
> -static int test_membarrier_query(void)
> +static int test_membarrier_query(int *supported)
>  {
>  	int flags = 0, ret;
>  
> @@ -297,16 +303,19 @@ static int test_membarrier_query(void)
>  		ksft_exit_skip(
>  			"sys_membarrier unsupported: CMD_GLOBAL not found.\n");
>  
> +	*supported = ret;
>  	ksft_test_result_pass("sys_membarrier available\n");
>  	return 0;
>  }
>  
>  int main(int argc, char **argv)
>  {
> +	int supported;
> +
>  	ksft_print_header();
>  
> -	test_membarrier_query();
> -	test_membarrier();
> +	test_membarrier_query(&supported);
> +	test_membarrier(supported);
>  
>  	return ksft_exit_pass();
>  }
> 

thanks,
-- Shuah

[PATCH v3] membarrier_test: work in progress

[shuah]

Hi Rafael,

Thanks for the patch. Comments below.

On 09/02/2018 08:12 PM, Rafael David Tinoco wrote:
> Shuah,
> 
> This is a draft only. I will remove summary from the top, run checkers,
> etc. Im thinking in replacing membarrier_test entirely with this code
> (compatible to existing one). Right now, this code:
> 
>  - allows each test to succeed, fail or be skipped independently
>  - allows each test to be tested even when not supported (force option)
>  - considers false negatives and false positives on every case
>  - can be extended easily
> 
> Right now, just to show as an example, it gives us:
> 
> TAP version 13
> ok 1 sys_membarrier(): cmd_query succeeded.
> ok 2 sys_membarrier(): bad_cmd failed as expected.
> ok 3 sys_membarrier(): cmd_with_flags_set failed as expected.
> ok 4 sys_membarrier(): cmd_global succeeded.
> Pass 4 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0
> 1..4
> 
> Are you okay with such move ? Only big TODO here is adding all covered
> tests in the test array (easy move), testing all combinations with all
> supported kernel versions (lab already ready) and suggesting it to you,
> replacing membarrier_test.c.
> 
> PS: This is pretty close to how a LTP test would be, using their new
> API, but, since it addresses your concerns and seems like a
> simple/clean, code, I decided to suggest it as a replacement here (and
> it also fixes the issue with this test and LTS kernels).
> ---
>  tools/testing/selftests/membarrier/Makefile   |   2 +-
>  .../selftests/membarrier/membarrier_test2.c   | 180 ++++++++++++++++++
>  2 files changed, 181 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/membarrier/membarrier_test2.c
> 
> diff --git a/tools/testing/selftests/membarrier/Makefile b/tools/testing/selftests/membarrier/Makefile
> index 02845532b059..3d44d4cd3a9d 100644
> --- a/tools/testing/selftests/membarrier/Makefile
> +++ b/tools/testing/selftests/membarrier/Makefile
> @@ -1,6 +1,6 @@
>  CFLAGS += -g -I../../../../usr/include/
>  
> -TEST_GEN_PROGS := membarrier_test
> +TEST_GEN_PROGS := membarrier_test membarrier_test2
>  
>  include ../lib.mk
>  
> diff --git a/tools/testing/selftests/membarrier/membarrier_test2.c b/tools/testing/selftests/membarrier/membarrier_test2.c
> new file mode 100644
> index 000000000000..8fa1be6156fb
> --- /dev/null
> +++ b/tools/testing/selftests/membarrier/membarrier_test2.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <linux/membarrier.h>
> +#include <syscall.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <string.h>
> +
> +#include "../kselftest.h"
> +/*
> +	MEMBARRIER_CMD_QUERY
> +		returns membarrier_cmd with supported features
> +	MEMBARRIER_CMD_GLOBAL
> +		returns 0
> +		EINVAL = if nohz_full is enabled
> +	MEMBARRIER_CMD_GLOBAL_EXPEDITED
> +		returns 0
> +	MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED
> +		returns 0
> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +		EPERM  = if process did not register for PRIVATE_EXPEDITED
> +	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +		EPERM = if process did not register for PRIVATE_EXPEDITED
> +	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +*/
> +
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
> +struct memb_tests {
> +	char testname[80];
> +	int command;
> +	int flags;
> +	int exp_ret;
> +	int exp_errno;
> +	int supported;
> +	int force;
> +};
> +
> +struct memb_tests mbt[] = {
> +	{
> +	 .testname = "bad_cmd\0",
> +	 .command = -1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .supported = 1,
> +	 },
> +	{
> +	 .testname = "cmd_with_flags_set\0",
> +	 .command = MEMBARRIER_CMD_QUERY,
> +	 .flags = 1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .supported = 1,
> +	 },
> +	{
> +	 .testname = "cmd_global\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +};
> +
> +static void info_passed_ok(struct memb_tests test)
> +{
> +	ksft_test_result_pass("sys_membarrier(): %s succeeded.\n",
> +			test.testname);
> +}
> +

Why do we need to add new routines for these conditions. Why can't handle
these strings in array. For example you can define an array of strings for

passed unexpectedly etc. and the pass the string to appropriate ksft_* interface
instead of adding of these routines. Also it is hard to review the code this way.
I would like to see the changes made to membarrier_test.c instead of adding a new
file.

I do like the direction though.

thanks,
-- Shuah

[PATCH v3] membarrier_test: work in progress

[Shuah Khan]

Hi Rafael,

Thanks for the patch. Comments below.

On 09/02/2018 08:12 PM, Rafael David Tinoco wrote:
> Shuah,
> 
> This is a draft only. I will remove summary from the top, run checkers,
> etc. Im thinking in replacing membarrier_test entirely with this code
> (compatible to existing one). Right now, this code:
> 
>  - allows each test to succeed, fail or be skipped independently
>  - allows each test to be tested even when not supported (force option)
>  - considers false negatives and false positives on every case
>  - can be extended easily
> 
> Right now, just to show as an example, it gives us:
> 
> TAP version 13
> ok 1 sys_membarrier(): cmd_query succeeded.
> ok 2 sys_membarrier(): bad_cmd failed as expected.
> ok 3 sys_membarrier(): cmd_with_flags_set failed as expected.
> ok 4 sys_membarrier(): cmd_global succeeded.
> Pass 4 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0
> 1..4
> 
> Are you okay with such move ? Only big TODO here is adding all covered
> tests in the test array (easy move), testing all combinations with all
> supported kernel versions (lab already ready) and suggesting it to you,
> replacing membarrier_test.c.
> 
> PS: This is pretty close to how a LTP test would be, using their new
> API, but, since it addresses your concerns and seems like a
> simple/clean, code, I decided to suggest it as a replacement here (and
> it also fixes the issue with this test and LTS kernels).
> ---
>  tools/testing/selftests/membarrier/Makefile   |   2 +-
>  .../selftests/membarrier/membarrier_test2.c   | 180 ++++++++++++++++++
>  2 files changed, 181 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/membarrier/membarrier_test2.c
> 
> diff --git a/tools/testing/selftests/membarrier/Makefile b/tools/testing/selftests/membarrier/Makefile
> index 02845532b059..3d44d4cd3a9d 100644
> --- a/tools/testing/selftests/membarrier/Makefile
> +++ b/tools/testing/selftests/membarrier/Makefile
> @@ -1,6 +1,6 @@
>  CFLAGS += -g -I../../../../usr/include/
>  
> -TEST_GEN_PROGS := membarrier_test
> +TEST_GEN_PROGS := membarrier_test membarrier_test2
>  
>  include ../lib.mk
>  
> diff --git a/tools/testing/selftests/membarrier/membarrier_test2.c b/tools/testing/selftests/membarrier/membarrier_test2.c
> new file mode 100644
> index 000000000000..8fa1be6156fb
> --- /dev/null
> +++ b/tools/testing/selftests/membarrier/membarrier_test2.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <linux/membarrier.h>
> +#include <syscall.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <string.h>
> +
> +#include "../kselftest.h"
> +/*
> +	MEMBARRIER_CMD_QUERY
> +		returns membarrier_cmd with supported features
> +	MEMBARRIER_CMD_GLOBAL
> +		returns 0
> +		EINVAL = if nohz_full is enabled
> +	MEMBARRIER_CMD_GLOBAL_EXPEDITED
> +		returns 0
> +	MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED
> +		returns 0
> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +		EPERM  = if process did not register for PRIVATE_EXPEDITED
> +	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +		EPERM = if process did not register for PRIVATE_EXPEDITED
> +	MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE
> +		returns 0
> +		EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled
> +*/
> +
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
> +struct memb_tests {
> +	char testname[80];
> +	int command;
> +	int flags;
> +	int exp_ret;
> +	int exp_errno;
> +	int supported;
> +	int force;
> +};
> +
> +struct memb_tests mbt[] = {
> +	{
> +	 .testname = "bad_cmd\0",
> +	 .command = -1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .supported = 1,
> +	 },
> +	{
> +	 .testname = "cmd_with_flags_set\0",
> +	 .command = MEMBARRIER_CMD_QUERY,
> +	 .flags = 1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .supported = 1,
> +	 },
> +	{
> +	 .testname = "cmd_global\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +};
> +
> +static void info_passed_ok(struct memb_tests test)
> +{
> +	ksft_test_result_pass("sys_membarrier(): %s succeeded.\n",
> +			test.testname);
> +}
> +

Why do we need to add new routines for these conditions. Why can't handle
these strings in array. For example you can define an array of strings for

passed unexpectedly etc. and the pass the string to appropriate ksft_* interface
instead of adding of these routines. Also it is hard to review the code this way.
I would like to see the changes made to membarrier_test.c instead of adding a new
file.

I do like the direction though.

thanks,
-- Shuah

[PATCH v4] selftests: membarrier: reorganized test for LTS supportability

[shuah]

On 09/03/2018 01:04 PM, Rafael David Tinoco wrote:
> This commit re-organizes membarrier test, solving issues when testing
> LTS kernels. Now, the code:
> 
>  - always run the same amount of tests (even on older kernels).
>  - allows each test to succeed, fail or be skipped independently.
>  - allows testing features even when explicitly unsupported (force=1).
>  - able to consider different return codes for diff kernel versions.
>  - checks false positive/negative by checking ret code and errno.
>  - can be extended easily: to expand an array with commands.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> Signed-off-by: Rafael David Tinoco <rafael.tinoco at linaro.org>
> ---
>  .../selftests/membarrier/membarrier_test.c    | 482 +++++++++---------
>  1 file changed, 241 insertions(+), 241 deletions(-)
> 
> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> index 6793f8ecc8e7..151bc8a944a3 100644
> --- a/tools/testing/selftests/membarrier/membarrier_test.c
> +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #define _GNU_SOURCE
>  #include <linux/membarrier.h>
> +#include <sys/utsname.h>
>  #include <syscall.h>
>  #include <stdio.h>
>  #include <errno.h>
> @@ -8,305 +9,304 @@
>  
>  #include "../kselftest.h"
>  
> -static int sys_membarrier(int cmd, int flags)
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> +
> +struct memb_tests {
> +	char testname[80];
> +	int command;
> +	int flags;
> +	int exp_ret;
> +	int exp_errno;
> +	int enabled;
> +	int force;
> +	int force_exp_errno;
> +	int above;
> +	int bellow;
> +};
> +
> +struct memb_tests mbt[] = {
> +	{
> +	 .testname = "cmd_fail\0",
> +	 .command = -1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .enabled = 1,
> +	 },
> +	{
> +	 .testname = "cmd_flags_fail\0",
> +	 .command = MEMBARRIER_CMD_QUERY,
> +	 .flags = 1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .enabled = 1,
> +	 },
> +	{
> +	 .testname = "cmd_global_success\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	/*
> +	 * PRIVATE EXPEDITED (forced)
> +	 */
> +	{
> +	 .testname = "cmd_private_expedited_fail\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = -1,
> +	 .exp_errno = EPERM,
> +	 .force = 1,
> +	 .force_exp_errno = EINVAL,
> +	 .bellow = KERNEL_VERSION(4, 10, 0),
> +	 },
> +	{
> +	 .testname = "cmd_private_expedited_fail\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = -1,
> +	 .exp_errno = EPERM,
> +	 .force = 1,
> +	 .force_exp_errno = EPERM,
> +	 .above = KERNEL_VERSION(4, 10, 0),
> +	 },
> +	{
> +	 .testname = "cmd_register_private_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 .force = 1,
> +	 .force_exp_errno = EINVAL,
> +	 },
> +	{
> +	 .testname = "cmd_private_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 .force = 1,
> +	 .force_exp_errno = EINVAL,
> +	 },
> +	 /*
> +	  * PRIVATE EXPEDITED SYNC CORE
> +	  */
> +	{
> +	 .testname = "cmd_private_expedited_sync_core_fail\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
> +	 .flags = 0,
> +	 .exp_ret = -1,
> +	 .exp_errno = EPERM,
> +	 },
> +	{
> +	 .testname = "cmd_register_private_expedited_sync_core_success\0",
> +	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	{
> +	 .testname = "cmd_private_expedited_sync_core_success\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	/*
> +	 * GLOBAL EXPEDITED
> +	 * global membarrier from a non-registered process is valid
> +	 */
> +	{
> +	 .testname = "cmd_global_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	{
> +	 .testname = "cmd_register_global_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	{
> +	 .testname = "cmd_global_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +};
> +
> +static void
> +info_passed_ok(struct memb_tests test)
>  {
> -	return syscall(__NR_membarrier, cmd, flags);
> +	ksft_test_result_pass("sys_membarrier(): %s succeeded.\n",
> +			test.testname);
>  }
>  

Why do we need to add new routines for these conditions. Why can't handle
these strings in array. For example you can define an array of strings for

passed unexpectedly etc. and the pass the string to appropriate ksft_* interface
instead of adding of these routines. Also it is hard to review the code this way.

I do like the direction though. Also please run get_maintainer.pl and cc everybody
it suggests.

thanks,
-- Shuah

[PATCH v4] selftests: membarrier: reorganized test for LTS supportability

[Shuah Khan]

On 09/03/2018 01:04 PM, Rafael David Tinoco wrote:
> This commit re-organizes membarrier test, solving issues when testing
> LTS kernels. Now, the code:
> 
>  - always run the same amount of tests (even on older kernels).
>  - allows each test to succeed, fail or be skipped independently.
>  - allows testing features even when explicitly unsupported (force=1).
>  - able to consider different return codes for diff kernel versions.
>  - checks false positive/negative by checking ret code and errno.
>  - can be extended easily: to expand an array with commands.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> Signed-off-by: Rafael David Tinoco <rafael.tinoco at linaro.org>
> ---
>  .../selftests/membarrier/membarrier_test.c    | 482 +++++++++---------
>  1 file changed, 241 insertions(+), 241 deletions(-)
> 
> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
> index 6793f8ecc8e7..151bc8a944a3 100644
> --- a/tools/testing/selftests/membarrier/membarrier_test.c
> +++ b/tools/testing/selftests/membarrier/membarrier_test.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #define _GNU_SOURCE
>  #include <linux/membarrier.h>
> +#include <sys/utsname.h>
>  #include <syscall.h>
>  #include <stdio.h>
>  #include <errno.h>
> @@ -8,305 +9,304 @@
>  
>  #include "../kselftest.h"
>  
> -static int sys_membarrier(int cmd, int flags)
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> +
> +struct memb_tests {
> +	char testname[80];
> +	int command;
> +	int flags;
> +	int exp_ret;
> +	int exp_errno;
> +	int enabled;
> +	int force;
> +	int force_exp_errno;
> +	int above;
> +	int bellow;
> +};
> +
> +struct memb_tests mbt[] = {
> +	{
> +	 .testname = "cmd_fail\0",
> +	 .command = -1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .enabled = 1,
> +	 },
> +	{
> +	 .testname = "cmd_flags_fail\0",
> +	 .command = MEMBARRIER_CMD_QUERY,
> +	 .flags = 1,
> +	 .exp_ret = -1,
> +	 .exp_errno = EINVAL,
> +	 .enabled = 1,
> +	 },
> +	{
> +	 .testname = "cmd_global_success\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	/*
> +	 * PRIVATE EXPEDITED (forced)
> +	 */
> +	{
> +	 .testname = "cmd_private_expedited_fail\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = -1,
> +	 .exp_errno = EPERM,
> +	 .force = 1,
> +	 .force_exp_errno = EINVAL,
> +	 .bellow = KERNEL_VERSION(4, 10, 0),
> +	 },
> +	{
> +	 .testname = "cmd_private_expedited_fail\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = -1,
> +	 .exp_errno = EPERM,
> +	 .force = 1,
> +	 .force_exp_errno = EPERM,
> +	 .above = KERNEL_VERSION(4, 10, 0),
> +	 },
> +	{
> +	 .testname = "cmd_register_private_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 .force = 1,
> +	 .force_exp_errno = EINVAL,
> +	 },
> +	{
> +	 .testname = "cmd_private_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 .force = 1,
> +	 .force_exp_errno = EINVAL,
> +	 },
> +	 /*
> +	  * PRIVATE EXPEDITED SYNC CORE
> +	  */
> +	{
> +	 .testname = "cmd_private_expedited_sync_core_fail\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
> +	 .flags = 0,
> +	 .exp_ret = -1,
> +	 .exp_errno = EPERM,
> +	 },
> +	{
> +	 .testname = "cmd_register_private_expedited_sync_core_success\0",
> +	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	{
> +	 .testname = "cmd_private_expedited_sync_core_success\0",
> +	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	/*
> +	 * GLOBAL EXPEDITED
> +	 * global membarrier from a non-registered process is valid
> +	 */
> +	{
> +	 .testname = "cmd_global_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	{
> +	 .testname = "cmd_register_global_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +	{
> +	 .testname = "cmd_global_expedited_success\0",
> +	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
> +	 .flags = 0,
> +	 .exp_ret = 0,
> +	 },
> +};
> +
> +static void
> +info_passed_ok(struct memb_tests test)
>  {
> -	return syscall(__NR_membarrier, cmd, flags);
> +	ksft_test_result_pass("sys_membarrier(): %s succeeded.\n",
> +			test.testname);
>  }
>  

Why do we need to add new routines for these conditions. Why can't handle
these strings in array. For example you can define an array of strings for

passed unexpectedly etc. and the pass the string to appropriate ksft_* interface
instead of adding of these routines. Also it is hard to review the code this way.

I do like the direction though. Also please run get_maintainer.pl and cc everybody
it suggests.

thanks,
-- Shuah

[PATCH v5] selftests: membarrier: re-organize test

[rafael.tinoco]

This commit re-organizes membarrier test, solving issues when testing
LTS kernels. Now, the code:

 - always run the same amount of tests (even on older kernels).
 - allows each test to succeed, fail or be skipped independently.
 - allows testing features even when explicitly unsupported (force=1).
 - checks false positive/negative by checking ret code and errno.
 - can be extended easily: to expand an array with commands.

Note: like this, the test is pretty close to the LTP membarrier basic
tests, and both can be maintained together.

Link: https://bugs.linaro.org/show_bug.cgi?id=3771
Link: http://lists.linux.it/pipermail/ltp/2018-October/009578.html
Signed-off-by: Rafael David Tinoco <rafael.tinoco at linaro.org>
---
 .../selftests/membarrier/membarrier_test.c    | 566 ++++++++++--------
 1 file changed, 314 insertions(+), 252 deletions(-)

diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
index 6793f8ecc8e7..7eb0e2395cbd 100644
--- a/tools/testing/selftests/membarrier/membarrier_test.c
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -8,305 +8,367 @@
 
 #include "../kselftest.h"
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
+struct test_case {
+	char testname[80];
+	int command;		/* membarrier cmd                            */
+	int needregister;	/* membarrier cmd needs register cmd	     */
+	int flags;		/* flags for given membarrier cmd	     */
+	long exp_ret;		/* expected return code for given cmd        */
+	int exp_errno;		/* expected errno for given cmd failure      */
+	int enabled;		/* enabled, despite results from CMD_QUERY   */
+	int always;		/* CMD_QUERY should always enable this test  */
+	int force;		/* force if CMD_QUERY reports not enabled    */
+	int force_exp_errno;	/* expected errno after forced cmd           */
+};
+
+struct test_case tc[] = {
+	{
+	 /*
+	  * case 00) invalid cmd
+	  *     - enabled by default
+	  *     - should always fail with EINVAL
+	  */
+	 .testname = "cmd_fail",
+	 .command = -1,
+	 .exp_ret = -1,
+	 .exp_errno = EINVAL,
+	 .enabled = 1,
+	 },
+	{
+	 /*
+	  * case 01) invalid flags
+	  *     - enabled by default
+	  *     - should always fail with EINVAL
+	  */
+	 .testname = "cmd_flags_fail",
+	 .command = MEMBARRIER_CMD_QUERY,
+	 .flags = 1,
+	 .exp_ret = -1,
+	 .exp_errno = EINVAL,
+	 .enabled = 1,
+	 },
+	{
+	 /*
+	  * case 02) global barrier
+	  *     - should ALWAYS be enabled by CMD_QUERY
+	  *     - should always succeed
+	  */
+	 .testname = "cmd_global_success",
+	 .command = MEMBARRIER_CMD_GLOBAL,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .always = 1,
+	 },
+	 /*
+	  * commit 22e4ebb975 (v4.14-rc1) added cases 03, 04 and 05 features:
+	  */
+	{
+	 /*
+	  * case 03) private expedited barrier with no registrations
+	  *     - should fail with errno=EPERM due to no registrations
+	  *     - or be skipped if unsupported by running kernel
+	  */
+	 .testname = "cmd_private_expedited_fail",
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = -1,
+	 .exp_errno = EPERM,
+	 },
+	{
+	 /*
+	  * case 04) register private expedited
+	  *     - should succeed when supported by running kernel
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_private_expedited_register_success",
+	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	{
+	 /*
+	  * case 05) private expedited barrier with registration
+	  *     - should succeed due to existing registration
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  *     - NOTE: commit 70216e18e5 (v4.16-rc1) changed behavior:
+	  *     -       (a) if unsupported, and forced, < 4.16 , errno is EINVAL
+	  *     -       (b) if unsupported, and forced, >= 4.16, errno is EPERM
+	  */
+	 .testname = "cmd_private_expedited_success",
+	 .needregister = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EPERM,
+	 },
+	 /*
+	  * commit 70216e18e5 (v4.16-rc1) added cases 06, 07 and 08 features:
+	  */
+	{
+	 /*
+	  * case 06) private expedited sync core barrier with no registrations
+	  *     - should fail with errno=EPERM due to no registrations
+	  *     - or be skipped if unsupported by running kernel
+	  */
+	 .testname = "cmd_private_expedited_sync_core_fail",
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .flags = 0,
+	 .exp_ret = -1,
+	 .exp_errno = EPERM,
+	 },
+	{
+	 /*
+	  * case 07) register private expedited sync core
+	  *     - should succeed when supported by running kernel
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_private_expedited_sync_core_register_success",
+	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	{
+	 /*
+	  * case 08) private expedited sync core barrier with registration
+	  *     - should succeed due to existing registration
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_private_expedited_sync_core_success",
+	 .needregister = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	 /*
+	  * commit c5f58bd58f4 (v4.16-rc1) added cases 09, 10 and 11 features:
+	  */
+	{
+	 /*
+	  * case 09) global expedited barrier with no registrations
+	  *     - should never fail due to no registrations
+	  *     - or be skipped if unsupported by running kernel
+	  */
+	 .testname = "cmd_global_expedited_success",
+	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 },
+	{
+	 /*
+	  * case 10) register global expedited
+	  *     - should succeed when supported by running kernel
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_global_expedited_register_success",
+	 .command = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	{
+	 /*
+	  * case 11) global expedited barrier with registration
+	  *     - should also succeed with registrations
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_global_expedited_success",
+	 .needregister = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
+	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+};
+
+#define passed_ok(_test)						       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s passed.\n",	       \
+			_test.testname);				       \
+		return;							       \
+	} while (0)
+
+#define passed_unexpec(_test)						       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s passed unexpectedly. "   \
+			"ret = %ld with errno %d were expected. (force: %d)\n",\
+			_test.testname, _test.exp_ret, _test.exp_errno,        \
+			_test.force);					       \
+		return;							       \
+	} while (0)
+
+#define failed_ok(_test)						       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s failed as "	       \
+			"expected.\n", _test.testname);			       \
+		return;							       \
+	} while (0)
+
+#define failed_ok_unsupported(_test)					       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s failed as expected.\n"\
+			"(unsupported)", _test.testname);		       \
+		return;							       \
+	} while (0)
+
+#define failed_not_ok(_test, _gotret, _goterr)				       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s failed. "		       \
+			"ret = %ld when expected was %ld. "		       \
+			"errno = %d when expected was %d. (force: %d)\n",      \
+			_test.testname, _gotret, _test.exp_ret, _goterr,       \
+			_test.exp_errno, _test.force);			       \
+		return;							       \
+	} while (0)
+
+#define failed_unexpec(_test, _gotret, _goterr) 			       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s failed unexpectedly. "   \
+			"Got ret = %ld with errno %d. (force: %d)\n",	       \
+			_test.testname, _gotret, _goterr, _test.force);	       \
+		return;							       \
+	} while (0)
+
+#define skipped(_test)							       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s skipped (unsupp)\n",  \
+			_test.testname);				       \
+		return;							       \
+	} while (0)
+
+#define skipped_fail(_test)						       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s reported as "	       \
+			"unsupported\n", _test.testname);		       \
+		return;							       \
+	} while (0)
+
 static int sys_membarrier(int cmd, int flags)
 {
 	return syscall(__NR_membarrier, cmd, flags);
 }
 
-static int test_membarrier_cmd_fail(void)
+static void test_membarrier_setup(void)
 {
-	int cmd = -1, flags = 0;
-	const char *test_name = "sys membarrier invalid command";
+	size_t i;
+	int ret;
 
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: command = %d, flags = %d. Should fail, but passed\n",
-			test_name, cmd, flags);
-	}
-	if (errno != EINVAL) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EINVAL, strerror(EINVAL),
-			errno, strerror(errno));
+	ret = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
+	if (ret < 0) {
+		if (errno == ENOSYS)
+			ksft_test_result_skip("membarrier(2): not supported\n");
 	}
 
-	ksft_test_result_pass(
-		"%s test: command = %d, flags = %d, errno = %d. Failed as expected\n",
-		test_name, cmd, flags, errno);
-	return 0;
-}
-
-static int test_membarrier_flags_fail(void)
-{
-	int cmd = MEMBARRIER_CMD_QUERY, flags = 1;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_QUERY invalid flags";
-
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should fail, but passed\n",
-			test_name, flags);
+	for (i = 0; i < ARRAY_SIZE(tc); i++) {
+		if ((tc[i].command > 0) && (ret & tc[i].command))
+			tc[i].enabled = 1;
 	}
-	if (errno != EINVAL) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EINVAL, strerror(EINVAL),
-			errno, strerror(errno));
-	}
-
-	ksft_test_result_pass(
-		"%s test: flags = %d, errno = %d. Failed as expected\n",
-		test_name, flags, errno);
-	return 0;
 }
 
-static int test_membarrier_global_success(void)
+static void test_membarrier_run(unsigned int i)
 {
-	int cmd = MEMBARRIER_CMD_GLOBAL, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_GLOBAL";
+	int ret, err = 0;
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+	/* not enabled and not enforced: test is skipped */
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n", test_name, flags);
-	return 0;
-}
+	if (!tc[i].enabled && !tc[i].force) {
 
-static int test_membarrier_private_expedited_fail(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED not registered failure";
+		if (tc[i].always == 0)
+			skipped(tc[i]);
 
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should fail, but passed\n",
-			test_name, flags);
-	}
-	if (errno != EPERM) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EPERM, strerror(EPERM),
-			errno, strerror(errno));
+		skipped_fail(tc[i]);
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d, errno = %d\n",
-		test_name, flags, errno);
-	return 0;
-}
+	/* iterations: registration needed for some cases */
 
-static int test_membarrier_register_private_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED";
-
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
+	if (tc[i].needregister && tc[i].enabled) {
+		ret = sys_membarrier(tc[i].needregister, 0);
+		if (ret < 0) {
+			ksft_test_result_fail("membarrier(2): %s could not"
+					"register\n", tc[i].testname);
+		}
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+	ret = sys_membarrier(tc[i].command, tc[i].flags);
+	err = errno;
 
-static int test_membarrier_private_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED";
+	/* enabled and not enforced: regular expected results only */
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+	if (tc[i].enabled && !tc[i].force) {
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+		if (ret >= 0 && tc[i].exp_ret == ret)
+			passed_ok(tc[i]);
 
-static int test_membarrier_private_expedited_sync_core_fail(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE not registered failure";
-
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should fail, but passed\n",
-			test_name, flags);
-	}
-	if (errno != EPERM) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EPERM, strerror(EPERM),
-			errno, strerror(errno));
-	}
-
-	ksft_test_result_pass(
-		"%s test: flags = %d, errno = %d\n",
-		test_name, flags, errno);
-	return 0;
-}
-
-static int test_membarrier_register_private_expedited_sync_core_success(void)
-{
-	int cmd = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE";
-
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
+		if (ret < 0) {
+			if (tc[i].exp_ret == ret)
+				failed_ok(tc[i]);
+			else
+				failed_not_ok(tc[i], ret, err);
+		}
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+	/* not enabled and enforced: failure and expected errors */
 
-static int test_membarrier_private_expedited_sync_core_success(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE";
+	if (!tc[i].enabled && tc[i].force) {
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+		if (ret >= 0)
+			passed_unexpec(tc[i]);
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
-
-static int test_membarrier_register_global_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED";
-
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
+		if (ret < 0) {
+			if (tc[i].force_exp_errno == err)
+				failed_ok_unsupported(tc[i]);
+			else
+				failed_unexpec(tc[i], ret, err);
+		}
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+	/* enabled and enforced: tricky */
 
-static int test_membarrier_global_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_GLOBAL_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED";
+	if (tc[i].enabled && tc[i].force) {
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+		if (ret >= 0) {
+			if (tc[i].exp_ret == ret)
+				passed_ok(tc[i]);
+			else
+				passed_unexpec(tc[i]);
+		}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+		if (ret < 0) {
 
-static int test_membarrier(void)
-{
-	int status;
-
-	status = test_membarrier_cmd_fail();
-	if (status)
-		return status;
-	status = test_membarrier_flags_fail();
-	if (status)
-		return status;
-	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;
-	}
-	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
-		status = test_membarrier_private_expedited_sync_core_fail();
-		if (status)
-			return status;
-		status = test_membarrier_register_private_expedited_sync_core_success();
-		if (status)
-			return status;
-		status = test_membarrier_private_expedited_sync_core_success();
-		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;
-	return 0;
-}
+			if (tc[i].exp_ret == ret) {
 
-static int test_membarrier_query(void)
-{
-	int flags = 0, ret;
+				if (tc[i].exp_errno == err)
+					failed_ok(tc[i]);
+				else
+					failed_unexpec(tc[i], ret, err);
+			}
 
-	ret = sys_membarrier(MEMBARRIER_CMD_QUERY, flags);
-	if (ret < 0) {
-		if (errno == ENOSYS) {
-			/*
-			 * It is valid to build a kernel with
-			 * CONFIG_MEMBARRIER=n. However, this skips the tests.
-			 */
-			ksft_exit_skip(
-				"sys membarrier (CONFIG_MEMBARRIER) is disabled.\n");
+			/* unknown on force failure if enabled and forced */
+			failed_unexpec(tc[i], ret, err);
 		}
-		ksft_exit_fail_msg("sys_membarrier() failed\n");
 	}
-	if (!(ret & MEMBARRIER_CMD_GLOBAL))
-		ksft_exit_skip(
-			"sys_membarrier unsupported: CMD_GLOBAL not found.\n");
-
-	ksft_test_result_pass("sys_membarrier available\n");
-	return 0;
 }
 
 int main(int argc, char **argv)
 {
+	size_t i;
+
 	ksft_print_header();
 
-	test_membarrier_query();
-	test_membarrier();
+	test_membarrier_setup();
+
+	for (i = 0; i < ARRAY_SIZE(tc); i++)
+		test_membarrier_run(i);
 
 	return ksft_exit_pass();
 }
-- 
2.19.1

[PATCH v5] selftests: membarrier: re-organize test

[Rafael David Tinoco]

This commit re-organizes membarrier test, solving issues when testing
LTS kernels. Now, the code:

 - always run the same amount of tests (even on older kernels).
 - allows each test to succeed, fail or be skipped independently.
 - allows testing features even when explicitly unsupported (force=1).
 - checks false positive/negative by checking ret code and errno.
 - can be extended easily: to expand an array with commands.

Note: like this, the test is pretty close to the LTP membarrier basic
tests, and both can be maintained together.

Link: https://bugs.linaro.org/show_bug.cgi?id=3771
Link: http://lists.linux.it/pipermail/ltp/2018-October/009578.html
Signed-off-by: Rafael David Tinoco <rafael.tinoco at linaro.org>
---
 .../selftests/membarrier/membarrier_test.c    | 566 ++++++++++--------
 1 file changed, 314 insertions(+), 252 deletions(-)

diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
index 6793f8ecc8e7..7eb0e2395cbd 100644
--- a/tools/testing/selftests/membarrier/membarrier_test.c
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -8,305 +8,367 @@
 
 #include "../kselftest.h"
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
+struct test_case {
+	char testname[80];
+	int command;		/* membarrier cmd                            */
+	int needregister;	/* membarrier cmd needs register cmd	     */
+	int flags;		/* flags for given membarrier cmd	     */
+	long exp_ret;		/* expected return code for given cmd        */
+	int exp_errno;		/* expected errno for given cmd failure      */
+	int enabled;		/* enabled, despite results from CMD_QUERY   */
+	int always;		/* CMD_QUERY should always enable this test  */
+	int force;		/* force if CMD_QUERY reports not enabled    */
+	int force_exp_errno;	/* expected errno after forced cmd           */
+};
+
+struct test_case tc[] = {
+	{
+	 /*
+	  * case 00) invalid cmd
+	  *     - enabled by default
+	  *     - should always fail with EINVAL
+	  */
+	 .testname = "cmd_fail",
+	 .command = -1,
+	 .exp_ret = -1,
+	 .exp_errno = EINVAL,
+	 .enabled = 1,
+	 },
+	{
+	 /*
+	  * case 01) invalid flags
+	  *     - enabled by default
+	  *     - should always fail with EINVAL
+	  */
+	 .testname = "cmd_flags_fail",
+	 .command = MEMBARRIER_CMD_QUERY,
+	 .flags = 1,
+	 .exp_ret = -1,
+	 .exp_errno = EINVAL,
+	 .enabled = 1,
+	 },
+	{
+	 /*
+	  * case 02) global barrier
+	  *     - should ALWAYS be enabled by CMD_QUERY
+	  *     - should always succeed
+	  */
+	 .testname = "cmd_global_success",
+	 .command = MEMBARRIER_CMD_GLOBAL,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .always = 1,
+	 },
+	 /*
+	  * commit 22e4ebb975 (v4.14-rc1) added cases 03, 04 and 05 features:
+	  */
+	{
+	 /*
+	  * case 03) private expedited barrier with no registrations
+	  *     - should fail with errno=EPERM due to no registrations
+	  *     - or be skipped if unsupported by running kernel
+	  */
+	 .testname = "cmd_private_expedited_fail",
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = -1,
+	 .exp_errno = EPERM,
+	 },
+	{
+	 /*
+	  * case 04) register private expedited
+	  *     - should succeed when supported by running kernel
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_private_expedited_register_success",
+	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	{
+	 /*
+	  * case 05) private expedited barrier with registration
+	  *     - should succeed due to existing registration
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  *     - NOTE: commit 70216e18e5 (v4.16-rc1) changed behavior:
+	  *     -       (a) if unsupported, and forced, < 4.16 , errno is EINVAL
+	  *     -       (b) if unsupported, and forced, >= 4.16, errno is EPERM
+	  */
+	 .testname = "cmd_private_expedited_success",
+	 .needregister = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED,
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EPERM,
+	 },
+	 /*
+	  * commit 70216e18e5 (v4.16-rc1) added cases 06, 07 and 08 features:
+	  */
+	{
+	 /*
+	  * case 06) private expedited sync core barrier with no registrations
+	  *     - should fail with errno=EPERM due to no registrations
+	  *     - or be skipped if unsupported by running kernel
+	  */
+	 .testname = "cmd_private_expedited_sync_core_fail",
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .flags = 0,
+	 .exp_ret = -1,
+	 .exp_errno = EPERM,
+	 },
+	{
+	 /*
+	  * case 07) register private expedited sync core
+	  *     - should succeed when supported by running kernel
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_private_expedited_sync_core_register_success",
+	 .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	{
+	 /*
+	  * case 08) private expedited sync core barrier with registration
+	  *     - should succeed due to existing registration
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_private_expedited_sync_core_success",
+	 .needregister = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	 /*
+	  * commit c5f58bd58f4 (v4.16-rc1) added cases 09, 10 and 11 features:
+	  */
+	{
+	 /*
+	  * case 09) global expedited barrier with no registrations
+	  *     - should never fail due to no registrations
+	  *     - or be skipped if unsupported by running kernel
+	  */
+	 .testname = "cmd_global_expedited_success",
+	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 },
+	{
+	 /*
+	  * case 10) register global expedited
+	  *     - should succeed when supported by running kernel
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_global_expedited_register_success",
+	 .command = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+	{
+	 /*
+	  * case 11) global expedited barrier with registration
+	  *     - should also succeed with registrations
+	  *     - or fail with errno=EINVAL if unsupported and forced
+	  */
+	 .testname = "cmd_global_expedited_success",
+	 .needregister = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
+	 .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED,
+	 .flags = 0,
+	 .exp_ret = 0,
+	 .force = 1,
+	 .force_exp_errno = EINVAL,
+	 },
+};
+
+#define passed_ok(_test)						       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s passed.\n",	       \
+			_test.testname);				       \
+		return;							       \
+	} while (0)
+
+#define passed_unexpec(_test)						       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s passed unexpectedly. "   \
+			"ret = %ld with errno %d were expected. (force: %d)\n",\
+			_test.testname, _test.exp_ret, _test.exp_errno,        \
+			_test.force);					       \
+		return;							       \
+	} while (0)
+
+#define failed_ok(_test)						       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s failed as "	       \
+			"expected.\n", _test.testname);			       \
+		return;							       \
+	} while (0)
+
+#define failed_ok_unsupported(_test)					       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s failed as expected.\n"\
+			"(unsupported)", _test.testname);		       \
+		return;							       \
+	} while (0)
+
+#define failed_not_ok(_test, _gotret, _goterr)				       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s failed. "		       \
+			"ret = %ld when expected was %ld. "		       \
+			"errno = %d when expected was %d. (force: %d)\n",      \
+			_test.testname, _gotret, _test.exp_ret, _goterr,       \
+			_test.exp_errno, _test.force);			       \
+		return;							       \
+	} while (0)
+
+#define failed_unexpec(_test, _gotret, _goterr) 			       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s failed unexpectedly. "   \
+			"Got ret = %ld with errno %d. (force: %d)\n",	       \
+			_test.testname, _gotret, _goterr, _test.force);	       \
+		return;							       \
+	} while (0)
+
+#define skipped(_test)							       \
+	do {								       \
+		ksft_test_result_pass("membarrier(2): %s skipped (unsupp)\n",  \
+			_test.testname);				       \
+		return;							       \
+	} while (0)
+
+#define skipped_fail(_test)						       \
+	do {								       \
+		ksft_exit_fail_msg("membarrier(2): %s reported as "	       \
+			"unsupported\n", _test.testname);		       \
+		return;							       \
+	} while (0)
+
 static int sys_membarrier(int cmd, int flags)
 {
 	return syscall(__NR_membarrier, cmd, flags);
 }
 
-static int test_membarrier_cmd_fail(void)
+static void test_membarrier_setup(void)
 {
-	int cmd = -1, flags = 0;
-	const char *test_name = "sys membarrier invalid command";
+	size_t i;
+	int ret;
 
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: command = %d, flags = %d. Should fail, but passed\n",
-			test_name, cmd, flags);
-	}
-	if (errno != EINVAL) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EINVAL, strerror(EINVAL),
-			errno, strerror(errno));
+	ret = sys_membarrier(MEMBARRIER_CMD_QUERY, 0);
+	if (ret < 0) {
+		if (errno == ENOSYS)
+			ksft_test_result_skip("membarrier(2): not supported\n");
 	}
 
-	ksft_test_result_pass(
-		"%s test: command = %d, flags = %d, errno = %d. Failed as expected\n",
-		test_name, cmd, flags, errno);
-	return 0;
-}
-
-static int test_membarrier_flags_fail(void)
-{
-	int cmd = MEMBARRIER_CMD_QUERY, flags = 1;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_QUERY invalid flags";
-
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should fail, but passed\n",
-			test_name, flags);
+	for (i = 0; i < ARRAY_SIZE(tc); i++) {
+		if ((tc[i].command > 0) && (ret & tc[i].command))
+			tc[i].enabled = 1;
 	}
-	if (errno != EINVAL) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EINVAL, strerror(EINVAL),
-			errno, strerror(errno));
-	}
-
-	ksft_test_result_pass(
-		"%s test: flags = %d, errno = %d. Failed as expected\n",
-		test_name, flags, errno);
-	return 0;
 }
 
-static int test_membarrier_global_success(void)
+static void test_membarrier_run(unsigned int i)
 {
-	int cmd = MEMBARRIER_CMD_GLOBAL, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_GLOBAL";
+	int ret, err = 0;
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+	/* not enabled and not enforced: test is skipped */
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n", test_name, flags);
-	return 0;
-}
+	if (!tc[i].enabled && !tc[i].force) {
 
-static int test_membarrier_private_expedited_fail(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED not registered failure";
+		if (tc[i].always == 0)
+			skipped(tc[i]);
 
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should fail, but passed\n",
-			test_name, flags);
-	}
-	if (errno != EPERM) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EPERM, strerror(EPERM),
-			errno, strerror(errno));
+		skipped_fail(tc[i]);
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d, errno = %d\n",
-		test_name, flags, errno);
-	return 0;
-}
+	/* iterations: registration needed for some cases */
 
-static int test_membarrier_register_private_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED";
-
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
+	if (tc[i].needregister && tc[i].enabled) {
+		ret = sys_membarrier(tc[i].needregister, 0);
+		if (ret < 0) {
+			ksft_test_result_fail("membarrier(2): %s could not"
+					"register\n", tc[i].testname);
+		}
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+	ret = sys_membarrier(tc[i].command, tc[i].flags);
+	err = errno;
 
-static int test_membarrier_private_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED";
+	/* enabled and not enforced: regular expected results only */
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+	if (tc[i].enabled && !tc[i].force) {
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+		if (ret >= 0 && tc[i].exp_ret == ret)
+			passed_ok(tc[i]);
 
-static int test_membarrier_private_expedited_sync_core_fail(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE not registered failure";
-
-	if (sys_membarrier(cmd, flags) != -1) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should fail, but passed\n",
-			test_name, flags);
-	}
-	if (errno != EPERM) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d. Should return (%d: \"%s\"), but returned (%d: \"%s\").\n",
-			test_name, flags, EPERM, strerror(EPERM),
-			errno, strerror(errno));
-	}
-
-	ksft_test_result_pass(
-		"%s test: flags = %d, errno = %d\n",
-		test_name, flags, errno);
-	return 0;
-}
-
-static int test_membarrier_register_private_expedited_sync_core_success(void)
-{
-	int cmd = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE";
-
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
+		if (ret < 0) {
+			if (tc[i].exp_ret == ret)
+				failed_ok(tc[i]);
+			else
+				failed_not_ok(tc[i], ret, err);
+		}
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+	/* not enabled and enforced: failure and expected errors */
 
-static int test_membarrier_private_expedited_sync_core_success(void)
-{
-	int cmd = MEMBARRIER_CMD_PRIVATE_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE";
+	if (!tc[i].enabled && tc[i].force) {
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+		if (ret >= 0)
+			passed_unexpec(tc[i]);
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
-
-static int test_membarrier_register_global_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED";
-
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
+		if (ret < 0) {
+			if (tc[i].force_exp_errno == err)
+				failed_ok_unsupported(tc[i]);
+			else
+				failed_unexpec(tc[i], ret, err);
+		}
 	}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+	/* enabled and enforced: tricky */
 
-static int test_membarrier_global_expedited_success(void)
-{
-	int cmd = MEMBARRIER_CMD_GLOBAL_EXPEDITED, flags = 0;
-	const char *test_name = "sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED";
+	if (tc[i].enabled && tc[i].force) {
 
-	if (sys_membarrier(cmd, flags) != 0) {
-		ksft_exit_fail_msg(
-			"%s test: flags = %d, errno = %d\n",
-			test_name, flags, errno);
-	}
+		if (ret >= 0) {
+			if (tc[i].exp_ret == ret)
+				passed_ok(tc[i]);
+			else
+				passed_unexpec(tc[i]);
+		}
 
-	ksft_test_result_pass(
-		"%s test: flags = %d\n",
-		test_name, flags);
-	return 0;
-}
+		if (ret < 0) {
 
-static int test_membarrier(void)
-{
-	int status;
-
-	status = test_membarrier_cmd_fail();
-	if (status)
-		return status;
-	status = test_membarrier_flags_fail();
-	if (status)
-		return status;
-	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;
-	}
-	if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) {
-		status = test_membarrier_private_expedited_sync_core_fail();
-		if (status)
-			return status;
-		status = test_membarrier_register_private_expedited_sync_core_success();
-		if (status)
-			return status;
-		status = test_membarrier_private_expedited_sync_core_success();
-		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;
-	return 0;
-}
+			if (tc[i].exp_ret == ret) {
 
-static int test_membarrier_query(void)
-{
-	int flags = 0, ret;
+				if (tc[i].exp_errno == err)
+					failed_ok(tc[i]);
+				else
+					failed_unexpec(tc[i], ret, err);
+			}
 
-	ret = sys_membarrier(MEMBARRIER_CMD_QUERY, flags);
-	if (ret < 0) {
-		if (errno == ENOSYS) {
-			/*
-			 * It is valid to build a kernel with
-			 * CONFIG_MEMBARRIER=n. However, this skips the tests.
-			 */
-			ksft_exit_skip(
-				"sys membarrier (CONFIG_MEMBARRIER) is disabled.\n");
+			/* unknown on force failure if enabled and forced */
+			failed_unexpec(tc[i], ret, err);
 		}
-		ksft_exit_fail_msg("sys_membarrier() failed\n");
 	}
-	if (!(ret & MEMBARRIER_CMD_GLOBAL))
-		ksft_exit_skip(
-			"sys_membarrier unsupported: CMD_GLOBAL not found.\n");
-
-	ksft_test_result_pass("sys_membarrier available\n");
-	return 0;
 }
 
 int main(int argc, char **argv)
 {
+	size_t i;
+
 	ksft_print_header();
 
-	test_membarrier_query();
-	test_membarrier();
+	test_membarrier_setup();
+
+	for (i = 0; i < ARRAY_SIZE(tc); i++)
+		test_membarrier_run(i);
 
 	return ksft_exit_pass();
 }
-- 
2.19.1

[PATCH v5] selftests: membarrier: re-organize test

[rafael.tinoco]

>> Why do we need to add new routines for these conditions. Why can't handle these strings in array. For example you can define an array of strings for passed unexpectedly etc. and the pass the string to appropriate ksft_* interface instead of adding of these routines. Also it is hard to review the code this way.

I was able to fit all the logic in the 80 char limit and, still, give a 
notion what was being called on each condition (instead of using an 
array number or equivalent). Considering this is not the core code, and 
the this has been already accepted and reviewed in LTP project, would 
you mind accepting it so both can be maintained together ? It is much 
better than the existing one, anyway...

Note: I have removed the part where we test for older return codes, 
since kselftest is not focusing in those (but LTP does).

>> I do like the direction though. Also please run get_maintainer.pl and cc everybody it suggests.

Done in v5.

Thanks a lot.

>> 
>> thanks,
>> -- Shuah


> This commit re-organizes membarrier test, solving issues when testing
> LTS kernels. Now, the code:
> 
>   - always run the same amount of tests (even on older kernels).
>   - allows each test to succeed, fail or be skipped independently.
>   - allows testing features even when explicitly unsupported (force=1).
>   - checks false positive/negative by checking ret code and errno.
>   - can be extended easily: to expand an array with commands.
> 
> Note: like this, the test is pretty close to the LTP membarrier basic
> tests, and both can be maintained together.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> Link: http://lists.linux.it/pipermail/ltp/2018-October/009578.html
> Signed-off-by: Rafael David Tinoco <rafael.tinoco at linaro.org>
> ---
-- 
Rafael D. Tinoco
Linaro Kernel Validation

[PATCH v5] selftests: membarrier: re-organize test

[Rafael David Tinoco]

>> Why do we need to add new routines for these conditions. Why can't handle these strings in array. For example you can define an array of strings for passed unexpectedly etc. and the pass the string to appropriate ksft_* interface instead of adding of these routines. Also it is hard to review the code this way.

I was able to fit all the logic in the 80 char limit and, still, give a 
notion what was being called on each condition (instead of using an 
array number or equivalent). Considering this is not the core code, and 
the this has been already accepted and reviewed in LTP project, would 
you mind accepting it so both can be maintained together ? It is much 
better than the existing one, anyway...

Note: I have removed the part where we test for older return codes, 
since kselftest is not focusing in those (but LTP does).

>> I do like the direction though. Also please run get_maintainer.pl and cc everybody it suggests.

Done in v5.

Thanks a lot.

>> 
>> thanks,
>> -- Shuah


> This commit re-organizes membarrier test, solving issues when testing
> LTS kernels. Now, the code:
> 
>   - always run the same amount of tests (even on older kernels).
>   - allows each test to succeed, fail or be skipped independently.
>   - allows testing features even when explicitly unsupported (force=1).
>   - checks false positive/negative by checking ret code and errno.
>   - can be extended easily: to expand an array with commands.
> 
> Note: like this, the test is pretty close to the LTP membarrier basic
> tests, and both can be maintained together.
> 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3771
> Link: http://lists.linux.it/pipermail/ltp/2018-October/009578.html
> Signed-off-by: Rafael David Tinoco <rafael.tinoco at linaro.org>
> ---
-- 
Rafael D. Tinoco
Linaro Kernel Validation