Re: (re)name cpumem_realloc to cpumem_malloc_ncpus

2016-10-24 Thread David Gwynne 
thank you for looking at this.

ive committed it with tweaks based on your suggestions.

> On 24 Oct 2016, at 22:15, Alexander Bluhm  wrote:
> 
> On Mon, Oct 24, 2016 at 02:48:03PM +1000, David Gwynne wrote:
>> cos its not resizing the allocation, its allocating them for new cpus.
> 
> realloc is not a good name, so yes to renaming.
> 
>> the same goes for counters_realloc being named counters_alloc_ncpus.
> 
> Does the n mean new?  This is never mentioned, I initially thought
> it means number.  What about ..._newcpus or ..._allcpus?
> 
>> +.Fn COUNTERS_BOOT_INITIALIZER
>> +is used to initialise a cpumem pointer with the memory that was previously
>> +allocated using
>> +.Fn COUNTERS_BOOT_MEMORY
>> +and identified by
>> +.Fa NAME .
> 
> The boot counters are zero initially.
> 
>> +The
>> +.Fa type
>> +argument specifies the type of memory that the counters will be
>> +allocated as via
>> +.Xr malloc 9 .
>> +The memory will be zeroed on allocation by passing
>> +.Fn M_ZERO
> 
> Is the M_ZERO an implementation detail for counters that should not
> be mentionend in the man page?
> 
>> +to
>> +.Xr malloc 9 .
> 
> The counters on the boot cpu are preserved.
> 
>> +.Fn CPUMEM_BOOT_INITIALIZER
>> +is used to initialise a cpumem pointer with the memory that was previously
>> +allocated using
>> +.Fn CPUMEM_BOOT_MEMORY
>> +and identified by
>> +.Fa NAME .
> 
> The boot cpu uses static memory that is initially zero.
> 
>> +.Fn cpumem_malloc_ncpus
>> +allocates
>> +.Fa sz
>> +bytes of
>> +.Fa type
>> +memory for each additional CPU using
>> +.Xr malloc 9 .
>> +The memory will be zeroed on allocation by passing
>> +.Fn M_ZERO
>> +to
>> +.Xr malloc 9 .
> 
> The the memory of the boot cpu is preserved.
> 
>> ===
>> RCS file: /cvs/src/sys/kern/subr_percpu.c,v
>> retrieving revision 1.3
>> diff -u -p -r1.3 subr_percpu.c
>> --- sys/kern/subr_percpu.c   24 Oct 2016 03:15:38 -  1.3
>> +++ sys/kern/subr_percpu.c   24 Oct 2016 04:43:34 -
>> @@ -76,7 +76,7 @@ cpumem_malloc(size_t sz, int type)
>> }
>> 
>> struct cpumem *
>> -cpumem_realloc(struct cpumem *bootcm, size_t sz, int type)
>> +cpumem_alloc_ncpus(struct cpumem *bootcm, size_t sz, int type)
> 
> This should be cpumem_malloc_ncpus.
> 
>> {
>>  struct cpumem *cm;
>>  unsigned int cpu;
>> @@ -146,10 +146,10 @@ counters_alloc(unsigned int n, int type)
>> }
>> 
>> struct cpumem *
>> -counters_realloc(struct cpumem *cm, unsigned int n, int type)
>> +counters_alloc_ncpus(struct cpumem *cm, unsigned int n, int type)
>> {
>>  n++; /* the generation number */
>> -return (cpumem_realloc(cm, n * sizeof(uint64_t), type));
>> +return (cpumem_alloc_ncpus(cm, n * sizeof(uint64_t), type));
> 
> This should be cpumem_malloc_ncpus.
> 
>> }
>> 
>> void
>> @@ -259,7 +259,7 @@ cpumem_malloc(size_t sz, int type)
>> }
>> 
>> struct cpumem *
>> -cpumem_realloc(struct cpumem *cm, size_t sz, int type)
>> +cpumem_allod_ncpus(struct cpumem *cm, size_t sz, int type)
> 
> This should be cpumem_malloc_ncpus.
> 
>> {
>>  return (cm);
>> }
>> @@ -291,10 +291,10 @@ counters_alloc(unsigned int n, int type)
>> }
>> 
>> struct cpumem *
>> -counters_realloc(struct cpumem *cm, unsigned int n, int type)
>> +counters_alloc_ncpus(struct cpumem *cm, unsigned int n, int type)
>> {
>>  /* this is unecessary, but symmetrical */
>> -return (cpumem_realloc(cm, n * sizeof(uint64_t), type));
>> +return (cpumem_alloc_ncpus(cm, n * sizeof(uint64_t), type));
> 
> This should be cpumem_malloc_ncpus.
> 
>> }
>> 
>> void
>> Index: sys/sys/percpu.h
>> ===
>> RCS file: /cvs/src/sys/sys/percpu.h,v
>> retrieving revision 1.2
>> diff -u -p -r1.2 percpu.h
>> --- sys/sys/percpu.h 24 Oct 2016 03:15:35 -  1.2
>> +++ sys/sys/percpu.h 24 Oct 2016 04:43:34 -
>> @@ -54,7 +54,7 @@ struct cpumem  *cpumem_get(struct pool *)
>> void  cpumem_put(struct pool *, struct cpumem *);
>> 
>> struct cpumem*cpumem_malloc(size_t, int);
>> -struct cpumem   *cpumem_realloc(struct cpumem *, size_t, int);
>> +struct cpumem   *cpumem_malloc_ncpus(struct cpumem *, size_t, int);
> 
> I wonder how this compiled as you use cpumem_malloc_ncpus correctly
> here.
> 
>> void  cpumem_free(struct cpumem *, int, size_t);
>> 
>> void *cpumem_first(struct cpumem_iter *, struct cpumem *);
>> @@ -111,7 +111,7 @@ static struct {  
>> \
>>  */
>> 
>> struct cpumem*counters_alloc(unsigned int, int);
>> -struct cpumem   *counters_realloc(struct cpumem *, unsigned int, int);
>> +struct cpumem   *counters_alloc_ncpus(struct cpumem *, unsigned int, 
>> int);
>> void  counters_free(struct cpumem *, int, unsigned int);
>> void  counters_read(struct cpumem *, uint64_t *, unsigned int);
>> void  counters_zero(struct cpumem *, unsigned

Re: (re)name cpumem_realloc to cpumem_malloc_ncpus

2016-10-24 Thread Alexander Bluhm 
On Mon, Oct 24, 2016 at 02:48:03PM +1000, David Gwynne wrote:
> cos its not resizing the allocation, its allocating them for new cpus.

realloc is not a good name, so yes to renaming.

> the same goes for counters_realloc being named counters_alloc_ncpus.

Does the n mean new?  This is never mentioned, I initially thought
it means number.  What about ..._newcpus or ..._allcpus?

> +.Fn COUNTERS_BOOT_INITIALIZER
> +is used to initialise a cpumem pointer with the memory that was previously
> +allocated using
> +.Fn COUNTERS_BOOT_MEMORY
> +and identified by
> +.Fa NAME .

The boot counters are zero initially.

> +The
> +.Fa type
> +argument specifies the type of memory that the counters will be
> +allocated as via
> +.Xr malloc 9 .
> +The memory will be zeroed on allocation by passing
> +.Fn M_ZERO

Is the M_ZERO an implementation detail for counters that should not
be mentionend in the man page?

> +to
> +.Xr malloc 9 .

The counters on the boot cpu are preserved.

> +.Fn CPUMEM_BOOT_INITIALIZER
> +is used to initialise a cpumem pointer with the memory that was previously
> +allocated using
> +.Fn CPUMEM_BOOT_MEMORY
> +and identified by
> +.Fa NAME .

The boot cpu uses static memory that is initially zero.

> +.Fn cpumem_malloc_ncpus
> +allocates
> +.Fa sz
> +bytes of
> +.Fa type
> +memory for each additional CPU using
> +.Xr malloc 9 .
> +The memory will be zeroed on allocation by passing
> +.Fn M_ZERO
> +to
> +.Xr malloc 9 .

The the memory of the boot cpu is preserved.

> ===
> RCS file: /cvs/src/sys/kern/subr_percpu.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 subr_percpu.c
> --- sys/kern/subr_percpu.c24 Oct 2016 03:15:38 -  1.3
> +++ sys/kern/subr_percpu.c24 Oct 2016 04:43:34 -
> @@ -76,7 +76,7 @@ cpumem_malloc(size_t sz, int type)
>  }
>  
>  struct cpumem *
> -cpumem_realloc(struct cpumem *bootcm, size_t sz, int type)
> +cpumem_alloc_ncpus(struct cpumem *bootcm, size_t sz, int type)

This should be cpumem_malloc_ncpus.

>  {
>   struct cpumem *cm;
>   unsigned int cpu;
> @@ -146,10 +146,10 @@ counters_alloc(unsigned int n, int type)
>  }
>  
>  struct cpumem *
> -counters_realloc(struct cpumem *cm, unsigned int n, int type)
> +counters_alloc_ncpus(struct cpumem *cm, unsigned int n, int type)
>  {
>   n++; /* the generation number */
> - return (cpumem_realloc(cm, n * sizeof(uint64_t), type));
> + return (cpumem_alloc_ncpus(cm, n * sizeof(uint64_t), type));

This should be cpumem_malloc_ncpus.

>  }
>  
>  void
> @@ -259,7 +259,7 @@ cpumem_malloc(size_t sz, int type)
>  }
>  
>  struct cpumem *
> -cpumem_realloc(struct cpumem *cm, size_t sz, int type)
> +cpumem_allod_ncpus(struct cpumem *cm, size_t sz, int type)

