[LTP] [RFC][PATCH 0/2] Add safe_get_nodemap()

[LTP] [RFC][PATCH 0/2] Add safe_get_nodemap()

[Petr Vorel]

Hi,

not a huge improvement, but because all tst_get_nodemap() are in the
setup (and only ksm06.c allows input parameter) we could have struct
tst_test member which would call safe_get_nodemap().

e.g.:

.nodemap = (const struct tst_path_val) {
		.type = TST_NUMA_MEM
		.required = 2,
},

safe_get_nodemap(tst_test->nodemap->type,
	tst_test->nodemap->required * getpagesize() / 1024);

This would not work for non - page sized nodes, e.g.:
nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);

=> extra member would need to be added:

.nodemap = (const struct tst_path_val) {
		.type = TST_NUMA_MEM
		.required = 2,
		.size = PAGES_ALLOCATED, // default == 1
},

would call:

safe_get_nodemap(tst_test->nodemap->type,
	tst_test->nodemap->required * tst_test->nodemap->size * getpagesize() / 1024,
	tst_test->nodemap->required);

Petr Vorel (2):
  libs: Add safe_get_nodemap()
  Use safe_get_nodemap()

 include/tst_numa.h                            | 41 ++++++++++++++++++-
 libs/numa/tst_numa.c                          | 14 +++++++
 testcases/kernel/mem/ksm/ksm06.c              |  4 +-
 .../syscalls/get_mempolicy/get_mempolicy01.c  |  4 +-
 .../syscalls/get_mempolicy/get_mempolicy02.c  |  4 +-
 testcases/kernel/syscalls/mbind/mbind02.c     |  4 +-
 testcases/kernel/syscalls/mbind/mbind03.c     |  4 +-
 testcases/kernel/syscalls/mbind/mbind04.c     |  4 +-
 .../syscalls/set_mempolicy/set_mempolicy01.c  |  4 +-
 .../syscalls/set_mempolicy/set_mempolicy02.c  |  4 +-
 .../syscalls/set_mempolicy/set_mempolicy03.c  |  4 +-
 .../syscalls/set_mempolicy/set_mempolicy04.c  |  4 +-
 12 files changed, 63 insertions(+), 32 deletions(-)

-- 
2.49.0


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

[LTP] [RFC][PATCH 1/2] libs: Add safe_get_nodemap()

[Petr Vorel]

This requires to add also tst_numa_type_name().

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_numa.h   | 41 +++++++++++++++++++++++++++++++++++++++--
 libs/numa/tst_numa.c | 14 ++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/include/tst_numa.h b/include/tst_numa.h
index a1f9616300..191a57fa6b 100644
--- a/include/tst_numa.h
+++ b/include/tst_numa.h
@@ -106,8 +106,30 @@ enum tst_numa_types {
 };
 
 /**
- * tst_get_nodemap() - Allocates and returns numa node map, which is an array of numa nodes which
- * contain desired resources e.g. memory.
+ * tst_numa_type_name() - Convert enum tst_numa_types to a description of a NUMA type.
+ *
+ * @type Bitflags of enum tst_numa_types specifying desired resources.
+ *
+ * @return a string describing a NUMA type.
+ */
+static inline const char *tst_numa_type_name(int type)
+{
+	if (type & ~(TST_NUMA_MEM))
+		tst_brk(TBROK, "Invalid type %i\n", type);
+
+	switch (type) {
+	case TST_NUMA_MEM:
+		return " memory";
+	case TST_NUMA_ANY:
+		return "";
+	default:
+		return " unknown";
+	}
+}
+
+/**
+ * tst_get_nodemap() - Allocates and returns numa node map, which is an array of
+ * NUMA nodes which contain desired resources e.g. memory.
  *
  * @type:       Bitflags of enum tst_numa_types specifying desired resources.
  * @min_mem_kb: Minimal free RAM on memory nodes, if given node has less than
@@ -119,4 +141,19 @@ enum tst_numa_types {
  */
 struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb);
 
