numa_helper: don't break is_numa() with TCONF

PATCH New Jan Stancek 2 participants 5 messages Feb 23, 2016 – Mar 3, 2016

Share & tools

Jan Stancek

author

2016-02-23 10:00 UTC #0 lore raw

PATCH v2 1/2 numa_helper: don't break is_numa() with TCONF

Li reported, that some oom tests on aarch64 exit with TCONF during is_numa() call, because get_mempolicy syscall is not implemented. And because numa_helper used ltp_syscall() it terminated the test in all instances.

Make sure we check that NUMA is available first with numa_available(). In case it's not supported return empty set of nodes.

Also ignore ENOSYS from get_mempolicy. At the moment this is redundant, because numa_available() implementation is based on availability of get_mempolicy as well, but extra check doesn't hurt in case it would change in future.

Reported-by Li Wang <[email protected]>

Signed-off-by Jan Stancek <[email protected]>

---
 testcases/kernel/lib/numa_helper.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

testcases/kernel/lib/numa_helper.c +18 -5

index 0074180991ec..82b1978f7d8a 100644
@@ -81,10 +81,19 @@ static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long max_node)
 	 * avoid numa_get_mems_allowed(), because of bug in getpol()
 	 * utility function in older versions:
 	 * http://www.spinics.net/lists/linux-numa/msg00849.html
+	 *
+	 * At the moment numa_available() implementation also uses
+	 * get_mempolicy, but let's make explicit check for ENOSYS
+	 * here as well in case it changes in future. Silent ignore
+	 * of ENOSYS is OK, because without NUMA caller gets empty
+	 * set of nodes anyway.
 	 */
-	if (ltp_syscall(__NR_get_mempolicy, NULL, nodemask->n,
-		    max_node, 0, MPOL_F_MEMS_ALLOWED) < 0)
+	if (syscall(__NR_get_mempolicy, NULL, nodemask->n,
+		    max_node, 0, MPOL_F_MEMS_ALLOWED) < 0) {
+		if (errno == ENOSYS)
+			return 0;
 		return -2;
+	}
 #else
 	int i;
 	/*

@@ -163,9 +172,13 @@ int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes)
 		*nodes = NULL;
 
 #if HAVE_NUMA_H
-	unsigned long max_node = LTP_ALIGN(get_max_node(),
-						sizeof(unsigned long)*8);
-	unsigned long nodemask_size = max_node / 8;
+	unsigned long max_node, nodemask_size;
+
+	if (numa_available() == -1)
+		return 0;
+
+	max_node = LTP_ALIGN(get_max_node(), sizeof(unsigned long)*8);
+	nodemask_size = max_node / 8;
 
 	nodemask = malloc(nodemask_size);
 	if (nodes)

Signature · 1 line

1.8.3.1

Jan Stancek

author

2016-02-23 10:00 UTC #1 lore raw

PATCH v2 2/2 numa_helper: add min_nodes parameter to is_numa()

get_allowed_nodes() can fail if callers request more nodes than are available. This patch changes is_numa() by adding a parameter to it, which allows to easily check that system supports NUMA and has at least certain number of nodes and it also updates tests that used get_allowed_nodes() without checking node count first.

Signed-off-by Jan Stancek <[email protected]>

---
 testcases/kernel/include/numa_helper.h                    | 2 +-
 testcases/kernel/lib/numa_helper.c                        | 9 +++++----
 testcases/kernel/mem/ksm/ksm06.c                          | 4 ++--
 testcases/kernel/mem/oom/oom02.c                          | 2 +-
 testcases/kernel/mem/oom/oom03.c                          | 2 +-
 testcases/kernel/mem/oom/oom04.c                          | 5 ++++-
 testcases/kernel/mem/oom/oom05.c                          | 5 ++++-
 testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c | 3 +++
 testcases/kernel/syscalls/mbind/mbind01.c                 | 3 +++
 testcases/kernel/syscalls/migrate_pages/migrate_pages01.c | 4 ++--
 10 files changed, 26 insertions(+), 13 deletions(-)

testcases/kernel/include/numa_helper.h +1 -1

index 3cf65d5638c4..a1eb593ea70c 100644
@@ -34,6 +34,6 @@ unsigned long get_max_node(void);
 int get_allowed_nodes_arr(int flag, int *num_nodes, int **nodes);
 int get_allowed_nodes(int flag, int count, ...);
 void nh_dump_nodes(void);
-int is_numa(void (*cleanup_fn)(void));
+int is_numa(void (*cleanup_fn)(void), int min_nodes);
 
 #endif

testcases/kernel/lib/numa_helper.c +5 -4

index 82b1978f7d8a..5b69bb0e0d5b 100644
@@ -281,14 +281,15 @@ void nh_dump_nodes(void)
 
 /*
  * is_numa - judge a system is NUMA system or not
- * NOTE: the function is designed to try to find more than
- *       1 available node, at least each node contains memory.
+ * @min_nodes: find at least 'min_nodes' nodes with memory
+ * NOTE: the function is designed to try to find at least 'min_nodes'
+ * available nodes, where each node contains memory.
  * WARN: Don't use this func in child, as it calls tst_brkm()
  * RETURNS:
  *     0 - it's not a NUMA system
  *     1 - it's a NUMA system
  */
