[PATCH 0/8] numactl/libnuma - Fixes and Cleanup

[PATCH 0/8] numactl/libnuma - Fixes and Cleanup

[Lee Schermerhorn]

This is a series of patches to the numactl/libnuma package, against
numactl-2.0.3-rc2.  A couple of these patches include hunks that may
be or are already in the Cliff's tree--see the patch descriptions. 
I've included them here as these are what I've tested with.

+ libnuma:  
++  fix apparent bitmask memory leaks;
++  always return freeable bitmask pointers, instead of pointers to
    libnuma exported masks such as numa_no_nodes_ptr
++  fix parsing of /proc/self/status cpu and node masks in
    set_thread_constraints.

+ numademo:
++  add extra loop in memtest() to eliminate page allocation overhead
    from bandwidth stats.

+ Makefile:
++ define LIBNUMA_SO and use throughout the Makefile to support
   multiple versions on a system.

+ test/checkaffinity:
++  fix test expression to work if numactl should fail.


Pick and choose as you see fit, Cliff.

Lee

[PATCH 1/8] numactl/libnuma - Possibly already fixed leaks and cleanup

[Lee Schermerhorn]

[PATCH 01/08] Possibly already fixed leaks and cleanup

Against:  numactl-2.0.3-rc2

Kornilios says these [Hunks #1 and 3] are already fixed,
so I pulled them into a separate patch.

Hunk #2 that modifies numa_alloc_local() just reorganizes the code
to match the structure of numa_alloc_onnode() with, I think, no change
in behavior.  This is probably not "already fixed".

I suppose I could/should change those '(char *)-1's to MAP_FAILED, too...


 libnuma.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

Index: numactl-2.0.3-rc2/libnuma.c
===================================================================
--- numactl-2.0.3-rc2.orig/libnuma.c	2009-04-27 08:47:14.000000000 -0400
+++ numactl-2.0.3-rc2/libnuma.c	2009-04-27 09:01:33.000000000 -0400
@@ -919,8 +919,10 @@ void *numa_alloc_onnode(size_t size, int
 	mem = mmap(0, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS,
 		   0, 0);  
 	if (mem == (char *)-1)
-		return NULL;		
-	dombind(mem, size, bind_policy, bmp);
+		mem = NULL;
+	else
+		dombind(mem, size, bind_policy, bmp);
+	numa_bitmask_free(bmp);
 	return mem; 	
 } 
 
@@ -930,8 +932,9 @@ void *numa_alloc_local(size_t size) 
 	mem = mmap(0, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS,
 		   0, 0); 
 	if (mem == (char *)-1)
-		return NULL;
-	dombind(mem, size, MPOL_PREFERRED, NULL);
+		mem =  NULL;
+	else
+		dombind(mem, size, MPOL_PREFERRED, NULL);
 	return mem; 	
 } 
 
@@ -1496,7 +1499,7 @@ int numa_run_on_node(int node)
 
 int numa_preferred(void)
 { 
-	int policy;
+	int policy, node = 0;
 	struct bitmask *bmp;
 
 	bmp = numa_allocate_nodemask();
@@ -1505,12 +1508,15 @@ int numa_preferred(void)
 		int i;
 		int max = numa_num_possible_nodes();
 		for (i = 0; i < max ; i++) 
-			if (numa_bitmask_isbitset(bmp, i))
-				return i; 
+			if (numa_bitmask_isbitset(bmp, i)) {
+				node = i;
+				break;
+			}
 	}
 	/* could read the current CPU from /proc/self/status. Probably 
 	   not worth it. */
-	return 0; /* or random one? */
+	numa_bitmask_free(bmp);
+	return node;
 }
 
 void numa_set_preferred(int node)

[PATCH 2/8] numactl/libnuma - Simple bitmask leak fixes

[Lee Schermerhorn]

[PATCH 02/08] Fix Simple Bitmask Leaks

Against:  numactl-2.0.3-rc2

Add or move numa_bitmask_free() or exit via path that already
frees bitmask to plug leaks.

Kornilios pulled a couple of these hunks out of a single big patch
that I had originally created.  I figured the node_parse_*string()
hunks were simple enough to fit here as well.

