| Message ID | 20221206165840.12107-1-rpalethorpe@suse.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | getcpu01: Reinstate node_id test | expand |
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
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
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.
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
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",
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(-)