-int is_numa(void (*cleanup_fn)(void))
+int is_numa(void (*cleanup_fn)(void), int min_nodes)
 {
 	int ret;
 	int numa_nodes = 0;

@@ -297,7 +298,7 @@ int is_numa(void (*cleanup_fn)(void))
 	if (ret < 0)
 		tst_brkm(TBROK | TERRNO, cleanup_fn, "get_allowed_nodes_arr");
 
-	if (numa_nodes > 1)
+	if (numa_nodes >= min_nodes)
 		return 1;
 	else
 		return 0;

testcases/kernel/mem/ksm/ksm06.c +2 -2

index 1cda01fe61b8..fc4d5b5bd825 100644
@@ -95,8 +95,8 @@ void setup(void)
 	if (access(PATH_KSM "merge_across_nodes", F_OK) == -1)
 		tst_brkm(TCONF, NULL, "no merge_across_nodes sysfs knob");
 
-	if (!is_numa(NULL))
-		tst_brkm(TCONF, NULL, "The case need a NUMA system.");
+	if (!is_numa(NULL, 2))
+		tst_brkm(TCONF, NULL, "The case needs a NUMA system.");
 
 	/* save the current value */
 	SAFE_FILE_SCANF(NULL, PATH_KSM "run", "%d", &run);

testcases/kernel/mem/oom/oom02.c +1 -1

index 48bd0a3ad632..53acc1534a7c 100644
@@ -79,7 +79,7 @@ void setup(void)
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 	TEST_PAUSE;
 
-	if (!is_numa(NULL))
+	if (!is_numa(NULL, 2))
 		tst_brkm(TCONF, NULL, "The case need a NUMA system.");
 
 	overcommit = get_sys_tune("overcommit_memory");

testcases/kernel/mem/oom/oom03.c +1 -1

index 3afc3dea48bf..06e6e90a346f 100644
@@ -77,7 +77,7 @@ int main(int argc, char *argv[])
 		}
 
 		/* OOM for MEMCG with mempolicy */
-		if (is_numa(cleanup)) {
+		if (is_numa(cleanup, 2)) {
 			tst_resm(TINFO, "OOM on MEMCG & mempolicy...");
 			testoom(MPOL_BIND, 0, ENOMEM, 1);
 			testoom(MPOL_INTERLEAVE, 0, ENOMEM, 1);

testcases/kernel/mem/oom/oom04.c +4 -1

index 0a315e110972..391cc1651c0e 100644
@@ -63,7 +63,7 @@ int main(int argc, char *argv[])
 		tst_resm(TINFO, "OOM on CPUSET...");
 		testoom(0, 0, ENOMEM, 1);
 
-		if (is_numa(cleanup)) {
+		if (is_numa(cleanup, 2)) {
 			/*
 			 * Under NUMA system, the migration of cpuset's memory
 			 * is in charge of cpuset.memory_migrate, we can write

@@ -87,6 +87,9 @@ void setup(void)
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 	TEST_PAUSE;
 
+	if (!is_numa(NULL, 1))
+		tst_brkm(TCONF, NULL, "requires NUMA with at least 1 node");
+
 	overcommit = get_sys_tune("overcommit_memory");
 	set_sys_tune("overcommit_memory", 1, 1);

testcases/kernel/mem/oom/oom05.c +4 -1

index 44a243e2f2bd..4b31d59e1a65 100644
@@ -69,7 +69,7 @@ int main(int argc, char *argv[])
 		 * is in charge of cpuset.memory_migrate, we can write
 		 * 1 to cpuset.memory_migrate to enable the migration.
 		 */
-		if (is_numa(cleanup)) {
+		if (is_numa(cleanup, 2)) {
 			write_cpuset_files(CPATH_NEW, "memory_migrate", "1");
 			tst_resm(TINFO, "OOM on CPUSET & MEMCG with "
 					"cpuset.memory_migrate=1");

@@ -110,6 +110,9 @@ void setup(void)
 	tst_sig(FORK, DEF_HANDLER, cleanup);
 	TEST_PAUSE;
 
+	if (!is_numa(NULL, 1))
+		tst_brkm(TCONF, NULL, "requires NUMA with at least 1 node");
+
 	overcommit = get_sys_tune("overcommit_memory");
 	set_sys_tune("overcommit_memory", 1, 1);

testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c +3 -0

index e4cff88a5b6d..c0f83f908467 100644
@@ -339,6 +339,9 @@ static void setup(void)
 	/* check syscall availability */
 	ltp_syscall(__NR_get_mempolicy, NULL, NULL, 0, NULL, 0);
 
+	if (!is_numa(NULL, 1))
+		tst_brkm(TCONF, NULL, "requires NUMA with at least 1 node");
+
 	TEST_PAUSE;
 	tst_tmpdir();
 }

testcases/kernel/syscalls/mbind/mbind01.c +3 -0

index d56f39e96c59..28506c50df36 100644
@@ -298,6 +298,9 @@ static void setup(void)
 	/* check syscall availability */
 	ltp_syscall(__NR_mbind, NULL, 0, 0, NULL, 0, 0);
 
+	if (!is_numa(NULL, 1))
+		tst_brkm(TCONF, NULL, "requires NUMA with at least 1 node");
+
 	TEST_PAUSE;
 	tst_tmpdir();
 }

testcases/kernel/syscalls/migrate_pages/migrate_pages01.c +2 -2

index 6484b40d72a5..ad520fc15ab9 100644
@@ -226,8 +226,8 @@ static void setup(void)
 	tst_require_root();
 	TEST(ltp_syscall(__NR_migrate_pages, 0, 0, NULL, NULL));
 
-	if (numa_available() == -1)
-		tst_brkm(TCONF, NULL, "NUMA not available");
+	if (!is_numa(NULL, 1))
+		tst_brkm(TCONF, NULL, "requires NUMA with at least 1 node");
 
 	ret = get_allowed_nodes(NH_MEMS, 1, &node);
 	if (ret < 0)

Signature · 1 line

1.8.3.1

Cyril Hrubis

2016-03-03 13:35 UTC #2 lore raw

PATCH v2 2/2 numa_helper: add min_nodes parameter to is_numa()

Hi!

Quoted 5 lines

get_allowed_nodes() can fail if callers request more nodes than are available. This patch changes is_numa() by adding a parameter to it, which allows to easily check that system supports NUMA and has at least certain number of nodes and it also updates tests that used get_allowed_nodes() without checking node count first.

Shouldn't we pass the NH_MEMS/NH_CPUS flags to the is_numa() as well?

Otherwise the patchset looks good to me.

Signature · 2 lines

Cyril Hrubis
[email protected]

Jan Stancek

author

2016-03-03 14:10 UTC #3 lore raw

PATCH v2 2/2 numa_helper: add min_nodes parameter to is_numa()

----- Original Message -----

Quoted 6 lines

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], [email protected]
Sent: Thursday, 3 March, 2016 2:35:48 PM
Subject: Re: [PATCH v2 2/2] numa_helper: add min_nodes parameter to is_numa()

Quoted 8 lines

Hi!
Quoted 5 lines
get_allowed_nodes() can fail if callers request more nodes than are available. This patch changes is_numa() by adding a parameter to it, which allows to easily check that system supports NUMA and has at least certain number of nodes and it also updates tests that used get_allowed_nodes() without checking node count first.
Shouldn't we pass the NH_MEMS/NH_CPUS flags to the is_numa() as well?

We probably should, we have maybe 1 or 2 tests that care about cpus, not just mem. I'll add it.

Quoted 2 lines

Otherwise the patchset looks good to me.

Thanks for review,
Jan

Quoted 5 lines

--
Cyril Hrubis
[email protected]

Jan Stancek

author

2016-03-03 14:59 UTC #4 lore raw

PATCH v2 2/2 numa_helper: add min_nodes parameter to is_numa()

----- Original Message -----

Quoted 23 lines

From: "Jan Stancek" <[email protected]>
To: "Cyril Hrubis" <[email protected]>
Cc: [email protected]
Sent: Thursday, 3 March, 2016 3:10:21 PM
Subject: Re: [LTP] [PATCH v2 2/2] numa_helper: add min_nodes parameter to is_numa()
----- Original Message -----
Quoted 14 lines
From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected], [email protected]
Sent: Thursday, 3 March, 2016 2:35:48 PM
Subject: Re: [PATCH v2 2/2] numa_helper: add min_nodes parameter to is_numa()
Hi!
Quoted 5 lines
get_allowed_nodes() can fail if callers request more nodes than are available. This patch changes is_numa() by adding a parameter to it, which allows to easily check that system supports NUMA and has at least certain number of nodes and it also updates tests that used get_allowed_nodes() without checking node count first.

Quoted 4 lines

Quoted 1 line
Shouldn't we pass the NH_MEMS/NH_CPUS flags to the is_numa() as well?
We probably should, we have maybe 1 or 2 tests that care about cpus, not just mem. I'll add it.

Pushed with added flag parameter.

Regards,
Jan

Quoted 16 lines

Quoted 3 lines
Otherwise the patchset looks good to me.
Thanks for review,
Jan
Quoted 5 lines
--
Cyril Hrubis
[email protected]
--
Mailing list info: http://lists.linux.it/listinfo/ltp