This should be cpumem_malloc_ncpus.

>  {
>   return (cm);
>  }
> @@ -291,10 +291,10 @@ counters_alloc(unsigned int n, int type)
>  }
>  
>  struct cpumem *
> -counters_realloc(struct cpumem *cm, unsigned int n, int type)
> +counters_alloc_ncpus(struct cpumem *cm, unsigned int n, int type)
>  {
>   /* this is unecessary, but symmetrical */
> - return (cpumem_realloc(cm, n * sizeof(uint64_t), type));
> + return (cpumem_alloc_ncpus(cm, n * sizeof(uint64_t), type));

This should be cpumem_malloc_ncpus.

>  }
>  
>  void
> Index: sys/sys/percpu.h
> ===
> RCS file: /cvs/src/sys/sys/percpu.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 percpu.h
> --- sys/sys/percpu.h  24 Oct 2016 03:15:35 -  1.2
> +++ sys/sys/percpu.h  24 Oct 2016 04:43:34 -
> @@ -54,7 +54,7 @@ struct cpumem   *cpumem_get(struct pool *)
>  void  cpumem_put(struct pool *, struct cpumem *);
>  
>  struct cpumem*cpumem_malloc(size_t, int);
> -struct cpumem*cpumem_realloc(struct cpumem *, size_t, int);
> +struct cpumem*cpumem_malloc_ncpus(struct cpumem *, size_t, int);