+/**
+ * safe_get_nodemap() - Calls tst_get_nodemap() with check for minimal required
+ * NUMA nodes. Call tst_brk(TCONF) if not enough NUMA nodes.
+ *
+ * @type:       Bitflags of enum tst_numa_types specifying desired resources.
+ * @min_mem_kb: Minimal free RAM on memory nodes, if given node has less than
+ *             requested amount of free+buffers memory it's not included in
+ *             the resulting list of nodes.
+ * @required:   Minimal number of a required NUMA nodes.
+ *
+ * return: On success returns allocated and initialized struct tst_nodemap which contains
+ *         array of numa node ids that contains desired resources.
+ */
+struct tst_nodemap *safe_get_nodemap(int type, size_t min_mem_kb, size_t required);
+
 #endif /* TST_NUMA_H__ */
diff --git a/libs/numa/tst_numa.c b/libs/numa/tst_numa.c
index c3297013be..f004e29014 100644
--- a/libs/numa/tst_numa.c
+++ b/libs/numa/tst_numa.c
@@ -221,4 +221,18 @@ struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb)
 	return nodes;
 }
 
+struct tst_nodemap *safe_get_nodemap(int type, size_t min_mem_kb, size_t required)
+{
+	struct tst_nodemap *nodes;
+
+	nodes = tst_get_nodemap(type, min_mem_kb);
+
+	if (nodes->cnt < required) {
+		tst_brk(TCONF, "Test requires at least %zi NUMA%s node%s",
+				required, tst_numa_type_name(type), required > 1 ? "s" : "");
+	}
+
+	return nodes;
+}
+
 #endif
-- 
2.49.0


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

Re: [LTP] [RFC][PATCH 1/2] libs: Add safe_get_nodemap()

[Cyril Hrubis]

On Fri, Mar 28, 2025 at 12:43:10PM +0100, Petr Vorel wrote:
> This requires to add also tst_numa_type_name().
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  include/tst_numa.h   | 41 +++++++++++++++++++++++++++++++++++++++--
>  libs/numa/tst_numa.c | 14 ++++++++++++++
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/include/tst_numa.h b/include/tst_numa.h
> index a1f9616300..191a57fa6b 100644
> --- a/include/tst_numa.h
> +++ b/include/tst_numa.h
> @@ -106,8 +106,30 @@ enum tst_numa_types {
>  };
>  
>  /**
> - * tst_get_nodemap() - Allocates and returns numa node map, which is an array of numa nodes which
> - * contain desired resources e.g. memory.
> + * tst_numa_type_name() - Convert enum tst_numa_types to a description of a NUMA type.
> + *
> + * @type Bitflags of enum tst_numa_types specifying desired resources.
> + *
> + * @return a string describing a NUMA type.
> + */
> +static inline const char *tst_numa_type_name(int type)
> +{
> +	if (type & ~(TST_NUMA_MEM))
> +		tst_brk(TBROK, "Invalid type %i\n", type);
> +
> +	switch (type) {
> +	case TST_NUMA_MEM:
> +		return " memory";
> +	case TST_NUMA_ANY:
> +		return "";
> +	default:
> +		return " unknown";
> +	}
> +}
> +
> +/**
> + * tst_get_nodemap() - Allocates and returns numa node map, which is an array of
> + * NUMA nodes which contain desired resources e.g. memory.
>   *
>   * @type:       Bitflags of enum tst_numa_types specifying desired resources.
>   * @min_mem_kb: Minimal free RAM on memory nodes, if given node has less than
> @@ -119,4 +141,19 @@ enum tst_numa_types {
>   */
>  struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb);
>  
> +/**
> + * safe_get_nodemap() - Calls tst_get_nodemap() with check for minimal required
> + * NUMA nodes. Call tst_brk(TCONF) if not enough NUMA nodes.
> + *
> + * @type:       Bitflags of enum tst_numa_types specifying desired resources.
> + * @min_mem_kb: Minimal free RAM on memory nodes, if given node has less than
> + *             requested amount of free+buffers memory it's not included in
> + *             the resulting list of nodes.
> + * @required:   Minimal number of a required NUMA nodes.
> + *
> + * return: On success returns allocated and initialized struct tst_nodemap which contains
> + *         array of numa node ids that contains desired resources.
> + */
> +struct tst_nodemap *safe_get_nodemap(int type, size_t min_mem_kb, size_t required);
> +
>  #endif /* TST_NUMA_H__ */
> diff --git a/libs/numa/tst_numa.c b/libs/numa/tst_numa.c
> index c3297013be..f004e29014 100644
> --- a/libs/numa/tst_numa.c
> +++ b/libs/numa/tst_numa.c
> @@ -221,4 +221,18 @@ struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb)
>  	return nodes;
>  }
>  
> +struct tst_nodemap *safe_get_nodemap(int type, size_t min_mem_kb, size_t required)
> +{
> +	struct tst_nodemap *nodes;
> +
> +	nodes = tst_get_nodemap(type, min_mem_kb);
> +
> +	if (nodes->cnt < required) {
> +		tst_brk(TCONF, "Test requires at least %zi NUMA%s node%s",
> +				required, tst_numa_type_name(type), required > 1 ? "s" : "");
                                                                     ^
				     It does not make any sense to
				     request a NUMA with at least 1
				     node. Every computer has at least 1
				     node.

It may make sense to TBROK if the minimal number of nodes <= 1


> +	}
> +
> +	return nodes;
> +}
> +
>  #endif
> -- 
> 2.49.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC][PATCH 1/2] libs: Add safe_get_nodemap()