Note:  this patch leaves a bitmask leak in numa_parse_nodestring()
that will be fixed in a later patch.

 libnuma.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

Index: numactl-2.0.3-rc2/libnuma.c
===================================================================
--- numactl-2.0.3-rc2.orig/libnuma.c	2009-04-27 09:33:09.000000000 -0400
+++ numactl-2.0.3-rc2/libnuma.c	2009-04-27 09:34:40.000000000 -0400
@@ -755,6 +755,7 @@ void numa_tonode_memory(void *mem, size_
 	nodes = numa_allocate_nodemask();
 	numa_bitmask_setbit(nodes, node);
 	dombind(mem, size, bind_policy, nodes);
+	numa_bitmask_free(nodes);
 }
 
 void
@@ -882,6 +883,7 @@ numa_get_interleave_mask_v1(void)
 		copy_bitmask_to_nodemask(bmp, &mask);
 	else
 	 	copy_bitmask_to_nodemask(numa_no_nodes_ptr, &mask);
+	numa_bitmask_free(bmp);
 	return mask;
 }
 __asm__(".symver numa_get_interleave_mask_v1,numa_get_interleave_mask@libnuma_1.1");
@@ -1388,13 +1390,13 @@ numa_run_on_node_mask_v2(struct bitmask 
 	}
 	err = numa_sched_setaffinity_v2_int(0, cpus);
 
+	numa_bitmask_free(cpus);
+	numa_bitmask_free(nodecpus);
+
 	/* used to have to consider that this could fail - it shouldn't now */
 	if (err < 0) {
 		numa_error("numa_sched_setaffinity_v2_int() failed; abort\n");
-		return -1;
 	}
-	numa_bitmask_free(cpus);
-	numa_bitmask_free(nodecpus);
 
 	return err;
 } 
@@ -1662,7 +1664,7 @@ numa_parse_nodestring(char *s)
 		}
 	} while (*s++ == ',');
 	if (s[-1] != '\0')
-		return 0;
+		goto err;
 	if (invert) {
 		int i;
 		for (i = 0; i < conf_nodes; i++) {
@@ -1758,7 +1760,7 @@ numa_parse_cpustring(char *s)
 		}
 	} while (*s++ == ',');
 	if (s[-1] != '\0')
