[LTP] [PATCH] getcpu01: Reinstate node_id test

[LTP] [PATCH] getcpu01: Reinstate node_id test

[Richard Palethorpe via ltp]

Presently the node_id is only checked on i386 and it is broken. The
sched_getcpu call was substituted for getcpu when
available. sched_getcpu does not have the node_id parameter and does
not even call SYS_getcpu if it can be completed by vDSO.

Also we can at least check the node_id on x86_64 as well.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/kernel/syscalls/getcpu/getcpu01.c | 22 +++++++++------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/testcases/kernel/syscalls/getcpu/getcpu01.c b/testcases/kernel/syscalls/getcpu/getcpu01.c
index fcc273e29..21c67f412 100644
--- a/testcases/kernel/syscalls/getcpu/getcpu01.c
+++ b/testcases/kernel/syscalls/getcpu/getcpu01.c
@@ -15,20 +15,16 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/types.h>
+#include "tst_test.h"
 #include "lapi/syscalls.h"
 #include "lapi/cpuset.h"
-#include "tst_test.h"
 #include "config.h"
 
 static inline int get_cpu(unsigned *cpu_id,
-			  unsigned *node_id LTP_ATTRIBUTE_UNUSED,
-			  void *cache_struct LTP_ATTRIBUTE_UNUSED)
+			  unsigned *node_id)
 {
-#ifndef HAVE_SCHED_GETCPU
-	return tst_syscall(__NR_getcpu, cpu_id, node_id, cache_struct);
-#else
-	*cpu_id = sched_getcpu();
-#endif
+	return tst_syscall(__NR_getcpu, cpu_id, node_id, NULL);
+
 	return 0;
 }
 
@@ -78,7 +74,7 @@ realloc:
 	return cpu_max;
 }
 
