[PATCH] libnuma: disable caching of node cpusmasks

[PATCH] libnuma: disable caching of node cpusmasks

[Petr Holasek]

Caching of node cpumasks confuses long-time running tasks like libvirtd.
If there was an offlined/onlined cpu between two numa_node_to_cpus() calls,
libnuma still returns the same cpumask as before.

Signed-off-by: Petr Holasek <pholasek@redhat.com>
---
 libnuma.c | 36 ++----------------------------------
 1 file changed, 2 insertions(+), 34 deletions(-)

diff --git a/libnuma.c b/libnuma.c
index 3717d5b..7bd5f76 100644
--- a/libnuma.c
+++ b/libnuma.c
@@ -58,7 +58,6 @@ struct bitmask *numa_possible_cpus_ptr = NULL;
 struct bitmask *numa_nodes_ptr = NULL;
 static struct bitmask *numa_memnode_ptr = NULL;
 static unsigned long *node_cpu_mask_v1[NUMA_NUM_NODES];
-static struct bitmask **node_cpu_mask_v2;
 
 WEAK void numa_error(char *where);
 
@@ -1233,13 +1232,6 @@ numa_parse_bitmap_v2(char *line, struct bitmask *mask)
 }
 __asm__(".symver numa_parse_bitmap_v2,numa_parse_bitmap@@libnuma_1.2");
 
-void
-static init_node_cpu_mask_v2(void)
-{
-	int nnodes = numa_max_possible_node_v2_int() + 1;
-	node_cpu_mask_v2 = calloc (nnodes, sizeof(struct bitmask *));
-}
-
 /* This would be better with some locking, but I don't want to make libnuma
    dependent on pthreads right now. The races are relatively harmless. */
 int
@@ -1329,25 +1321,12 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
 	size_t len = 0; 
 	struct bitmask *mask;
 
-	if (!node_cpu_mask_v2)
-		init_node_cpu_mask_v2();
-
 	if (node > nnodes) {
 		errno = ERANGE;
 		return -1;
 	}
 	numa_bitmask_clearall(buffer);
 
-	if (node_cpu_mask_v2[node]) {
-		/* have already constructed a mask for this node */
-		if (buffer->size < node_cpu_mask_v2[node]->size) {
-			numa_error("map size mismatch; abort\n");
-			return -1;
-		}
-		copy_bitmask_to_bitmask(node_cpu_mask_v2[node], buffer);
-		return 0;
-	}
-
 	/* need a new mask for this node */
 	mask = numa_allocate_cpumask();
 
@@ -1375,19 +1354,8 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
 	free(line);
 	copy_bitmask_to_bitmask(mask, buffer);
 
-	/* slightly racy, see above */ 
-	/* save the mask we created */
-	if (node_cpu_mask_v2[node]) {
-		/* how could this be? */
-		if (mask != buffer)
-			numa_bitmask_free(mask);
-	} else {
-		/* we don't want to cache faulty result */
-		if (!err)
-			node_cpu_mask_v2[node] = mask;
-		else
-			numa_bitmask_free(mask);
-	}
+	numa_bitmask_free(mask);
+
 	return err; 
 }
 __asm__(".symver numa_node_to_cpus_v2,numa_node_to_cpus@@libnuma_1.2");
-- 
2.4.3

Re: [PATCH] libnuma: disable caching of node cpusmasks

[Andi Kleen]

> @@ -1329,25 +1321,12 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
>  	size_t len = 0; 
>  	struct bitmask *mask;
>  
> -	if (!node_cpu_mask_v2)
> -		init_node_cpu_mask_v2();
> -
>  	if (node > nnodes) {
>  		errno = ERANGE;
>  		return -1;
>  	}
>  	numa_bitmask_clearall(buffer);
>  
> -	if (node_cpu_mask_v2[node]) {

I was playing around with info (http://fbinfer.com/) earlier, and ran it over
numactl, and it complained about exactly this line: it can reference NULL
when the memory allocation above fails. So it's good to remove it.

Would be good to see how much performance difference it makes though.
Do you have any data? If it's significant may need to do a time out
or similar.

-Andi

Re: [PATCH] libnuma: disable caching of node cpusmasks

[Petr Holasek]

On Fri, 12 Jun 2015, Andi Kleen <andi@firstfloor.org> wrote:
> > @@ -1329,25 +1321,12 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
> >  	size_t len = 0; 
> >  	struct bitmask *mask;
> >  
> > -	if (!node_cpu_mask_v2)
> > -		init_node_cpu_mask_v2();
> > -
> >  	if (node > nnodes) {
> >  		errno = ERANGE;
> >  		return -1;
> >  	}
> >  	numa_bitmask_clearall(buffer);
> >  
> > -	if (node_cpu_mask_v2[node]) {
> 
> I was playing around with info (http://fbinfer.com/) earlier, and ran it over
> numactl, and it complained about exactly this line: it can reference NULL
> when the memory allocation above fails. So it's good to remove it.
> 
> Would be good to see how much performance difference it makes though.
> Do you have any data? If it's significant may need to do a time out
> or similar.
> 

I did simple comparison for 1 million of numa_node_to_cpus() calls
and disabled caching extended time three times (from 10 seconds to 30).

-- 
Petr Holasek
pholasek@redhat.com

Re: [PATCH] libnuma: disable caching of node cpusmasks

[Andi Kleen]

> I did simple comparison for 1 million of numa_node_to_cpus() calls
> and disabled caching extended time three times (from 10 seconds to 30).

that's a big slow down. Someone could have it in their fast paths.

I think need a time out or so.

-Andi

[PATCH] libnuma: don't cache node cpumasks older than 1 second

[Petr Holasek]

To preserve useful caching feature and minimize the risk of getting outdated
node cpumask after cpu hotplugging, numactl refreshes cache for every call
delayed more as one second from previous one. This is compromise between
outdated static cache and node cpumask parsing during every call of
numa_node_to_cpus which would be very inefficient.

Signed-off-by: Petr Holasek <pholasek@redhat.com>
---
 libnuma.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/libnuma.c b/libnuma.c
index 3717d5b..30d4ac8 100644
--- a/libnuma.c
+++ b/libnuma.c
@@ -30,6 +30,7 @@
 
 #include <sys/mman.h>
 #include <limits.h>
+#include <time.h>
 
 #include "config.h"
 #include "numa.h"
@@ -76,6 +77,7 @@ static int numprocnode = -1;
 static int numproccpu = -1;
 static int nodemask_sz = 0;
 static int cpumask_sz = 0;
+static time_t last_tstamp = 0;
 
 int numa_exit_on_error = 0;
 int numa_exit_on_warn = 0;
@@ -1328,6 +1330,7 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
 	FILE *f; 
 	size_t len = 0; 
 	struct bitmask *mask;
+	time_t curr_tstamp;
 
 	if (!node_cpu_mask_v2)
 		init_node_cpu_mask_v2();
@@ -1338,7 +1341,9 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
 	}
 	numa_bitmask_clearall(buffer);
 
-	if (node_cpu_mask_v2[node]) {
+	curr_tstamp = time(NULL);
+
+	if (node_cpu_mask_v2[node] && curr_tstamp == last_tstamp) {
 		/* have already constructed a mask for this node */
 		if (buffer->size < node_cpu_mask_v2[node]->size) {
 			numa_error("map size mismatch; abort\n");
@@ -1388,6 +1393,9 @@ numa_node_to_cpus_v2(int node, struct bitmask *buffer)
 		else
 			numa_bitmask_free(mask);
 	}
+
+	last_tstamp = curr_tstamp;
+
 	return err; 
 }
 __asm__(".symver numa_node_to_cpus_v2,numa_node_to_cpus@@libnuma_1.2");
-- 
2.4.3