-		return 0;
+		goto err;
 	if (invert) {
 		int i;
 		for (i = 0; i < conf_cpus; i++) {

[PATCH 3/8] numactl/libnuma - more bitmask leak fixes

[Lee Schermerhorn]

[PATCH 03/08] - reorganize code to plug apparent bitmask leaks

Against:  numactl-2.0.3-rc2

The remainder of the fixex for apparent leaks.  These required a bit
more reorg to address.

 libnuma.c |   75 ++++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 47 insertions(+), 28 deletions(-)

Index: numactl-2.0.3-rc2/libnuma.c
===================================================================
--- numactl-2.0.3-rc2.orig/libnuma.c	2009-04-27 09:44:52.000000000 -0400
+++ numactl-2.0.3-rc2/libnuma.c	2009-04-27 16:49:39.000000000 -0400
@@ -1031,17 +1031,19 @@ numa_get_membind_v1(void)
 {
 	int oldpolicy;
 	struct bitmask *bmp;
-	nodemask_t *nmp;
+	nodemask_t nmp;
 
 	bmp = allocate_nodemask_v1();
 	getpol(&oldpolicy, bmp);
 	if (oldpolicy == MPOL_BIND) {
-		nmp = (nodemask_t *)bmp->maskp;
-		return *nmp;
+		copy_bitmask_to_nodemask(bmp, &nmp);
+	} else {
+		/* copy the body of the map to numa_all_nodes */
+		copy_bitmask_to_nodemask(bmp, &numa_all_nodes);
+		nmp = numa_all_nodes;
 	}
-	/* copy the body of the map to numa_all_nodes */
-	copy_bitmask_to_nodemask(bmp, &numa_all_nodes);
-	return numa_all_nodes;
+	numa_bitmask_free(bmp);
+	return nmp;
 }
 __asm__(".symver numa_get_membind_v1,numa_get_membind@libnuma_1.1");
 
@@ -1411,14 +1413,16 @@ numa_get_run_node_mask_v1(void)
 	int i, k;
 	int max = numa_max_node_int();
 	struct bitmask *bmp, *cpus, *nodecpus;
-	nodemask_t *nmp;
+	nodemask_t nmp;
 
-	bmp = allocate_nodemask_v1(); /* the size of a nodemask_t */
 	cpus = numa_allocate_cpumask();
-	nodecpus = numa_allocate_cpumask();
-	if (numa_sched_getaffinity_v2_int(0, cpus) < 0)
-		return numa_no_nodes;
+	if (numa_sched_getaffinity_v2_int(0, cpus) < 0){
+		nmp = numa_no_nodes;
+		goto free_cpus;
+	}
 
+	nodecpus = numa_allocate_cpumask();
+	bmp = allocate_nodemask_v1(); /* the size of a nodemask_t */
 	for (i = 0; i <= max; i++) {
 		if (numa_node_to_cpus_v2_int(i, nodecpus) < 0) {
 			/* It's possible for the node to not exist */
@@ -1429,8 +1433,12 @@ numa_get_run_node_mask_v1(void)
 				numa_bitmask_setbit(bmp, i);
 		}
 	}
-	nmp = (nodemask_t *)bmp->maskp;
-	return *nmp;
+	copy_bitmask_to_nodemask(bmp, &nmp);
+	numa_bitmask_free(bmp);
+	numa_bitmask_free(nodecpus);
+free_cpus:
+	numa_bitmask_free(cpus);
+	return nmp;
 }
 __asm__(".symver numa_get_run_node_mask_v1,numa_get_run_node_mask@libnuma_1.1");
 
@@ -1442,13 +1450,15 @@ numa_get_run_node_mask_v2(void)
 	int max = numa_max_node_int();
 	struct bitmask *bmp, *cpus, *nodecpus;
 
+
 	bmp = numa_allocate_cpumask();
 	cpus = numa_allocate_cpumask();
-	nodecpus = numa_allocate_cpumask();
-
-	if (numa_sched_getaffinity_v2_int(0, cpus) < 0)
-		return numa_no_nodes_ptr;
+	if (numa_sched_getaffinity_v2_int(0, cpus) < 0){
+		copy_bitmask_to_bitmask(numa_no_nodes_ptr, bmp);
+		goto free_cpus;
+	}
 
+	nodecpus = numa_allocate_cpumask();
 	for (i = 0; i <= max; i++) {
 		if (numa_node_to_cpus_v2_int(i, nodecpus) < 0) {
 			/* It's possible for the node to not exist */
@@ -1459,6 +1469,9 @@ numa_get_run_node_mask_v2(void)
 				numa_bitmask_setbit(bmp, i);
 		}
 	}		
+	numa_bitmask_free(nodecpus);
+free_cpus:
+	numa_bitmask_free(cpus);
 	return bmp;
 } 
 __asm__(".symver numa_get_run_node_mask_v2,numa_get_run_node_mask@@libnuma_1.2");