[Petr Vorel]

Hi Cyril,

> > +struct tst_nodemap *safe_get_nodemap(int type, size_t min_mem_kb, size_t required)
> > +{
> > +	struct tst_nodemap *nodes;
> > +
> > +	nodes = tst_get_nodemap(type, min_mem_kb);
> > +
> > +	if (nodes->cnt < required) {
> > +		tst_brk(TCONF, "Test requires at least %zi NUMA%s node%s",
> > +				required, tst_numa_type_name(type), required > 1 ? "s" : "");
>                                                                      ^
> 				     It does not make any sense to
> 				     request a NUMA with at least 1
> 				     node. Every computer has at least 1
> 				     node.

Thanks!

> It may make sense to TBROK if the minimal number of nodes <= 1

+1.

I guess we will find use for this even in the case of more complicated version
with struct tst_numa (it'd be used in tst_lib.c and in tests which specify nodes
via the command line parameter.

Thanks for your review!
Petr

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

[LTP] [RFC][PATCH 2/2] Use safe_get_nodemap()

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/mem/ksm/ksm06.c                          | 4 +---
 testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c | 4 +---
 testcases/kernel/syscalls/get_mempolicy/get_mempolicy02.c | 4 +---
 testcases/kernel/syscalls/mbind/mbind02.c                 | 4 +---
 testcases/kernel/syscalls/mbind/mbind03.c                 | 4 +---
 testcases/kernel/syscalls/mbind/mbind04.c                 | 4 +---
 testcases/kernel/syscalls/set_mempolicy/set_mempolicy01.c | 4 +---
 testcases/kernel/syscalls/set_mempolicy/set_mempolicy02.c | 4 +---
 testcases/kernel/syscalls/set_mempolicy/set_mempolicy03.c | 4 +---
 testcases/kernel/syscalls/set_mempolicy/set_mempolicy04.c | 4 +---
 10 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/testcases/kernel/mem/ksm/ksm06.c b/testcases/kernel/mem/ksm/ksm06.c
index a8e73275dd..936980dee6 100644
--- a/testcases/kernel/mem/ksm/ksm06.c
+++ b/testcases/kernel/mem/ksm/ksm06.c
@@ -124,9 +124,7 @@ static void setup(void)
 
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, nr_pages * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, nr_pages * page_size / 1024, 2);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c b/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c
index 3c854d1096..031c35f40e 100644
--- a/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c
+++ b/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c
@@ -142,9 +142,7 @@ static void setup(void)
 {
 	unsigned int i;
 
-	node = tst_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024);
-	if (node->cnt < 1)
-		tst_brk(TCONF, "test requires at least one NUMA memory node");
+	node = safe_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024, 1);
 
 	nodemask = numa_allocate_nodemask();
 	empty_nodemask = numa_allocate_nodemask();
diff --git a/testcases/kernel/syscalls/get_mempolicy/get_mempolicy02.c b/testcases/kernel/syscalls/get_mempolicy/get_mempolicy02.c
index 79ff5d94f8..9344cdbb12 100644
--- a/testcases/kernel/syscalls/get_mempolicy/get_mempolicy02.c
+++ b/testcases/kernel/syscalls/get_mempolicy/get_mempolicy02.c
@@ -55,9 +55,7 @@ static struct test_case tcase[] = {
 
 static void setup(void)
 {
-	node = tst_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024);
-	if (node->cnt < 1)
-		tst_brk(TCONF, "test requires at least one NUMA memory node");
+	node = safe_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024, 1);
 
 	nodemask = numa_allocate_nodemask();
 }