-#ifdef __i386__
+#if  defined(__i386__) || defined(__x86_64__)
 static unsigned int get_nodeid(unsigned int cpu_id)
 {
 	DIR *directory_parent, *directory_node;
@@ -125,22 +121,22 @@ static void run(void)
 {
 	unsigned int cpu_id, node_id = 0;
 	unsigned int cpu_set;
-#ifdef __i386__
+#if defined(__i386__) || defined(__x86_64__)
 	unsigned int node_set;
 #endif
 
 	cpu_set = set_cpu_affinity();
-#ifdef __i386__
+#if defined(__i386__) || defined(__x86_64__)
 	node_set = get_nodeid(cpu_set);
 #endif
 
-	TEST(get_cpu(&cpu_id, &node_id, NULL));
+	TEST(get_cpu(&cpu_id, &node_id));
 	if (TST_RET == 0) {
 		if (cpu_id != cpu_set)
 			tst_res(TFAIL, "getcpu() returned wrong value"
 				" expected cpuid:%d, returned value cpuid: %d",
 				cpu_set, cpu_id);
-#ifdef __i386__
+#if defined(__i386__) || defined(__x86_64__)
 		else if (node_id != node_set)
 			tst_res(TFAIL, "getcpu() returned wrong value"
 				" expected  node id:%d returned  node id:%d",
-- 
2.38.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] getcpu01: Reinstate node_id test

[Richard Palethorpe]

Hello,

Ping! I will merge soon if there are no comments.

Richard Palethorpe via ltp <ltp@lists.linux.it> writes:

> Presently the node_id is only checked on i386 and it is broken. The
> sched_getcpu call was substituted for getcpu when
> available. sched_getcpu does not have the node_id parameter and does
> not even call SYS_getcpu if it can be completed by vDSO.
>
> Also we can at least check the node_id on x86_64 as well.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  testcases/kernel/syscalls/getcpu/getcpu01.c | 22 +++++++++------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/getcpu/getcpu01.c b/testcases/kernel/syscalls/getcpu/getcpu01.c
> index fcc273e29..21c67f412 100644
> --- a/testcases/kernel/syscalls/getcpu/getcpu01.c
> +++ b/testcases/kernel/syscalls/getcpu/getcpu01.c
> @@ -15,20 +15,16 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <sys/types.h>
> +#include "tst_test.h"
>  #include "lapi/syscalls.h"
>  #include "lapi/cpuset.h"
> -#include "tst_test.h"
>  #include "config.h"
>  
>  static inline int get_cpu(unsigned *cpu_id,
> -			  unsigned *node_id LTP_ATTRIBUTE_UNUSED,
> -			  void *cache_struct LTP_ATTRIBUTE_UNUSED)
> +			  unsigned *node_id)
>  {
> -#ifndef HAVE_SCHED_GETCPU
> -	return tst_syscall(__NR_getcpu, cpu_id, node_id, cache_struct);
> -#else
> -	*cpu_id = sched_getcpu();
> -#endif
> +	return tst_syscall(__NR_getcpu, cpu_id, node_id, NULL);
> +
>  	return 0;
>  }
>  
> @@ -78,7 +74,7 @@ realloc:
>  	return cpu_max;
>  }
>  
> -#ifdef __i386__
> +#if  defined(__i386__) || defined(__x86_64__)
>  static unsigned int get_nodeid(unsigned int cpu_id)
>  {
>  	DIR *directory_parent, *directory_node;
> @@ -125,22 +121,22 @@ static void run(void)
>  {
>  	unsigned int cpu_id, node_id = 0;
>  	unsigned int cpu_set;
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>  	unsigned int node_set;
>  #endif
>  
>  	cpu_set = set_cpu_affinity();
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>  	node_set = get_nodeid(cpu_set);
>  #endif
>  
> -	TEST(get_cpu(&cpu_id, &node_id, NULL));
> +	TEST(get_cpu(&cpu_id, &node_id));
>  	if (TST_RET == 0) {
>  		if (cpu_id != cpu_set)
>  			tst_res(TFAIL, "getcpu() returned wrong value"
>  				" expected cpuid:%d, returned value cpuid: %d",
>  				cpu_set, cpu_id);
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>  		else if (node_id != node_set)
>  			tst_res(TFAIL, "getcpu() returned wrong value"
>  				" expected  node id:%d returned  node id:%d",
> -- 
> 2.38.1


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] getcpu01: Reinstate node_id test

[Cyril Hrubis]

Hi!
> Presently the node_id is only checked on i386 and it is broken. The
> sched_getcpu call was substituted for getcpu when
> available. sched_getcpu does not have the node_id parameter and does
> not even call SYS_getcpu if it can be completed by vDSO.
> 
> Also we can at least check the node_id on x86_64 as well.

I suppose that the getcpu manual page is a bit confusing, the function
is implemented on most of the major achitectures and as VDSO on most of
them too. It was only added first to x86 architectures.

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  testcases/kernel/syscalls/getcpu/getcpu01.c | 22 +++++++++------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/getcpu/getcpu01.c b/testcases/kernel/syscalls/getcpu/getcpu01.c
> index fcc273e29..21c67f412 100644
> --- a/testcases/kernel/syscalls/getcpu/getcpu01.c
> +++ b/testcases/kernel/syscalls/getcpu/getcpu01.c
> @@ -15,20 +15,16 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <sys/types.h>
> +#include "tst_test.h"
>  #include "lapi/syscalls.h"
>  #include "lapi/cpuset.h"
> -#include "tst_test.h"
>  #include "config.h"
>  
>  static inline int get_cpu(unsigned *cpu_id,
> -			  unsigned *node_id LTP_ATTRIBUTE_UNUSED,
> -			  void *cache_struct LTP_ATTRIBUTE_UNUSED)
> +			  unsigned *node_id)
>  {
> -#ifndef HAVE_SCHED_GETCPU
> -	return tst_syscall(__NR_getcpu, cpu_id, node_id, cache_struct);
> -#else
> -	*cpu_id = sched_getcpu();
> -#endif
> +	return tst_syscall(__NR_getcpu, cpu_id, node_id, NULL);

The call is mostly implemneted as VDSO so it would make much more sense
to test the libc function instead and test the implementation that is
actually used in production.

>  	return 0;
>  }
>  
> @@ -78,7 +74,7 @@ realloc:
>  	return cpu_max;
>  }
>  
> -#ifdef __i386__
> +#if  defined(__i386__) || defined(__x86_64__)
>  static unsigned int get_nodeid(unsigned int cpu_id)
>  {
>  	DIR *directory_parent, *directory_node;
> @@ -125,22 +121,22 @@ static void run(void)
>  {
>  	unsigned int cpu_id, node_id = 0;
>  	unsigned int cpu_set;


The get_nodeid() just parses sysfs, that should be portable, can't we
just get rid of the ifdefs completely instead?

> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>  	unsigned int node_set;
>  #endif
>  
>  	cpu_set = set_cpu_affinity();
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>  	node_set = get_nodeid(cpu_set);
>  #endif
>  
> -	TEST(get_cpu(&cpu_id, &node_id, NULL));
> +	TEST(get_cpu(&cpu_id, &node_id));
>  	if (TST_RET == 0) {
>  		if (cpu_id != cpu_set)
>  			tst_res(TFAIL, "getcpu() returned wrong value"
>  				" expected cpuid:%d, returned value cpuid: %d",
>  				cpu_set, cpu_id);
> -#ifdef __i386__
> +#if defined(__i386__) || defined(__x86_64__)
>  		else if (node_id != node_set)
>  			tst_res(TFAIL, "getcpu() returned wrong value"
>  				" expected  node id:%d returned  node id:%d",
> -- 
> 2.38.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] getcpu01: Reinstate node_id test

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> Presently the node_id is only checked on i386 and it is broken. The
>> sched_getcpu call was substituted for getcpu when
>> available. sched_getcpu does not have the node_id parameter and does
>> not even call SYS_getcpu if it can be completed by vDSO.
>> 
>> Also we can at least check the node_id on x86_64 as well.
>
> I suppose that the getcpu manual page is a bit confusing, the function
> is implemented on most of the major achitectures and as VDSO on most of
> them too. It was only added first to x86 architectures.

I suppose I overlooked that there is a libc implementation of
getcpu. IDK why sched_getcpu is here at all and my vDSO comment is just
about how sched_getcpu would not result in a call to getcpu. However I
agree we should just call the libc version.

>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>  testcases/kernel/syscalls/getcpu/getcpu01.c | 22 +++++++++------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/testcases/kernel/syscalls/getcpu/getcpu01.c b/testcases/kernel/syscalls/getcpu/getcpu01.c
>> index fcc273e29..21c67f412 100644
>> --- a/testcases/kernel/syscalls/getcpu/getcpu01.c
>> +++ b/testcases/kernel/syscalls/getcpu/getcpu01.c
>> @@ -15,20 +15,16 @@
>>  #include <stdio.h>
>>  #include <stdlib.h>
>>  #include <sys/types.h>
>> +#include "tst_test.h"
>>  #include "lapi/syscalls.h"
>>  #include "lapi/cpuset.h"
>> -#include "tst_test.h"
>>  #include "config.h"
>>  
>>  static inline int get_cpu(unsigned *cpu_id,
>> -			  unsigned *node_id LTP_ATTRIBUTE_UNUSED,
>> -			  void *cache_struct LTP_ATTRIBUTE_UNUSED)
>> +			  unsigned *node_id)
>>  {
>> -#ifndef HAVE_SCHED_GETCPU
>> -	return tst_syscall(__NR_getcpu, cpu_id, node_id, cache_struct);
>> -#else
>> -	*cpu_id = sched_getcpu();
>> -#endif
>> +	return tst_syscall(__NR_getcpu, cpu_id, node_id, NULL);
>
> The call is mostly implemneted as VDSO so it would make much more sense
> to test the libc function instead and test the implementation that is
> actually used in production.
>
>>  	return 0;
>>  }
>>  
>> @@ -78,7 +74,7 @@ realloc:
>>  	return cpu_max;
>>  }
>>  
>> -#ifdef __i386__
>> +#if  defined(__i386__) || defined(__x86_64__)
>>  static unsigned int get_nodeid(unsigned int cpu_id)
>>  {
>>  	DIR *directory_parent, *directory_node;
>> @@ -125,22 +121,22 @@ static void run(void)
>>  {
>>  	unsigned int cpu_id, node_id = 0;
>>  	unsigned int cpu_set;
>
>
> The get_nodeid() just parses sysfs, that should be portable, can't we
> just get rid of the ifdefs completely instead?

That's what I thought too. I Will remove them.

>
>> -#ifdef __i386__
>> +#if defined(__i386__) || defined(__x86_64__)
>>  	unsigned int node_set;
>>  #endif
>>  
>>  	cpu_set = set_cpu_affinity();
>> -#ifdef __i386__
>> +#if defined(__i386__) || defined(__x86_64__)
>>  	node_set = get_nodeid(cpu_set);
>>  #endif
>>  
>> -	TEST(get_cpu(&cpu_id, &node_id, NULL));
>> +	TEST(get_cpu(&cpu_id, &node_id));
>>  	if (TST_RET == 0) {
>>  		if (cpu_id != cpu_set)
>>  			tst_res(TFAIL, "getcpu() returned wrong value"
>>  				" expected cpuid:%d, returned value cpuid: %d",
>>  				cpu_set, cpu_id);
>> -#ifdef __i386__
>> +#if defined(__i386__) || defined(__x86_64__)
>>  		else if (node_id != node_set)
>>  			tst_res(TFAIL, "getcpu() returned wrong value"
>>  				" expected  node id:%d returned  node id:%d",
>> -- 
>> 2.38.1
>> 
>> 
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp

Also I guess the kver check for 2.6 can be removed.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] getcpu01: Reinstate node_id test

[Petr Vorel]

Hi all,

...
> Also I guess the kver check for 2.6 can be removed.

This is part o Xu's patchset, I'd keep it there (otherwise that patchset will
not be applicable):
https://patchwork.ozlabs.org/project/ltp/patch/1670845229-1981-3-git-send-email-xuyang2018.jy@fujitsu.com/

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp