Re: numactl-2.0.4-rc3 addition

[Cliff Wickman <cpw@sgi.com>]

Hi Lee,

On Mon, Jul 12, 2010 at 05:00:02PM -0400, Lee Schermerhorn wrote:
> On Fri, 2010-07-02 at 17:43 -0500, Cliff Wickman wrote: 
> > Hi again,
> > 
> > On Fri, Jul 02, 2010 at 03:54:38PM -0500, Cliff Wickman wrote:
> > > Hi linux-numa members,
> > > 
> > > numactl-2.0.4-rc3.tar.gz
> > > is now on ftp://oss.sgi.com/www/projects/libnuma/download/
> > > 
> > > It includes:
> > > - Fix numactl for a machine with sparse cpu ids (Anton Blanchard)
> > > - Fix makefile to remove move_pages on make clean (Andi)
> > > - Fix numa_node_to_cpus() (Sharyathi Nagesh)
> > > - Rename 'thread' to 'task' (Lee Schermerhorn)
> > > - Remove other trailing spaces (Cliff)
> > > 
> > > and recently:
> > > - Man page correction/clarification for numa_node_to_cpus() (Ian Wienand)
> > > - Man page clarification for numactl (Mike MacCana)
> > > - Fix numactl --hardware for cpu-less nodes (Thomas Renninger)
> > > - Fix set_configured_cpus() (Jan Beulich)
> > > - Fix memory corrupting use of strlen (Jan Beulich)
> > 
> > I found one other patch that for some reason had never been applied:
> > - Add a DSO destructor for memory deallocation (Neil Horman)
> > 
> > It is now included in numactl-2.0.4-rc3.tar.gz.
> 
> Cliff:
> 
> What happened to "nodes_allowed_list" ?
> 
> I'm trying to build 2.0.4-rc3 on a RHEL6b2 install and I'm seeing:
> 
> cc   numactl.o util.o shm.o bitops.o libnuma.so   -o numactl
> libnuma.so: undefined reference to `nodes_allowed_list'
> collect2: ld returned 1 exit status
> make: *** [numactl] Error 1

Huh.  Don't know why I didn't encounter that error myself.  I must
have missed a step.

 
> rc2 builds fine.  diffing libnuma.c rc2:rc3:  I see:
> 
> --- ../numactl-2.0.4-rc2/libnuma.c       2010-04-08 10:08:17.000000000 -0400
> +++ ./libnuma.c 2010-07-02 18:40:21.000000000 -0400
> @@ -54,8 +54,6 @@
>  static unsigned long *node_cpu_mask_v1[NUMA_NUM_NODES];
>  struct bitmask **node_cpu_mask_v2;
>  
> -char *nodes_allowed_list = NULL;
> -
>  WEAK void numa_error(char *where);
>  
>  #ifdef __thread
> @@ -95,6 +93,19 @@
>         memset(&numa_no_nodes, 0, sizeof(numa_no_nodes));
>  }
>  
> +void
> +numa_fini(void)
> +{
> +       if (numa_all_cpus_ptr)
> +               numa_bitmask_free(numa_all_cpus_ptr);
> +       if (numa_all_nodes_ptr)
> +               numa_bitmask_free(numa_all_nodes_ptr);
> +       if (numa_no_nodes_ptr)
> +               numa_bitmask_free(numa_no_nodes_ptr);
> +       if (nodes_allowed_list)
> +               free(nodes_allowed_list);
> +}
> +
>  /*
>   * The following bitmask declarations, bitmask_*() routines, and associated
>   * _setbit() and _getbit() routines are:
> @@ -460,12 +471,6 @@
>                         maxprocnode =
>                                 read_mask(mask, numa_all_nodes_ptr);
>                 }
> -               if (strncmp(buffer,"Mems_allowed_list:",18) == 0) {
> -                       nodes_allowed_list = malloc(strlen(buffer)-18);
> -                       strncpy(nodes_allowed_list, buffer + 19,
> -                               strlen(buffer) - 19);
> -                       nodes_allowed_list[strlen(nodes_allowed_list)-1] = '\0';
> -               }
>         }
>         fclose(f);
>         free(buffer);
> <snip>
> 
> So, it looks like the nodes_allowed_list was removed while the
> destructor was being added to free it.  Is the removal intentional?  If
> so, the patch below allows it to build.

Looks like it should have been removed, as it is not referenced.

I included your change in Neil's patch (removed the free and removed
the reference in numa.h, and in ldscript).

The -rc3 tarball is updated with that fix.

Thanks for the test and the detective work.

-Cliff


> 
> Lee
> 
> Remove free of undefined nodes_allowed_list from destructor and
> numa header.
> 
>  libnuma.c |    2 --
>  numa.h    |    2 --
>  2 files changed, 4 deletions(-)
> 
> Index: numactl-2.0.4-rc3/libnuma.c
> ===================================================================
> --- numactl-2.0.4-rc3.orig/libnuma.c	2010-07-02 18:40:21.000000000 -0400
> +++ numactl-2.0.4-rc3/libnuma.c	2010-07-12 16:52:49.000000000 -0400
> @@ -102,8 +102,6 @@ numa_fini(void)
>  		numa_bitmask_free(numa_all_nodes_ptr);
>  	if (numa_no_nodes_ptr)
>  		numa_bitmask_free(numa_no_nodes_ptr);
> -	if (nodes_allowed_list)
> -		free(nodes_allowed_list);
>  }
>  
>  /*
> Index: numactl-2.0.4-rc3/numa.h
> ===================================================================
> --- numactl-2.0.4-rc3.orig/numa.h	2010-07-02 18:40:21.000000000 -0400
> +++ numactl-2.0.4-rc3/numa.h	2010-07-12 16:52:38.000000000 -0400
> @@ -161,8 +161,6 @@ extern struct bitmask *numa_no_nodes_ptr
>  /* Source compatibility */
>  extern nodemask_t numa_no_nodes;
>  
> -extern char *nodes_allowed_list;
> -
>  /* Only run and allocate memory from a specific set of nodes. */
>  void numa_bind(struct bitmask *nodes);
>  
> 

-- 
Cliff Wickman
SGI
cpw@sgi.com
(651) 683-3824