I wonder how this compiled as you use cpumem_malloc_ncpus correctly
here.

>  void  cpumem_free(struct cpumem *, int, size_t);
>  
>  void *cpumem_first(struct cpumem_iter *, struct cpumem *);
> @@ -111,7 +111,7 @@ static struct {   
> \
>   */
>  
>  struct cpumem*counters_alloc(unsigned int, int);
> -struct cpumem*counters_realloc(struct cpumem *, unsigned int, int);
> +struct cpumem*counters_alloc_ncpus(struct cpumem *, unsigned int, 
> int);
>  void  counters_free(struct cpumem *, int, unsigned int);
>  void  counters_read(struct cpumem *, uint64_t *, unsigned int);
>  void  counters_zero(struct cpumem *, unsigned int);

Re: (re)name cpumem_realloc to cpumem_malloc_ncpus

2016-10-24 Thread Mathieu - 
David Gwynne wrote:
> cos its not resizing the allocation, its allocating them for new cpus.
> 
> the same goes for counters_realloc being named counters_alloc_ncpus.
> 
> this adds doco for them too.

Hi,
FWIW it makes sense to me, for when I was looking at the per-cpu mbstat
diff, I blocked a bit  on 'init' doing a realloc, and had to look at the
impl to understand its purpose.

My 2 cents..

(re)name cpumem_realloc to cpumem_malloc_ncpus

2016-10-23 Thread David Gwynne 
cos its not resizing the allocation, its allocating them for new cpus.

the same goes for counters_realloc being named counters_alloc_ncpus.

this adds doco for them too.

ok?

Index: share/man/man9/counters_alloc.9
===
RCS file: /cvs/src/share/man/man9/counters_alloc.9,v
retrieving revision 1.2
diff -u -p -r1.2 counters_alloc.9
--- share/man/man9/counters_alloc.9 21 Oct 2016 12:39:13 -  1.2
+++ share/man/man9/counters_alloc.9 24 Oct 2016 04:43:33 -
@@ -20,6 +20,9 @@
 .Sh NAME
 .Nm counters_alloc ,
 .Nm counters_free ,
+.Nm COUNTERS_BOOT_MEMORY ,
+.Nm COUNTERS_BOOT_INITIALIZER ,
+.Nm counters_alloc_ncpus ,
 .Nm counters_enter ,
 .Nm counters_leave ,
 .Nm counters_read ,
@@ -31,6 +34,14 @@
 .Fn counters_alloc "unsigned int ncounters" "int type"
 .Ft void
 .Fn counters_free "struct cpumem *cm" "unsigned int ncounters" "int type"
+.Fn COUNTERS_BOOT_MEMORY "NAME" "unsigned int ncounters"
+.Fn COUNTERS_BOOT_INITIALIZER "NAME"
+.Ft struct cpumemt *
+.Fo counters_alloc_ncpus
+.Fa "struct cpumem *cm"
+.Fa "unsigned int ncounters"
+.Fa "int type"
+.Fc
 .Ft uint64_t *
 .Fn counters_enter "struct counters_ref *ref" "struct cpumem *cm"
 .Ft void
@@ -84,6 +95,60 @@ arguments type originally provided to
 must be passed to
 .Fn counters_free .
 .Pp
+If a set of per CPU counters needs to be usable as soon as the
+kernel is booted, instead of after devices (which includes CPUs)
+are attached, a cpumem pointer and counters for the boot CPU may be
+statically allocated.
+.Pp
+.Fn COUNTERS_BOOT_MEMORY
+allocates counters for use by the boot CPU in the kernel and before
+the other CPUs are attached and run.
+The allocation is identified by
+.Fa NAME ,
+and provides memory for the number of counters specified by
+.Fa ncounters .
+.Pp
+.Pp
+.Fn COUNTERS_BOOT_INITIALIZER
+is used to initialise a cpumem pointer with the memory that was previously
+allocated using
+.Fn COUNTERS_BOOT_MEMORY
+and identified by
+.Fa NAME .
+.Pp
+.Fn counters_alloc_ncpus
+allocates additional per CPU counters for the CPUs that were attached
+during boot to the cpumem structure
+.Fa cm
+that was previously allocated statically using
+.Fn COUNTERS_BOOT_MEMORY
+and initialised with
+.Fn COUNTERS_BOOT_INITIALIZER .
+.Fn counters_alloc_ncpus
+allocates space for a number of counters specified by
+.Fa ncounters .
+The
+.Fa type
+argument specifies the type of memory that the counters will be
+allocated as via
+.Xr malloc 9 .
+The memory will be zeroed on allocation by passing
+.Fn M_ZERO
+to
+.Xr malloc 9 .
+The same number of counters originally passed to
+.Fa COUNTERS_BOOT_MEMORY
+must be specified by
+.Fa ncounters .
+.Pp
+Counters that have been allocated with
+.Fn COUNTERS_BOOT_MEMORY
+and
+.Fn counters_alloc_ncpus
+cannot be deallocated with
+.Fa counters_free .
+Any attempt to do so will lead to undefined behaviour.
+.Pp
 .Fn counters_enter
 provides access to the current CPU's set of counters referenced by
 .Fa cm .
@@ -119,6 +184,7 @@ up to the caller to serialise this call 
 .Sh CONTEXT
 .Fn counters_alloc ,
 .Fn counters_free ,
+.Fn counters_alloc_ncpus ,
 and
 .Fn counters_read
 may be called during autoconf, or from process context.
@@ -136,7 +202,9 @@ It is up to the caller to provide approp
 around calls to these functions to prevent concurrent access to the
 relevant data structures.
 .Sh RETURN VALUES
-.Fn counters_get
+.Fn counters_alloc
+and
+.Fn counters_alloc_ncpus
 will return an opaque cpumem pointer that references each CPU's
 set of counters.
 .Pp
Index: share/man/man9/cpumem_get.9
===
RCS file: /cvs/src/share/man/man9/cpumem_get.9,v
retrieving revision 1.5
diff -u -p -r1.5 cpumem_get.9
--- share/man/man9/cpumem_get.9 21 Oct 2016 15:00:30 -  1.5
+++ share/man/man9/cpumem_get.9 24 Oct 2016 04:43:33 -
@@ -21,7 +21,10 @@
 .Nm cpumem_get ,
 .Nm cpumem_put ,
 .Nm cpumem_malloc ,
+.Nm cpumem_malloc_ncpus ,
 .Nm cpumem_free ,
+.Nm CPUMEM_BOOT_MEMORY ,
+.Nm CPUMEM_BOOT_INITIALIZER ,
 .Nm cpumem_enter ,
 .Nm cpumem_leave ,
 .Nm cpumem_first ,
@@ -37,6 +40,10 @@
 .Fn cpumem_malloc "size_t sz" "int type"
 .Ft void
 .Fn cpumem_free "struct cpumem *cm" "int type" "size_t sz"
+.Fn CPUMEM_BOOT_MEMORY "NAME" "size_t sz"
+.Fn CPUMEM_BOOT_INITIALIZER "NAME"
+.Ft struct cpumem *
+.Fn cpumem_malloc_ncpus "struct cpumem *cm" "size_t sz" "int type"
 .Ft void *
 .Fn cpumem_enter "struct cpumem *cm"
 .Ft void
@@ -100,6 +107,59 @@ must be specified by
 and
 .Fa type
 respectively.
+.Pp
+If a per CPU memory allocation needs to be usable as soon as the
+kernel is booted, instead of after devices (which includes CPUs)
+are attached, a cpumem pointer and memory for the boot CPU may be
+statically allocated.
+.Pp
+.Fn CPUMEM_BOOT_MEMORY
+allocates memory for use by the boot CPU in the kernel and before
+the other CPUs are attached and run.  The allocation is identified
+by
+.Fn NAME ,
+and provides
+.Fn