diff --git a/testcases/kernel/syscalls/mbind/mbind02.c b/testcases/kernel/syscalls/mbind/mbind02.c
index cd6a032269..c690ac858e 100644
--- a/testcases/kernel/syscalls/mbind/mbind02.c
+++ b/testcases/kernel/syscalls/mbind/mbind02.c
@@ -32,9 +32,7 @@ static void setup(void)
 {
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, 2 * page_size / 1024, 2);
 }
 
 static void cleanup(void)
diff --git a/testcases/kernel/syscalls/mbind/mbind03.c b/testcases/kernel/syscalls/mbind/mbind03.c
index 3d477c9cbb..58967cc06a 100644
--- a/testcases/kernel/syscalls/mbind/mbind03.c
+++ b/testcases/kernel/syscalls/mbind/mbind03.c
@@ -29,9 +29,7 @@ static void setup(void)
 {
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, 2 * page_size / 1024, 2);
 }
 
 static void cleanup(void)
diff --git a/testcases/kernel/syscalls/mbind/mbind04.c b/testcases/kernel/syscalls/mbind/mbind04.c
index 50028835f9..795905cc14 100644
--- a/testcases/kernel/syscalls/mbind/mbind04.c
+++ b/testcases/kernel/syscalls/mbind/mbind04.c
@@ -31,9 +31,7 @@ static void setup(void)
 {
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024, 2);
 }
 
 static void cleanup(void)
