| Message ID | 20220303145032.21493-6-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ksm06 and libnuma cleanups and fixes | expand |
On Thu, Mar 3, 2022 at 10:49 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > testcases/kernel/mem/ksm/ksm06.c | 28 +++------------------------- > 1 file changed, 3 insertions(+), 25 deletions(-) > > diff --git a/testcases/kernel/mem/ksm/ksm06.c > b/testcases/kernel/mem/ksm/ksm06.c > index 61507b2aa..f5f7319d7 100644 > --- a/testcases/kernel/mem/ksm/ksm06.c > +++ b/testcases/kernel/mem/ksm/ksm06.c > @@ -39,9 +39,6 @@ > #ifdef HAVE_NUMA_V2 > #include <numaif.h> > > -static int run = -1; > -static int sleep_millisecs = -1; > -static int merge_across_nodes = -1; > static unsigned long nr_pages = 100; > > static char *n_opt; > @@ -141,27 +138,6 @@ static void setup(void) > > if (n_opt) > nr_pages = SAFE_STRTOUL(n_opt, 0, ULONG_MAX); > - > - /* save the current value */ > - SAFE_FILE_SCANF(PATH_KSM "run", "%d", &run); > - SAFE_FILE_SCANF(PATH_KSM "merge_across_nodes", > - "%d", &merge_across_nodes); > - SAFE_FILE_SCANF(PATH_KSM "sleep_millisecs", > - "%d", &sleep_millisecs); > -} > - > -static void cleanup(void) > -{ > - if (merge_across_nodes != -1) { > - FILE_PRINTF(PATH_KSM "merge_across_nodes", > - "%d", merge_across_nodes); > - } > - > - if (sleep_millisecs != -1) > - FILE_PRINTF(PATH_KSM "sleep_millisecs", "%d", > sleep_millisecs); > - > - if (run != -1) > - FILE_PRINTF(PATH_KSM "run", "%d", run); > } > > static struct tst_test test = { > @@ -171,9 +147,11 @@ static struct tst_test test = { > {} > }, > .setup = setup, > - .cleanup = cleanup, > .save_restore = (const char * const[]) { > "?/sys/kernel/mm/ksm/max_page_sharing", > The mem library verifies the max_page_sharing validity before setting because some old kernels do not have it. Thus it is fine to use the prefix '?'. + "?/sys/kernel/mm/ksm/run", > + "?/sys/kernel/mm/ksm/merge_across_nodes", > + "?/sys/kernel/mm/ksm/sleep_millisecs", > But for the two knobs(run, sleep_millisecs) that should exist unless the kernel disables KSM. So here we'd better start with prefix '!' and add .needs_kconfg for ‘CONFIG_KSM=y' check. (This also fit for other ksm tests) For 'merge_across_nodes', we don't need any prefix because ksm06 is actually relying on it, otherwise TCONF is expected. Thus it's fine to remove the file check from setup() as well.
Hi! > The mem library verifies the max_page_sharing validity before > setting because some old kernels do not have it. Thus it is fine > to use the prefix '?'. > > + "?/sys/kernel/mm/ksm/run", > > + "?/sys/kernel/mm/ksm/merge_across_nodes", > > + "?/sys/kernel/mm/ksm/sleep_millisecs", > > > > > But for the two knobs(run, sleep_millisecs) that should exist unless > the kernel disables KSM. So here we'd better start with prefix '!' and > add .needs_kconfg for ???CONFIG_KSM=y' check. > (This also fit for other ksm tests) I guess that if we put ! before the merge_across_nodes that would cause TBROK on systems without CONFIG_NUMA or kernels without that feature. So what about just removing the question marks there and adding .need_kconfigs for KSM and NUMA?
On Fri, Mar 4, 2022 at 8:00 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > The mem library verifies the max_page_sharing validity before > > setting because some old kernels do not have it. Thus it is fine > > to use the prefix '?'. > > > > + "?/sys/kernel/mm/ksm/run", > > > + "?/sys/kernel/mm/ksm/merge_across_nodes", > > > + "?/sys/kernel/mm/ksm/sleep_millisecs", > > > > > > > > > But for the two knobs(run, sleep_millisecs) that should exist unless > > the kernel disables KSM. So here we'd better start with prefix '!' and > > add .needs_kconfg for ???CONFIG_KSM=y' check. > > (This also fit for other ksm tests) > > I guess that if we put ! before the merge_across_nodes that would cause > TBROK on systems without CONFIG_NUMA or kernels without that feature. > > So what about just removing the question marks there and adding > .need_kconfigs for KSM and NUMA? > Er, that's exactly what I meant in the last email, maybe you overlooked the last sentence:). i.e. " prefix ! for 'run' and 'sleep_milisecs' no prefix for 'merge_across_nodes' .need_kconfigs for KSM and NUMA "
Hi! > > > But for the two knobs(run, sleep_millisecs) that should exist unless > > > the kernel disables KSM. So here we'd better start with prefix '!' and > > > add .needs_kconfg for ???CONFIG_KSM=y' check. > > > (This also fit for other ksm tests) > > > > I guess that if we put ! before the merge_across_nodes that would cause > > TBROK on systems without CONFIG_NUMA or kernels without that feature. > > > > So what about just removing the question marks there and adding > > .need_kconfigs for KSM and NUMA? > > > > Er, that's exactly what I meant in the last email, maybe you overlooked > the last sentence:). > > i.e. > > " > prefix ! for 'run' and 'sleep_milisecs' > no prefix for 'merge_across_nodes' > .need_kconfigs for KSM and NUMA > " Ah, right, sorry. What about this: diff --git a/testcases/kernel/mem/ksm/ksm06.c b/testcases/kernel/mem/ksm/ksm06.c index 61507b2aa..e734786c1 100644 --- a/testcases/kernel/mem/ksm/ksm06.c +++ b/testcases/kernel/mem/ksm/ksm06.c @@ -39,9 +39,6 @@ #ifdef HAVE_NUMA_V2 #include <numaif.h> -static int run = -1; -static int sleep_millisecs = -1; -static int merge_across_nodes = -1; static unsigned long nr_pages = 100; static char *n_opt; @@ -133,35 +130,11 @@ static void test_ksm(void) static void setup(void) { - if (access(PATH_KSM "merge_across_nodes", F_OK) == -1) - tst_brk(TCONF, "no merge_across_nodes sysfs knob"); - if (!is_numa(NULL, NH_MEMS, 2)) tst_brk(TCONF, "The case needs a NUMA system."); if (n_opt) nr_pages = SAFE_STRTOUL(n_opt, 0, ULONG_MAX); - - /* save the current value */ - SAFE_FILE_SCANF(PATH_KSM "run", "%d", &run); - SAFE_FILE_SCANF(PATH_KSM "merge_across_nodes", - "%d", &merge_across_nodes); - SAFE_FILE_SCANF(PATH_KSM "sleep_millisecs", - "%d", &sleep_millisecs); -} - -static void cleanup(void) -{ - if (merge_across_nodes != -1) { - FILE_PRINTF(PATH_KSM "merge_across_nodes", - "%d", merge_across_nodes); - } - - if (sleep_millisecs != -1) - FILE_PRINTF(PATH_KSM "sleep_millisecs", "%d", sleep_millisecs); - - if (run != -1) - FILE_PRINTF(PATH_KSM "run", "%d", run); } static struct tst_test test = { @@ -171,11 +144,18 @@ static struct tst_test test = { {} }, .setup = setup, - .cleanup = cleanup, .save_restore = (const char * const[]) { "?/sys/kernel/mm/ksm/max_page_sharing", + "!/sys/kernel/mm/ksm/run", + "!/sys/kernel/mm/ksm/sleep_millisecs", + "/sys/kernel/mm/ksm/merge_across_nodes", NULL, }, + .needs_kconfigs = (const char *const[]){ + "CONFIG_KSM=y", + "CONFIG_NUMA=y", + NULL + }, .test_all = test_ksm, }; If we add merge_across_nodes without any prefix we can as well remove the check for the file existence in the test setup.
On Mon, Mar 7, 2022 at 4:56 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > > But for the two knobs(run, sleep_millisecs) that should exist unless > > > > the kernel disables KSM. So here we'd better start with prefix '!' > and > > > > add .needs_kconfg for ???CONFIG_KSM=y' check. > > > > (This also fit for other ksm tests) > > > > > > I guess that if we put ! before the merge_across_nodes that would cause > > > TBROK on systems without CONFIG_NUMA or kernels without that feature. > > > > > > So what about just removing the question marks there and adding > > > .need_kconfigs for KSM and NUMA? > > > > > > > Er, that's exactly what I meant in the last email, maybe you overlooked > > the last sentence:). > > > > i.e. > > > > " > > prefix ! for 'run' and 'sleep_milisecs' > > no prefix for 'merge_across_nodes' > > .need_kconfigs for KSM and NUMA > > " > > Ah, right, sorry. What about this: > > diff --git a/testcases/kernel/mem/ksm/ksm06.c > b/testcases/kernel/mem/ksm/ksm06.c > index 61507b2aa..e734786c1 100644 > --- a/testcases/kernel/mem/ksm/ksm06.c > +++ b/testcases/kernel/mem/ksm/ksm06.c > @@ -39,9 +39,6 @@ > #ifdef HAVE_NUMA_V2 > #include <numaif.h> > > -static int run = -1; > -static int sleep_millisecs = -1; > -static int merge_across_nodes = -1; > static unsigned long nr_pages = 100; > > static char *n_opt; > @@ -133,35 +130,11 @@ static void test_ksm(void) > > static void setup(void) > { > - if (access(PATH_KSM "merge_across_nodes", F_OK) == -1) > - tst_brk(TCONF, "no merge_across_nodes sysfs knob"); > - > if (!is_numa(NULL, NH_MEMS, 2)) > tst_brk(TCONF, "The case needs a NUMA system."); > > if (n_opt) > nr_pages = SAFE_STRTOUL(n_opt, 0, ULONG_MAX); > - > - /* save the current value */ > - SAFE_FILE_SCANF(PATH_KSM "run", "%d", &run); > - SAFE_FILE_SCANF(PATH_KSM "merge_across_nodes", > - "%d", &merge_across_nodes); > - SAFE_FILE_SCANF(PATH_KSM "sleep_millisecs", > - "%d", &sleep_millisecs); > -} > - > -static void cleanup(void) > -{ > - if (merge_across_nodes != -1) { > - FILE_PRINTF(PATH_KSM "merge_across_nodes", > - "%d", merge_across_nodes); > - } > - > - if (sleep_millisecs != -1) > - FILE_PRINTF(PATH_KSM "sleep_millisecs", "%d", > sleep_millisecs); > - > - if (run != -1) > - FILE_PRINTF(PATH_KSM "run", "%d", run); > } > > static struct tst_test test = { > @@ -171,11 +144,18 @@ static struct tst_test test = { > {} > }, > .setup = setup, > - .cleanup = cleanup, > .save_restore = (const char * const[]) { > "?/sys/kernel/mm/ksm/max_page_sharing", > + "!/sys/kernel/mm/ksm/run", > + "!/sys/kernel/mm/ksm/sleep_millisecs", > + "/sys/kernel/mm/ksm/merge_across_nodes", > NULL, > }, > + .needs_kconfigs = (const char *const[]){ > + "CONFIG_KSM=y", > + "CONFIG_NUMA=y", > + NULL > + }, > .test_all = test_ksm, > }; > > > If we add merge_across_nodes without any prefix we can as well remove > the check for the file existence in the test setup. > ACK.
Hi!
> ACK.
Thanks I will push the patchset with the rest of the fixes you have
suggested.
diff --git a/testcases/kernel/mem/ksm/ksm06.c b/testcases/kernel/mem/ksm/ksm06.c index 61507b2aa..f5f7319d7 100644 --- a/testcases/kernel/mem/ksm/ksm06.c +++ b/testcases/kernel/mem/ksm/ksm06.c @@ -39,9 +39,6 @@ #ifdef HAVE_NUMA_V2 #include <numaif.h> -static int run = -1; -static int sleep_millisecs = -1; -static int merge_across_nodes = -1; static unsigned long nr_pages = 100; static char *n_opt; @@ -141,27 +138,6 @@ static void setup(void) if (n_opt) nr_pages = SAFE_STRTOUL(n_opt, 0, ULONG_MAX); - - /* save the current value */ - SAFE_FILE_SCANF(PATH_KSM "run", "%d", &run); - SAFE_FILE_SCANF(PATH_KSM "merge_across_nodes", - "%d", &merge_across_nodes); - SAFE_FILE_SCANF(PATH_KSM "sleep_millisecs", - "%d", &sleep_millisecs); -} - -static void cleanup(void) -{ - if (merge_across_nodes != -1) { - FILE_PRINTF(PATH_KSM "merge_across_nodes", - "%d", merge_across_nodes); - } - - if (sleep_millisecs != -1) - FILE_PRINTF(PATH_KSM "sleep_millisecs", "%d", sleep_millisecs); - - if (run != -1) - FILE_PRINTF(PATH_KSM "run", "%d", run); } static struct tst_test test = { @@ -171,9 +147,11 @@ static struct tst_test test = { {} }, .setup = setup, - .cleanup = cleanup, .save_restore = (const char * const[]) { "?/sys/kernel/mm/ksm/max_page_sharing", + "?/sys/kernel/mm/ksm/run", + "?/sys/kernel/mm/ksm/merge_across_nodes", + "?/sys/kernel/mm/ksm/sleep_millisecs", NULL, }, .test_all = test_ksm,
Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- testcases/kernel/mem/ksm/ksm06.c | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-)