@@ -1481,22 +1494,28 @@ int numa_move_pages(int pid, unsigned lo
 int numa_run_on_node(int node)
 { 
 	int numa_num_nodes = numa_num_possible_nodes();
+	int ret = -1;
 	struct bitmask *cpus;
 
+	if (node >= numa_num_nodes){
+		errno = EINVAL;
+		goto out;
+	}
+
 	cpus = numa_allocate_cpumask();
-	if (node == -1) {
+
+	if (node == -1)
 		numa_bitmask_setall(cpus);
-	} else if (node < numa_num_nodes) {
-		if (numa_node_to_cpus_v2_int(node, cpus) < 0) {
-			numa_warn(W_noderunmask,
-				"Cannot read node cpumask from sysfs");
-			return -1; 
-		} 		
-	} else { 
-		errno = EINVAL;
-		return -1; 
+	else if (numa_node_to_cpus_v2_int(node, cpus) < 0){
+		numa_warn(W_noderunmask, "Cannot read node cpumask from sysfs");
+		goto free;
 	}
-	return numa_sched_setaffinity_v2_int(0, cpus);
+
+	ret = numa_sched_setaffinity_v2_int(0, cpus);
+free:
+	numa_bitmask_free(cpus);
+out:
+	return ret;
 } 
 
 int numa_preferred(void)

[PATCH 4/8] numactl/libnuma - return freeable bitmasks

[Lee Schermerhorn]

[PATCH 04/08] - Return freeable masks

Against:  numactl-2.0.3-rc2

The V2+ libnuma API provides functions to free masks [bit, cpu, node]
returned by some functions.  [It should probably be more specific
about it being the caller's responsibility to free such masks.]
However, in some cases, the library returns pointers to libnuma
internal masks, such as numa_no_nodes.

Instead, return pointer to freshly allocated mask that the caller can
free without affecting the library's internal operation.

Kornilious replaced my bogus structure assignments with calls to
copy_bitmask_to_bitmask()   --lts


 libnuma.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

Index: numactl-2.0.3-rc2/libnuma.c
===================================================================
--- numactl-2.0.3-rc2.orig/libnuma.c	2009-04-27 16:49:39.000000000 -0400
+++ numactl-2.0.3-rc2/libnuma.c	2009-04-27 16:50:00.000000000 -0400
@@ -896,9 +896,9 @@ numa_get_interleave_mask_v2(void)
 
 	bmp = numa_allocate_nodemask();
 	getpol(&oldpolicy, bmp);
-	if (oldpolicy == MPOL_INTERLEAVE)
-		return bmp;
-	return numa_no_nodes_ptr;
+	if (oldpolicy != MPOL_INTERLEAVE)
+		copy_bitmask_to_bitmask(numa_no_nodes_ptr, bmp);
+	return bmp;
 } 
 __asm__(".symver numa_get_interleave_mask_v2,numa_get_interleave_mask@@libnuma_1.2");
 
@@ -1055,9 +1055,9 @@ numa_get_membind_v2(void)
 
 	bmp = numa_allocate_nodemask();
 	getpol(&oldpolicy, bmp);
-	if (oldpolicy == MPOL_BIND)
-		return bmp;
-	return numa_all_nodes_ptr;
+	if (oldpolicy != MPOL_BIND)
+		copy_bitmask_to_bitmask(numa_all_nodes_ptr, bmp);
+	return bmp;
 } 
 __asm__(".symver numa_get_membind_v2,numa_get_membind@@libnuma_1.2");
 
@@ -1629,8 +1629,10 @@ numa_parse_nodestring(char *s)
 
 	mask = numa_allocate_nodemask();
 
-	if (s[0] == 0)
-		return numa_no_nodes_ptr;
+	if (s[0] == 0){
+		copy_bitmask_to_bitmask(numa_no_nodes_ptr, mask);
+		return mask; /* return freeable mask */
+	}
 	if (*s == '!') {
 		invert = 1;
 		s++;

[PATCH 5/8] numactl/libnuma - fix parsing of cpu, node mask

[Lee Schermerhorn]

[PATCH 05/08] - Fix Parsing of maxproc{cpu|node} in set_thread_constraints()

Against:  numactl-2.0.3-rc2

NOTE:  a patch to fix this has already been posted to linux-numa by
Brice Goglin.  This is the version that I had in my tree.

Hard-coded '+15' was dropping the first character of the mask.
This caused, e.g., numactl to error out on last 4 processors of a
platform--on a 32-cpu system, in my case.

That's a tab separating the tag from the mask, so look for tab
explicitly.

 libnuma.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: numactl-2.0.3-rc2/libnuma.c
===================================================================
--- numactl-2.0.3-rc2.orig/libnuma.c	2009-02-13 11:17:57.000000000 -0500
+++ numactl-2.0.3-rc2/libnuma.c	2009-02-13 11:32:35.000000000 -0500
@@ -373,6 +373,9 @@ read_mask(char *s, struct bitmask *bmp)
 	unsigned int *start = tmp;
 	unsigned int i, n = 0, m = 0;
 
+	if (!s)
+		return 0;	/* shouldn't happen */
+
 	i = strtoul(s, &end, 16);
 
 	/* Skip leading zeros */
@@ -445,12 +448,15 @@ set_thread_constraints(void)
 	}
 
 	while (getline(&buffer, &buflen, f) > 0) {
+		/* mask starts after [last] tab */
+		char  *mask = strrchr(buffer,'\t') + 1;
+
 		if (strncmp(buffer,"Cpus_allowed:",13) == 0)
-			maxproccpu = read_mask(buffer + 15, numa_all_cpus_ptr);
+			maxproccpu = read_mask(mask, numa_all_cpus_ptr);
 
 		if (strncmp(buffer,"Mems_allowed:",13) == 0) {
 			maxprocnode =
-				read_mask(buffer + 15, numa_all_nodes_ptr);
+				read_mask(mask, numa_all_nodes_ptr);
 		}
 	}
 	fclose(f);

[PATCH 6/8] numactl/numademo - eliminate page allocation overhead from memtest measurements

[Lee Schermerhorn]

PATCH 6/08 numactl - Eliminate page allocation from memtest() measurements

Against:  numactl-2.0.3-rc2

The first loop [of LOOPS] in memtest() actually incurs the
page allocation overhead.  This results in an artificially
low minimum bandwidth, making the reported bandwidth appear to
be more variable than it really is.

Make an extra, un-measured pass through test buffer in
memtest() to allocate memory.  Don't report throughput
for page allocation phase.  If page allocation overhead
is of interest, a separate test could be added.  Or, we
could make this behavior [measure allocation loop],
optional.  

 numademo.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletion(-)

Index: numactl-2.0.3-rc2/numademo.c
===================================================================
--- numactl-2.0.3-rc2.orig/numademo.c	2009-01-30 10:10:15.000000000 -0500
+++ numactl-2.0.3-rc2/numademo.c	2009-02-13 10:05:42.000000000 -0500
@@ -165,7 +165,11 @@ void memtest(char *name, unsigned char *
 	max = 0; 
 	min = ~0UL; 
 	sum = 0;
-	for (i = 0; i < LOOPS; i++) { 
+
+	/*
+	 * Note:  0th pass allocates the pages, don't measure
+	 */
+	for (i = 0; i < LOOPS+1; i++) {
 		clearcache(mem, msize);
 		switch (thistest) { 
 		case PTRCHASE:
@@ -240,6 +244,9 @@ void memtest(char *name, unsigned char *
 			break;
 		} 
 
+		if (!i)
+			continue;  /* don't count allocation pass */
+
 		timersub(&end, &start, &res);
 		r = timerfold(&res); 
 		if (r > max) max = r;

[PATCH 7/8] numactl/Makefile - Generalize Makefile .so Version

[Lee Schermerhorn]

[PATCH 07/08] Generalize Makefile .so Version

Against:  numactl-2.0.3-rc2

When building "upstream" libnuma on a system with, e.g., a distro-supplied
numactl package installed, I find it useful to build a different version of
libnuma.so, so that both versions of the library can co-exist.  

This patch replaces all explicit usage of the libnuma.so.N file name with
the make variable LIBNUMA_SO.

Note that I'm building with 'libnuma.so.2'.  I think this makes sense for
numactl 2.x.y, but may not be what you want.


 Makefile |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

Index: numactl-2.0.3-rc2/Makefile
===================================================================
--- numactl-2.0.3-rc2.orig/Makefile	2009-01-30 10:10:15.000000000 -0500
+++ numactl-2.0.3-rc2/Makefile	2009-02-13 10:16:47.000000000 -0500
@@ -8,6 +8,8 @@ BENCH_CFLAGS := -O3 -ffast-math -funroll
 CFLAGS += ${OPT_CFLAGS}
 override CFLAGS += -I.
 
+LIBNUMA_SO =  libnuma.so.2
+
 # find out if compiler supports __thread
 THREAD_SUPPORT := $(shell if $(CC) $(CFLAGS) threadtest.c -o threadtest \
 			>/dev/null 2>/dev/null ; then echo "yes" ; else echo "no"; fi)
@@ -23,7 +25,7 @@ ifeq ($(THREAD_SUPPORT),yes)
 endif
 
 CLEANFILES := numactl.o libnuma.o numactl numademo numademo.o distance.o \
-	      memhog libnuma.so libnuma.so.1 numamon numamon.o syscall.o bitops.o \
+	      memhog libnuma.so $(LIBNUMA_SO) numamon numamon.o syscall.o bitops.o \
 	      memhog.o util.o stream_main.o stream_lib.o shm.o stream clearcache.o \
 	      test/pagesize test/tshared test/mynode.o test/tshared.o mt.o empty.o empty.c \
 	      test/mynode test/ftok test/prefered test/randmap \
@@ -77,13 +79,13 @@ stream: stream_lib.o stream_main.o  libn
 
 stream_main.o: stream_main.c
 
-libnuma.so.1: versions.ldscript
+$(LIBNUMA_SO): versions.ldscript
 
-libnuma.so.1: libnuma.o syscall.o distance.o
-	${CC} -shared -Wl,-soname=libnuma.so.1 -Wl,--version-script,versions.ldscript -Wl,-init,numa_init -o libnuma.so.1 $(filter-out versions.ldscript,$^)
+$(LIBNUMA_SO): libnuma.o syscall.o distance.o
+	${CC} -shared -Wl,-soname=$(LIBNUMA_SO) -Wl,--version-script,versions.ldscript -Wl,-init,numa_init -o $(LIBNUMA_SO) $(filter-out versions.ldscript,$^)
 
-libnuma.so: libnuma.so.1
-	ln -sf libnuma.so.1 libnuma.so
+libnuma.so: $(LIBNUMA_SO)
+	ln -sf $(LIBNUMA_SO) libnuma.so
 
 libnuma.o : CFLAGS += -fPIC
 
@@ -123,7 +125,7 @@ test/migrate_pages: test/migrate_pages.c
 
 MANPAGES := numa.3 numactl.8 numastat.8 migratepages.8 migspeed.8
 
-install: numactl migratepages migspeed numademo.c numamon memhog libnuma.so.1 numa.h numaif.h numacompat1.h numastat ${MANPAGES}
+install: numactl migratepages migspeed numademo.c numamon memhog $(LIBNUMA_SO) numa.h numaif.h numacompat1.h numastat ${MANPAGES}
 	mkdir -p ${prefix}/bin
 	install -m 0755 numactl ${prefix}/bin
 	install -m 0755 migratepages ${prefix}/bin
@@ -135,8 +137,8 @@ install: numactl migratepages migspeed n
 	install -m 0644 numa.3 ${prefix}/share/man/man3
 	( cd ${prefix}/share/man/man3 ; for i in $$(./manlinks) ; do ln -sf numa.3 $$i.3 ; done )
 	mkdir -p ${libdir}
-	install -m 0755 libnuma.so.1 ${libdir}
-	cd ${libdir} ; ln -sf libnuma.so.1 libnuma.so
+	install -m 0755 $(LIBNUMA_SO) ${libdir}
+	cd ${libdir} ; ln -sf $(LIBNUMA_SO) libnuma.so
 	install -m 0644 libnuma.a ${libdir}
 	mkdir -p ${prefix}/include
 	install -m 0644 numa.h numaif.h numacompat1.h ${prefix}/include

Re: [PATCH 7/8] numactl/Makefile - Generalize Makefile .so Version

[Andi Kleen]

On Tue, Apr 28, 2009 at 12:37:02PM -0400, Lee Schermerhorn wrote:
> [PATCH 07/08] Generalize Makefile .so Version
> 
> Against:  numactl-2.0.3-rc2
> 
> When building "upstream" libnuma on a system with, e.g., a distro-supplied
> numactl package installed, I find it useful to build a different version of
> libnuma.so, so that both versions of the library can co-exist.  

Sorry I don't think that's a good idea. If that ever escapes from
your system we have a binary incompatibility mess, which was 
always one of the goals of libnuma to avoid.

I just use LD_LIBRARY_PATH for testing newer libnumas. It's not too
bad.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 7/8] numactl/Makefile - Generalize Makefile .so Version

[Lee Schermerhorn]

On Tue, 2009-04-28 at 18:46 +0200, Andi Kleen wrote:
> On Tue, Apr 28, 2009 at 12:37:02PM -0400, Lee Schermerhorn wrote:
> > [PATCH 07/08] Generalize Makefile .so Version
> > 
> > Against:  numactl-2.0.3-rc2
> > 
> > When building "upstream" libnuma on a system with, e.g., a distro-supplied
> > numactl package installed, I find it useful to build a different version of
> > libnuma.so, so that both versions of the library can co-exist.  
> 
> Sorry I don't think that's a good idea. If that ever escapes from
> your system we have a binary incompatibility mess, which was 
> always one of the goals of libnuma to avoid.
> 
> I just use LD_LIBRARY_PATH for testing newer libnumas. It's not too
> bad.

Yeah, I use LD_LIBRARY_PATH when testing in the numactl build directory,
[make test], but I also install the tools on the test servers and like
to be able to use both version.  But, that's just for my convenience, so
I'm fine with not including this patch.  It's easy enough to rebase for
my own use.

I do wonder, tho', why we don't/didn't bump the library version when we
added the v2 API.  One can't tell from looking at the libnuma.so.1
whether it supports the new APIs or not.  [I know.  objdump will tell
me.] Given that the v2 lib still supports the v1 APIs and we install the
libnuma.so sym link, seems it would be nice to have the library version
match the API version.

Later,
Lee

Re: [PATCH 7/8] numactl/Makefile - Generalize Makefile .so Version

[Andi Kleen]

> 
> I do wonder, tho', why we don't/didn't bump the library version when we
> added the v2 API.  One can't tell from looking at the libnuma.so.1
> whether it supports the new APIs or not.  [I know.  objdump will tell

rpm -qf /usr/lib64/libnuma.so.1 ?

> me.] Given that the v2 lib still supports the v1 APIs and we install the
> libnuma.so sym link, seems it would be nice to have the library version
> match the API version.

That would have broken the compatibility to old programs. And it's
not needed.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

[PATCH 8/8] numactl/test - Make checkaffinity more robust

[Lee Schermerhorn]

[PATCH 08/08] Make 'checkaffinity' test script more robust

Against:  numactl-2.0.3-rc2

Add quotes around operands in test expressions, lest empty strings
in the case of numactl failure result in shell errors, obscuring the
test error message.

 test/checkaffinity |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: numactl-2.0.3-rc2/test/checkaffinity
===================================================================
--- numactl-2.0.3-rc2.orig/test/checkaffinity	2009-02-13 11:17:57.000000000 -0500
+++ numactl-2.0.3-rc2/test/checkaffinity	2009-02-13 11:35:27.000000000 -0500
@@ -9,11 +9,11 @@ numcpus=$(grep -c processor /proc/cpuinf
 numnodes=$(ls -1d /sys/devices/system/node/node[0-9]* | wc -l )
 
 for i in $(seq 0 $[$numcpus - 1]) ; do
-    if [ $(numactl --physcpubind=$i ./printcpu) != $i ] ; then
+    if [ "$(numactl --physcpubind=$i ./printcpu)" != "$i" ] ; then
        echo "--physcpubind for $i doesn't work"
        exit 1
     fi
-    if [ $(numactl --physcpubind=$i numactl --show | awk '/^physcpubind/ { print $2 }' ) != $i ] ; then
+    if [ "$(numactl --physcpubind=$i numactl --show | awk '/^physcpubind/ { print $2 }' )" != "$i" ] ; then
 	echo "--show doesn't agree with physcpubind for cpu $i"
 	exit 1
     fi

Re: [PATCH 0/8] numactl/libnuma - Fixes and Cleanup

[Cliff Wickman]

Hi Lee,

On Tue, Apr 28, 2009 at 12:36:21PM -0400, Lee Schermerhorn wrote:
> This is a series of patches to the numactl/libnuma package, against
> numactl-2.0.3-rc2.  A couple of these patches include hunks that may
> be or are already in the Cliff's tree--see the patch descriptions. 
> I've included them here as these are what I've tested with.
> 
> + libnuma:  
> ++  fix apparent bitmask memory leaks;
> ++  always return freeable bitmask pointers, instead of pointers to
>     libnuma exported masks such as numa_no_nodes_ptr
> ++  fix parsing of /proc/self/status cpu and node masks in
>     set_thread_constraints.
> 
> + numademo:
> ++  add extra loop in memtest() to eliminate page allocation overhead
>     from bandwidth stats.
> 
> + Makefile:
> ++ define LIBNUMA_SO and use throughout the Makefile to support
>    multiple versions on a system.
> 
> + test/checkaffinity:
> ++  fix test expression to work if numactl should fail.
> 
> 
> Pick and choose as you see fit, Cliff.

Thanks for all the clean-up.

I have applied your patches and tested on ia64 (8 nodes).
patch1: (but not the hunks that Kornilios had supplied)
patch2: bitmask memory leaks numa_tonode_memory()
        numa_get_interleave_mask_v1() numa_run_on_node_mask_v2()
patch3: leaks in numa_get_membind_v1() numa_get_run_node_mask_v1()
        numa_get_run_node_mask_v1() numa_get_run_node_mask_v2 numa_run_on_node()
patch4: return freeable bitmasks
patch5: parsing of /proc/self (extends Brice's)
patch6: numademo memtest page allocation overhead
patch8: test/checkaffinity script

I did not apply patch 7.  I think you and Andi agreed on that.


The tarball is
ftp://oss.sgi.com/www/projects/libnuma/download/numactl-2.0.3-rc3.tar.gz


-Cliff
-- 
Cliff Wickman
Silicon Graphics, Inc.
cpw@sgi.com
(651) 683-3824

Re: [PATCH 0/8] numactl/libnuma - Fixes and Cleanup

[Lee Schermerhorn]

On Tue, 2009-04-28 at 14:38 -0500, Cliff Wickman wrote:
> Hi Lee,
> 
> On Tue, Apr 28, 2009 at 12:36:21PM -0400, Lee Schermerhorn wrote:
> > This is a series of patches to the numactl/libnuma package, against
> > numactl-2.0.3-rc2.  A couple of these patches include hunks that may
> > be or are already in the Cliff's tree--see the patch descriptions. 
> > I've included them here as these are what I've tested with.
> > 
> > + libnuma:  
> > ++  fix apparent bitmask memory leaks;
> > ++  always return freeable bitmask pointers, instead of pointers to
> >     libnuma exported masks such as numa_no_nodes_ptr
> > ++  fix parsing of /proc/self/status cpu and node masks in
> >     set_thread_constraints.
> > 
> > + numademo:
> > ++  add extra loop in memtest() to eliminate page allocation overhead
> >     from bandwidth stats.
> > 
> > + Makefile:
> > ++ define LIBNUMA_SO and use throughout the Makefile to support
> >    multiple versions on a system.
> > 
> > + test/checkaffinity:
> > ++  fix test expression to work if numactl should fail.
> > 
> > 
> > Pick and choose as you see fit, Cliff.
> 
> Thanks for all the clean-up.
> 
> I have applied your patches and tested on ia64 (8 nodes).

Good.  I had tested on a 4-node x86_64 on a RHEL5 and a recent mmotm
kernel.  The latter is where I found the kernel breakage.

> patch1: (but not the hunks that Kornilios had supplied)
> patch2: bitmask memory leaks numa_tonode_memory()
>         numa_get_interleave_mask_v1() numa_run_on_node_mask_v2()
> patch3: leaks in numa_get_membind_v1() numa_get_run_node_mask_v1()
>         numa_get_run_node_mask_v1() numa_get_run_node_mask_v2 numa_run_on_node()
> patch4: return freeable bitmasks
> patch5: parsing of /proc/self (extends Brice's)
> patch6: numademo memtest page allocation overhead
> patch8: test/checkaffinity script
> 
> I did not apply patch 7.  I think you and Andi agreed on that.

Right.  Andi showed me the light :).

> 
> 
> The tarball is
> ftp://oss.sgi.com/www/projects/libnuma/download/numactl-2.0.3-rc3.tar.gz
> 

Thanks, 
Lee