diff --git a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy01.c b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy01.c
index 39e7156d06..92ef165d79 100644
--- a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy01.c
+++ b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy01.c
@@ -32,9 +32,7 @@ static void setup(void)
 {
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024, 2);
 
 	/*
 	 * In most cases, set_mempolicy01 finish quickly, but when the platform
diff --git a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy02.c b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy02.c
index 3db9c20090..57fef862da 100644
--- a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy02.c
+++ b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy02.c
@@ -33,9 +33,7 @@ static void setup(void)
 {
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * ALLOC_ON_NODE * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, 2 * ALLOC_ON_NODE * page_size / 1024, 2);
 }
 
 static void cleanup(void)
diff --git a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy03.c b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy03.c
index 5cfcda6d8c..ac464e5bc1 100644
--- a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy03.c
+++ b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy03.c
@@ -31,9 +31,7 @@ static void setup(void)
 {
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024, 2);
 }
 
 static void cleanup(void)
diff --git a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy04.c b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy04.c
index 2a1d2e1b9a..8e0ed2dc2a 100644
--- a/testcases/kernel/syscalls/set_mempolicy/set_mempolicy04.c
+++ b/testcases/kernel/syscalls/set_mempolicy/set_mempolicy04.c
@@ -44,9 +44,7 @@ static void setup(void)
 
 	page_size = getpagesize();
 
-	nodes = tst_get_nodemap(TST_NUMA_MEM, node_min_pages * page_size / 1024);
-	if (nodes->cnt <= 1)
-		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
+	nodes = safe_get_nodemap(TST_NUMA_MEM, node_min_pages * page_size / 1024, 2);
 }
 
 static void cleanup(void)
-- 
2.49.0


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

Re: [LTP] [RFC][PATCH 2/2] Use safe_get_nodemap()

[Cyril Hrubis]

Hi!
> --- a/testcases/kernel/mem/ksm/ksm06.c
> +++ b/testcases/kernel/mem/ksm/ksm06.c
> @@ -124,9 +124,7 @@ static void setup(void)
>  
>  	page_size = getpagesize();
>  
> -	nodes = tst_get_nodemap(TST_NUMA_MEM, nr_pages * page_size / 1024);
> -	if (nodes->cnt <= 1)
> -		tst_brk(TCONF, "Test requires at least two NUMA memory nodes");
> +	nodes = safe_get_nodemap(TST_NUMA_MEM, nr_pages * page_size / 1024, 2);
>  }
>  
>  static struct tst_test test = {
> diff --git a/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c b/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c
> index 3c854d1096..031c35f40e 100644
> --- a/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c
> +++ b/testcases/kernel/syscalls/get_mempolicy/get_mempolicy01.c
> @@ -142,9 +142,7 @@ static void setup(void)
>  {
>  	unsigned int i;
>  
> -	node = tst_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024);
> -	if (node->cnt < 1)
> -		tst_brk(TCONF, "test requires at least one NUMA memory node");
> +	node = safe_get_nodemap(TST_NUMA_MEM, PAGES_ALLOCATED * getpagesize() / 1024, 1);

Hmm, we are actually requesting at least 1 node.

The question is if we shouldn't change this. It's very unlikely that we
get wrong answer about on which node memory has been allocated if there
is only one node.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC][PATCH 0/2] Add safe_get_nodemap()

[Cyril Hrubis]

Hi!
> not a huge improvement, but because all tst_get_nodemap() are in the
> setup (and only ksm06.c allows input parameter) we could have struct
> tst_test member which would call safe_get_nodemap().
> 
> e.g.:
> 
> .nodemap = (const struct tst_path_val) {
> 		.type = TST_NUMA_MEM
> 		.required = 2,
> },

I do not get this, the struct tst_path_val is something completely
different.

> safe_get_nodemap(tst_test->nodemap->type,
> 	tst_test->nodemap->required * getpagesize() / 1024);
> 
> This would not work for non - page sized nodes, e.g.:
> nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);
> 
> => extra member would need to be added:
> 
> .nodemap = (const struct tst_path_val) {
> 		.type = TST_NUMA_MEM
> 		.required = 2,
> 		.size = PAGES_ALLOCATED, // default == 1
> },

I've avoided the size to be in pages and choosen kilobytes instead
because the page size can differ a lot.

> would call:
> 
> safe_get_nodemap(tst_test->nodemap->type,
> 	tst_test->nodemap->required * tst_test->nodemap->size * getpagesize() / 1024,
         ^
	 This does not make any sense.

The memory parameter is supposed to be minimal free memory on each node,
for each node we check that it has at least min_mem_kb in the
node_has_enough_memory() function.

There is no point in multiplying that by the number of nodes we require.

And this looks like it uses kilobytes not pages.

> 	tst_test->nodemap->required);

I guess that what you meant is that we will add tst_numa structure such as:

struct tst_numa {
	enum tst_numa_types node_type;
	unsigned int min_nodes;
	unsigned int min_mem_kb;
};

And then use that in tst_test structure. That would certainly make
sense, but I guess that we would have to move the numa library to the
lib/ as well. I'm not sure that we can have a function call from
tst_test.c library to something that is not compiled in by default.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [RFC][PATCH 0/2] Add safe_get_nodemap()

[Petr Vorel]

> Hi!
> > not a huge improvement, but because all tst_get_nodemap() are in the
> > setup (and only ksm06.c allows input parameter) we could have struct
> > tst_test member which would call safe_get_nodemap().

> > e.g.:

> > .nodemap = (const struct tst_path_val) {
> > 		.type = TST_NUMA_MEM
> > 		.required = 2,
> > },

> I do not get this, the struct tst_path_val is something completely
> different.

I'm sorry, copy paste error. This was meant to be a new struct,
e.g. struct tst_nodemap_val. But I see you below suggested struct tst_numa.

> > safe_get_nodemap(tst_test->nodemap->type,
> > 	tst_test->nodemap->required * getpagesize() / 1024);

> > This would not work for non - page sized nodes, e.g.:
> > nodes = tst_get_nodemap(TST_NUMA_MEM, 2 * PAGES_ALLOCATED * page_size / 1024);

> > => extra member would need to be added:

> > .nodemap = (const struct tst_path_val) {
> > 		.type = TST_NUMA_MEM
> > 		.required = 2,
> > 		.size = PAGES_ALLOCATED, // default == 1
> > },

> I've avoided the size to be in pages and choosen kilobytes instead
> because the page size can differ a lot.

Make sense.

> > would call:

> > safe_get_nodemap(tst_test->nodemap->type,
> > 	tst_test->nodemap->required * tst_test->nodemap->size * getpagesize() / 1024,
>          ^
> 	 This does not make any sense.

> The memory parameter is supposed to be minimal free memory on each node,
> for each node we check that it has at least min_mem_kb in the
> node_has_enough_memory() function.

> There is no point in multiplying that by the number of nodes we require.

> And this looks like it uses kilobytes not pages.

> > 	tst_test->nodemap->required);

> I guess that what you meant is that we will add tst_numa structure such as:

> struct tst_numa {
> 	enum tst_numa_types node_type;
> 	unsigned int min_nodes;
> 	unsigned int min_mem_kb;
> };

+1, thanks for suggesting a proper struct :).

BTW why we can use enum tst_numa_types in struct and not as function parameter?

-struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb);
+struct tst_nodemap *tst_get_nodemap(enum tst_numa_types type, size_t min_mem_kb);

Old toolchains? Maybe not needed nowadays? I guess with using enum we could get
rid of the check for enum validity:
if (type & ~(TST_NUMA_MEM))
	tst_brk(TBROK, "Invalid type %i\n", type);

> And then use that in tst_test structure. That would certainly make
> sense, but I guess that we would have to move the numa library to the
> lib/ as well. I'm not sure that we can have a function call from
> tst_test.c library to something that is not compiled in by default.

Correct, file is in libs/numa/tst_numa.c. But how about use predecessor check?

if (tst_test->numa) {
#ifdef HAVE_NUMA_V2
	...
#else
	tst_brk(TCONF, NUMA_ERROR_MSG);
#endif
}

With help we would even get rid of else part of the compile check in the
tests:

#else
TST_TEST_TCONF(NUMA_ERROR_MSG);

But we would have modify lib/Makefile to conditionally add "-lnuma -lltpnuma".
"-lnuma" would be based on NUMA_LIBS (stored in include/mk/config.mk, detected
in m4/ltp-numa.m4), e.g. something like this could work:

ifneq ($(NUMA_LIBS),)
LDLIBS += $(NUMA_LIBS) -lltpnuma
endif

Kind regards,
Petr

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

Re: [LTP] [RFC][PATCH 0/2] Add safe_get_nodemap()

[Cyril Hrubis]

Hi!
> +1, thanks for suggesting a proper struct :).
> 
> BTW why we can use enum tst_numa_types in struct and not as function parameter?

Lazines I suppose.

> -struct tst_nodemap *tst_get_nodemap(int type, size_t min_mem_kb);
> +struct tst_nodemap *tst_get_nodemap(enum tst_numa_types type, size_t min_mem_kb);
> 
> Old toolchains? Maybe not needed nowadays? I guess with using enum we could get
> rid of the check for enum validity:
> if (type & ~(TST_NUMA_MEM))
> 	tst_brk(TBROK, "Invalid type %i\n", type);

No we couldn't because you can stil pass whatever you want even when the
type is enum.

> > And then use that in tst_test structure. That would certainly make
> > sense, but I guess that we would have to move the numa library to the
> > lib/ as well. I'm not sure that we can have a function call from
> > tst_test.c library to something that is not compiled in by default.
> 
> Correct, file is in libs/numa/tst_numa.c. But how about use predecessor check?
> 
> if (tst_test->numa) {
> #ifdef HAVE_NUMA_V2
> 	...
> #else
> 	tst_brk(TCONF, NUMA_ERROR_MSG);
> #endif
> }
> 
> With help we would even get rid of else part of the compile check in the
> tests:
> 
> #else
> TST_TEST_TCONF(NUMA_ERROR_MSG);
> 
> But we would have modify lib/Makefile to conditionally add "-lnuma -lltpnuma".
> "-lnuma" would be based on NUMA_LIBS (stored in include/mk/config.mk, detected
> in m4/ltp-numa.m4), e.g. something like this could work:
> 
> ifneq ($(NUMA_LIBS),)
> LDLIBS += $(NUMA_LIBS) -lltpnuma
> endif

I do no think that this would be enough, because all tests are linking
againts -lltp and if -lltp potentionally calls into the libnuma suddenly
all tests has to be linked againts libnuma. Or libnuma must be moved
into the ltp library.

-- 
Cyril Hrubis
chrubis@suse.cz

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