[LTP] [PATCH] migrate_pages02: check file exist before opening it

[LTP] [PATCH] migrate_pages02: check file exist before opening it

[Li Wang]

This test reports a warning on some special kernel (with NUMA BALANCE
disabling). The main reason is that prefix '?' makes tst_sys_conf_save
only silent ignores non-exist paths. But seems we still open it in other
places (e.g. in setup function).

  tst_sys_conf.c:58: TINFO: Path not found: '/proc/sys/kernel/numa_balancing'
  tst_test.c:1365: TINFO: Timeout per run is 0h 05m 00s
  migrate_pages02.c:279: TWARN: Failed to open FILE '/proc/sys/kernel/numa_balancing'
  migrate_pages02.c:317: TINFO: Using nodes: 0 1

Signed-off-by: Li Wang <liwang@redhat.com>
---

Notes:
    Looks like we have many tests use the prefix '?' for knob save/restore,
    but none of them check if the paths are valid when opening at other places,
    by now, I just get informed that migrate_pages02 threw a warning.
    
    I'm thinking maybe we can keep the return value of tst_sys_conf_save
    as a check condition before opening the file, but this is not fit for
    test with many paths saving:
    e.g.
        add_key05.c-	"?/proc/sys/kernel/keys/gc_delay",
        add_key05.c-	"?/proc/sys/kernel/keys/maxkeys",

 testcases/kernel/syscalls/migrate_pages/migrate_pages02.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
index 485a1c5aa..309e707bc 100644
--- a/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
+++ b/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c
@@ -37,6 +37,8 @@
  */
 #define NODE_MIN_FREEMEM (32*1024*1024)
 
+#define NUMA_BALANCING "/proc/sys/kernel/numa_balancing"
+
 #ifdef HAVE_NUMA_V2
 
 static const char nobody_uid[] = "nobody";
@@ -276,7 +278,8 @@ static void setup(void)
 	else if (tst_kvercmp(2, 6, 18) < 0)
 		tst_brk(TCONF, "2.6.18 or greater kernel required");
 
-	FILE_PRINTF("/proc/sys/kernel/numa_balancing", "0");
+	if (access(NUMA_BALANCING, F_OK) == 0)
+		FILE_PRINTF(NUMA_BALANCING, "0");
 	/*
 	 * find 2 nodes, which can hold NODE_MIN_FREEMEM bytes
 	 * The reason is that:
@@ -328,7 +331,7 @@ static struct tst_test test = {
 	.test_all = run,
 	.setup = setup,
 	.save_restore = (const char * const[]) {
-		"?/proc/sys/kernel/numa_balancing",
+		"?NUMA_BALANCING",
 		NULL,
 	},
 };
-- 
2.31.1


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

Re: [LTP] [PATCH] migrate_pages02: check file exist before opening it

[Cyril Hrubis]

Hi!
> This test reports a warning on some special kernel (with NUMA BALANCE
> disabling). The main reason is that prefix '?' makes tst_sys_conf_save
> only silent ignores non-exist paths. But seems we still open it in other
> places (e.g. in setup function).
> 
>   tst_sys_conf.c:58: TINFO: Path not found: '/proc/sys/kernel/numa_balancing'
>   tst_test.c:1365: TINFO: Timeout per run is 0h 05m 00s
>   migrate_pages02.c:279: TWARN: Failed to open FILE '/proc/sys/kernel/numa_balancing'
>   migrate_pages02.c:317: TINFO: Using nodes: 0 1
> 
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
> 
> Notes:
>     Looks like we have many tests use the prefix '?' for knob save/restore,
>     but none of them check if the paths are valid when opening at other places,
>     by now, I just get informed that migrate_pages02 threw a warning.
>     
>     I'm thinking maybe we can keep the return value of tst_sys_conf_save
>     as a check condition before opening the file, but this is not fit for
>     test with many paths saving:
>     e.g.
>         add_key05.c-	"?/proc/sys/kernel/keys/gc_delay",
>         add_key05.c-	"?/proc/sys/kernel/keys/maxkeys",

Looking at the test we set these values to make the test run faster and
the test works fine even without these knobs, right?

Maybe we just need to enhance the .save_restore with another parameter;
a value to set the knob to if it does exists, so in add_key05.c it would
look like:

struct tst_path_val {
	const char *path;
	const char *val;
};

        .save_restore = (const struct tst_path_val const[]) {
                {"?/proc/sys/kernel/keys/gc_delay", "1"},
                {"?/proc/sys/kernel/keys/maxkeys", "200"},
                {"?/proc/sys/kernel/keys/maxbytes", "20000"}
                NULL,
        },

And in cases we do not need to set value we would just pass NULL as val...

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] migrate_pages02: check file exist before opening it

[Jan Stancek]

> +               "?NUMA_BALANCING",
You probably meant "?"NUMA_BALANCING

>
> Maybe we just need to enhance the .save_restore with another parameter;
> a value to set the knob to if it does exists, so in add_key05.c it would
> look like:
>
> struct tst_path_val {
>         const char *path;
>         const char *val;
> };
>
>         .save_restore = (const struct tst_path_val const[]) {
>                 {"?/proc/sys/kernel/keys/gc_delay", "1"},
>                 {"?/proc/sys/kernel/keys/maxkeys", "200"},
>                 {"?/proc/sys/kernel/keys/maxbytes", "20000"}
>                 NULL,
>         },
>
> And in cases we do not need to set value we would just pass NULL as val...

That should work for most testcases, I don't recall one where we need
to cycle through more values.


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

Re: [LTP] [PATCH] migrate_pages02: check file exist before opening it

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1243 bytes --]

On Mon, Mar 7, 2022 at 2:51 AM Jan Stancek <jstancek@redhat.com> wrote:

> > +               "?NUMA_BALANCING",
> You probably meant "?"NUMA_BALANCING
>

I guess both should be fine.
(tried locally with original patch and works well)


>
> >
> > Maybe we just need to enhance the .save_restore with another parameter;
> > a value to set the knob to if it does exists, so in add_key05.c it would
> > look like:
> >
> > struct tst_path_val {
> >         const char *path;
> >         const char *val;
> > };
> >
> >         .save_restore = (const struct tst_path_val const[]) {
> >                 {"?/proc/sys/kernel/keys/gc_delay", "1"},
> >                 {"?/proc/sys/kernel/keys/maxkeys", "200"},
> >                 {"?/proc/sys/kernel/keys/maxbytes", "20000"}
> >                 NULL,
> >         },
> >
> > And in cases we do not need to set value we would just pass NULL as
> val...
>

+1



>
> That should work for most testcases, I don't recall one where we need
> to cycle through more values.
>

Agree, even we have that requirement in the future, pass NULL at the
beginning
and reset it via FILE_PRINTF for more cycle values that should be working.
Thanks for the suggestions, I will get start work on V2.

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2747 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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