public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [Patch] statistics infrastructure - update 10
@ 2006-07-12 12:27 Martin Peschke
  2006-07-12 16:10 ` Andrew Morton
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Peschke @ 2006-07-12 12:27 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel@vger.kernel.org

lib/statistics.c didn't compile if CONFIG_SMP wasn't defined,
because struct percpu_data was used without precautions.

lib/statistic.c: In function 'statistic_move_ptr':
lib/statistic.c:182: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_free':
lib/statistic.c:197: error: dereferencing pointer to incomplete type
lib/statistic.c:198: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_alloc':
lib/statistic.c:217: error: invalid application of 'sizeof' to
incomplete
type 'struct percpu_data'
lib/statistic.c:222: error: dereferencing pointer to incomplete type
lib/statistic.c:223: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_reset':
lib/statistic.c:297: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_merge':
lib/statistic.c:309: error: dereferencing pointer to incomplete type
lib/statistic.c: In function '_statistic_hotcpu':
lib/statistic.c:443: error: dereferencing pointer to incomplete type
lib/statistic.c:444: error: dereferencing pointer to incomplete type
lib/statistic.c:449: error: dereferencing pointer to incomplete type
lib/statistic.c:450: error: dereferencing pointer to incomplete type
lib/statistic.c:451: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_add_counter_inc':
lib/statistic.c:854: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_add_counter_prod':
lib/statistic.c:862: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_add_util':
lib/statistic.c:923: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_add_histogram_lin':
lib/statistic.c:1054: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_add_histogram_log2':
lib/statistic.c:1061: error: dereferencing pointer to incomplete type
lib/statistic.c: In function 'statistic_add_sparse':
lib/statistic.c:1270: error: dereferencing pointer to incomplete type
make[1]: *** [lib/statistic.o] Error 1
make: *** [lib] Error 2

Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 include/linux/statistic.h |    2
 lib/statistic.c           |  146 ++++++++++++++++++++++++++++------------------
 2 files changed, 92 insertions(+), 56 deletions(-)

--- a/include/linux/statistic.h	2006-07-12 13:04:47.000000000 +0200
+++ b/include/linux/statistic.h	2006-07-12 13:07:12.000000000 +0200
@@ -83,7 +83,7 @@
 /* private: */
 	enum statistic_state	 state;
 	enum statistic_type	 type;
-	struct percpu_data	*pdata;
+	void			*data;
 	void			(*add)(struct statistic *, s64, u64);
 	u64			 started;
 	u64			 stopped;
--- a/lib/statistic.c	2006-07-12 13:04:48.000000000 +0200
+++ b/lib/statistic.c	2006-07-12 13:07:14.000000000 +0200
@@ -62,6 +62,13 @@
 #include <asm/bug.h>
 #include <asm/uaccess.h>
 
+#ifdef CONFIG_SMP
+#define statistic_ptr(stat, cpu) \
+	((struct percpu_data*)((stat)->data))->ptrs[(cpu)]
+#else
+#define statistic_ptr(stat, cpu) (stat)->data
+#endif
+
 struct statistic_file_private {
 	struct list_head read_seg_lh;
 	struct list_head write_seg_lh;
@@ -179,56 +186,87 @@
 	unsigned long flags;
 	if (src) {
 		local_irq_save(flags);
-		disc->merge(stat, stat->pdata->ptrs[smp_processor_id()], src);
+		disc->merge(stat, statistic_ptr(stat, smp_processor_id()), src);
 		local_irq_restore(flags);
 	}
 }
 
-static int statistic_free(struct statistic *stat, struct statistic_info *info)
+static void statistic_free_1(struct statistic *stat)
+{
+	statistic_free_ptr(stat, stat->data);
+	stat->data = NULL;
+}
+
+#ifdef CONFIG_SMP
+static void statistic_free_n(struct statistic *stat)
 {
 	int cpu;
-	stat->state = STATISTIC_STATE_RELEASED;
-	if (unlikely(info->flags & STATISTIC_FLAGS_NOINCR)) {
-		statistic_free_ptr(stat, stat->pdata);
-		stat->pdata = NULL;
-		return 0;
-	}
 	for_each_possible_cpu(cpu) {
-		statistic_free_ptr(stat, stat->pdata->ptrs[cpu]);
-		stat->pdata->ptrs[cpu] = NULL;
+		statistic_free_ptr(stat, statistic_ptr(stat, cpu));
+		statistic_ptr(stat, cpu) = NULL;
 	}
-	kfree(stat->pdata);
-	stat->pdata = NULL;
+	kfree(stat->data);
+	stat->data = NULL;
+}
+#endif
+
+static int statistic_free(struct statistic *stat, struct statistic_info *info)
+{
+#ifdef CONFIG_SMP
+	if (unlikely(info->flags & STATISTIC_FLAGS_NOINCR))
+		statistic_free_1(stat);
+	else
+		statistic_free_n(stat);
+#else
+	statistic_free_1(stat);
+#endif
+	stat->state = STATISTIC_STATE_RELEASED;
 	return 0;
 }
 
-static int statistic_alloc(struct statistic *stat,
-			   struct statistic_info *info)
+static int statistic_alloc_1(struct statistic *stat)
 {
-	int cpu, node;
+	stat->data = statistic_alloc_ptr(stat, -1);
+	return (likely(stat->data) ? 0 : -ENOMEM);
+}
 
-	stat->age = timestamp_clock();
-	if (unlikely(info->flags & STATISTIC_FLAGS_NOINCR)) {
-		stat->pdata = statistic_alloc_ptr(stat, -1);
-		if (unlikely(!stat->pdata))
-			return -ENOMEM;
-		stat->state = STATISTIC_STATE_OFF;
-		return 0;
-	}
-	stat->pdata = kzalloc(sizeof(struct percpu_data), GFP_KERNEL);
-	if (unlikely(!stat->pdata))
+#ifdef CONFIG_SMP
+static int statistic_alloc_n(struct statistic *stat)
+{
+	int cpu, node;
+	stat->data = kzalloc(sizeof(struct percpu_data), GFP_KERNEL);
+	if (unlikely(!stat->data))
 		return -ENOMEM;
 	for_each_online_cpu(cpu) {
 		node = cpu_to_node(cpu);
-		stat->pdata->ptrs[cpu] = statistic_alloc_ptr(stat, node);
-		if (unlikely(!stat->pdata->ptrs[cpu])) {
-			statistic_free(stat, info);
+		statistic_ptr(stat, cpu) = statistic_alloc_ptr(stat, node);
+		if (unlikely(!statistic_ptr(stat, cpu)))
 			return -ENOMEM;
-		}
 	}
-	stat->state = STATISTIC_STATE_OFF;
 	return 0;
 }
+#endif
+
+static int statistic_alloc(struct statistic *stat,
+			   struct statistic_info *info)
+{
+	int retval;
+#ifdef CONFIG_SMP
+	if (unlikely(info->flags & STATISTIC_FLAGS_NOINCR))
+		retval = statistic_alloc_1(stat);
+	else
+		retval = statistic_alloc_n(stat);
+#else
+	retval = statistic_alloc_1(stat);
+#endif
+	if (likely(!retval)) {
+		stat->state = STATISTIC_STATE_OFF;
+		stat->age = timestamp_clock();
+	} else
+		statistic_free(stat, info);
+	return retval;
+
+}
 
 static int statistic_start(struct statistic *stat)
 {
@@ -292,10 +330,10 @@
 		return 0;
 	statistic_transition(stat, info, STATISTIC_STATE_OFF);
 	if (unlikely(info->flags & STATISTIC_FLAGS_NOINCR))
-		statistic_reset_ptr(stat, stat->pdata);
+		statistic_reset_ptr(stat, stat->data);
 	else
 		for_each_possible_cpu(cpu)
-			statistic_reset_ptr(stat, stat->pdata->ptrs[cpu]);
+			statistic_reset_ptr(stat, statistic_ptr(stat, cpu));
 	stat->age = timestamp_clock();
 	statistic_transition(stat, info, prev_state);
 	return 0;
@@ -307,7 +345,7 @@
 	struct statistic *stat = mpriv->stat;
 	struct statistic_discipline *disc = &statistic_discs[stat->type];
 	spin_lock(&mpriv->lock);
-	disc->merge(stat, mpriv->dst, stat->pdata->ptrs[smp_processor_id()]);
+	disc->merge(stat, mpriv->dst, statistic_ptr(stat, smp_processor_id()));
 	spin_unlock(&mpriv->lock);
 }
 
@@ -414,7 +452,7 @@
 	if (unlikely(stat->state < STATISTIC_STATE_OFF))
 		return 0;
 	if (unlikely(info->flags & STATISTIC_FLAGS_NOINCR))
-		return disc->fdata(stat, info->name, fpriv, stat->pdata);
+		return disc->fdata(stat, info->name, fpriv, stat->data);
 	mpriv.dst = statistic_alloc_ptr(stat, -1);
 	if (unlikely(!mpriv.dst))
 		return -ENOMEM;
@@ -441,15 +479,15 @@
 		return NOTIFY_OK;
 	switch (action) {
 	case CPU_UP_PREPARE:
-		stat->pdata->ptrs[cpu] = statistic_alloc_ptr(stat, node);
-		if (!stat->pdata->ptrs[cpu])
+		statistic_ptr(stat, cpu) = statistic_alloc_ptr(stat, node);
+		if (!statistic_ptr(stat, cpu))
 			return NOTIFY_BAD;
 		break;
 	case CPU_UP_CANCELED:
 	case CPU_DEAD:
-		statistic_move_ptr(stat, stat->pdata->ptrs[cpu]);
-		statistic_free_ptr(stat, stat->pdata->ptrs[cpu]);
-		stat->pdata->ptrs[cpu] = NULL;
+		statistic_move_ptr(stat, statistic_ptr(stat, cpu));
+		statistic_free_ptr(stat, statistic_ptr(stat, cpu));
+		statistic_ptr(stat, cpu) = NULL;
 		break;
 	}
 	return NOTIFY_OK;
@@ -852,7 +890,7 @@
 
 void statistic_add_counter_inc(struct statistic *stat, s64 value, u64 incr)
 {
-	*(u64*)stat->pdata->ptrs[smp_processor_id()] += incr;
+	*(u64*)statistic_ptr(stat, smp_processor_id()) += incr;
 }
 EXPORT_SYMBOL_GPL(statistic_add_counter_inc);
 
@@ -860,14 +898,14 @@
 {
 	if (unlikely(value < 0))
 		value = -value;
-	*(u64*)stat->pdata->ptrs[smp_processor_id()] += value * incr;
+	*(u64*)statistic_ptr(stat, smp_processor_id()) += value * incr;
 }
 EXPORT_SYMBOL_GPL(statistic_add_counter_prod);
 
 static void statistic_set_counter_inc(struct statistic *stat,
 				      s64 value, u64 total)
 {
-	*(u64*)stat->pdata = total;
+	*(u64*)stat->data = total;
 }
 
 static void statistic_set_counter_prod(struct statistic *stat,
@@ -875,7 +913,7 @@
 {
 	if (unlikely(value < 0))
 		value = -value;
-	*(u64*)stat->pdata = value * total;
+	*(u64*)stat->data = value * total;
 }
 
 static void statistic_merge_counter(struct statistic *stat,
@@ -920,8 +958,8 @@
 
 void statistic_add_util(struct statistic *stat, s64 value, u64 incr)
 {
-	int cpu = smp_processor_id();
-	struct statistic_entry_util *util = stat->pdata->ptrs[cpu];
+	struct statistic_entry_util *util;
+	util = statistic_ptr(stat, smp_processor_id());
 	util->num += incr;
 	util->acc += value * incr;
 	util->sqr += value * value * incr;
@@ -934,8 +972,7 @@
 
 static void statistic_set_util(struct statistic *stat, s64 value, u64 total)
 {
-	struct statistic_entry_util *util;
-	util = (struct statistic_entry_util *) stat->pdata;
+	struct statistic_entry_util *util = stat->data;
 	util->num = total;
 	util->acc = value * total;
 	util->sqr = value * value * total;
@@ -1052,14 +1089,14 @@
 void statistic_add_histogram_lin(struct statistic *stat, s64 value, u64 incr)
 {
 	int i = statistic_histogram_calc_index_lin(stat, value);
-	((u64*)stat->pdata->ptrs[smp_processor_id()])[i] += incr;
+	((u64*)statistic_ptr(stat, smp_processor_id()))[i] += incr;
 }
 EXPORT_SYMBOL_GPL(statistic_add_histogram_lin);
 
 void statistic_add_histogram_log2(struct statistic *stat, s64 value, u64 incr)
 {
 	int i = statistic_histogram_calc_index_log2(stat, value);
-	((u64*)stat->pdata->ptrs[smp_processor_id()])[i] += incr;
+	((u64*)statistic_ptr(stat, smp_processor_id()))[i] += incr;
 }
 EXPORT_SYMBOL_GPL(statistic_add_histogram_log2);
 
@@ -1067,14 +1104,14 @@
 					s64 value, u64 total)
 {
 	int i = statistic_histogram_calc_index_lin(stat, value);
-	((u64*)stat->pdata)[i] = total;
+	((u64*)stat->data)[i] = total;
 }
 
 static void statistic_set_histogram_log2(struct statistic *stat,
 					 s64 value, u64 total)
 {
 	int i = statistic_histogram_calc_index_log2(stat, value);
-	((u64*)stat->pdata)[i] = total;
+	((u64*)stat->data)[i] = total;
 }
 
 static void statistic_merge_histogram(struct statistic *stat,
@@ -1267,16 +1304,15 @@
 
 void statistic_add_sparse(struct statistic *stat, s64 value, u64 incr)
 {
-	int cpu = smp_processor_id();
-	struct statistic_sparse_list *slist = stat->pdata->ptrs[cpu];
+	struct statistic_sparse_list *slist;
+	slist = statistic_ptr(stat, smp_processor_id());
 	_statistic_add_sparse(slist, value, incr);
 }
 EXPORT_SYMBOL_GPL(statistic_add_sparse);
 
 static void statistic_set_sparse(struct statistic *stat, s64 value, u64 total)
 {
-	struct statistic_sparse_list *slist = (struct statistic_sparse_list *)
-						stat->pdata;
+	struct statistic_sparse_list *slist = stat->data;
 	struct list_head *head = &slist->entry_lh;
 	struct statistic_entry_sparse *entry;
 




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch] statistics infrastructure - update 10
  2006-07-12 12:27 [Patch] statistics infrastructure - update 10 Martin Peschke
@ 2006-07-12 16:10 ` Andrew Morton
  2006-07-12 16:45   ` Martin Peschke
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2006-07-12 16:10 UTC (permalink / raw)
  To: Martin Peschke; +Cc: linux-kernel

On Wed, 12 Jul 2006 14:27:39 +0200
Martin Peschke <mp3@de.ibm.com> wrote:

> +#define statistic_ptr(stat, cpu) \
> +	((struct percpu_data*)((stat)->data))->ptrs[(cpu)]

This would be the only part of the kernel which uses percpu_data directly -
everything else uses the APIs (ie: per_cpu_ptr()).  How come?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch] statistics infrastructure - update 10
  2006-07-12 16:10 ` Andrew Morton
@ 2006-07-12 16:45   ` Martin Peschke
  2006-07-13  8:00     ` Andrew Morton
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Peschke @ 2006-07-12 16:45 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

On Wed, 2006-07-12 at 09:10 -0700, Andrew Morton wrote:
> On Wed, 12 Jul 2006 14:27:39 +0200
> Martin Peschke <mp3@de.ibm.com> wrote:
> 
> > +#define statistic_ptr(stat, cpu) \
> > +	((struct percpu_data*)((stat)->data))->ptrs[(cpu)]
> 
> This would be the only part of the kernel which uses percpu_data directly -
> everything else uses the APIs (ie: per_cpu_ptr()).  How come?

The API, i.e. per_cpu_ptr(), doesn't allow to assign a value to any of
the pointers in struct percpu_data. I need that capability because I
make use of cpu hotplug notifications to fix per-cpu data at run time.
With regard to memory footprint this is much more efficient than using
alloc_percpu().

Is it be preferable to add something like set_per_cpu_ptr() to the API?


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch] statistics infrastructure - update 10
  2006-07-12 16:45   ` Martin Peschke
@ 2006-07-13  8:00     ` Andrew Morton
  2006-07-13 11:12       ` Martin Peschke
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2006-07-13  8:00 UTC (permalink / raw)
  To: Martin Peschke; +Cc: linux-kernel

On Wed, 12 Jul 2006 18:45:08 +0200
Martin Peschke <mp3@de.ibm.com> wrote:

> On Wed, 2006-07-12 at 09:10 -0700, Andrew Morton wrote:
> > On Wed, 12 Jul 2006 14:27:39 +0200
> > Martin Peschke <mp3@de.ibm.com> wrote:
> > 
> > > +#define statistic_ptr(stat, cpu) \
> > > +	((struct percpu_data*)((stat)->data))->ptrs[(cpu)]
> > 
> > This would be the only part of the kernel which uses percpu_data directly -
> > everything else uses the APIs (ie: per_cpu_ptr()).  How come?
> 
> The API, i.e. per_cpu_ptr(), doesn't allow to assign a value to any of
> the pointers in struct percpu_data. I need that capability because I
> make use of cpu hotplug notifications to fix per-cpu data at run time.

Fair enough, I guess.

> With regard to memory footprint this is much more efficient than using
> alloc_percpu().

How much storage are we talking about here?  I find it a bit hard to work
that out.

> Is it be preferable to add something like set_per_cpu_ptr() to the API?

hm.  Add a generic extension to a generic interface within a specific
subsystem versus doing it generically.  Hard call ;)


I'd suggest that you:

- Create a new __alloc_percpu_mask(size_t size, cpumask_t cpus)

- Make that function use your newly added

	percpu_data_populate(struct percpu_data *p, int cpu, size_t size, gfp_t gfp);

	(maybe put `size' into 'struct percpu_data'?)

- implement __alloc_percpu() as __alloc_percpu_mask(size, cpu_possible_map)

- hack around madly until it compiles on uniprocessor.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch] statistics infrastructure - update 10
  2006-07-13  8:00     ` Andrew Morton
@ 2006-07-13 11:12       ` Martin Peschke
  2006-07-13 14:43         ` Andrew Morton
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Peschke @ 2006-07-13 11:12 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

Andrew Morton wrote:
> On Wed, 12 Jul 2006 18:45:08 +0200
> Martin Peschke <mp3@de.ibm.com> wrote:
> 
>> On Wed, 2006-07-12 at 09:10 -0700, Andrew Morton wrote:
>>> On Wed, 12 Jul 2006 14:27:39 +0200
>>> Martin Peschke <mp3@de.ibm.com> wrote:
>>>
>>>> +#define statistic_ptr(stat, cpu) \
>>>> +	((struct percpu_data*)((stat)->data))->ptrs[(cpu)]
>>>
>>> This would be the only part of the kernel which uses percpu_data directly -
>>> everything else uses the APIs (ie: per_cpu_ptr()).  How come?
>>
>> The API, i.e. per_cpu_ptr(), doesn't allow to assign a value to any of
>> the pointers in struct percpu_data. I need that capability because I
>> make use of cpu hotplug notifications to fix per-cpu data at run time.
> 
> Fair enough, I guess.
> 
>> With regard to memory footprint this is much more efficient than using
>> alloc_percpu().
> 
> How much storage are we talking about here?  I find it a bit hard to work
> that out.

32 CPUs appears to be default during kernel build. My z/VM guest happens
to have 4 (virtual) CPUs right now. With alloc_percpu() the wasted/used
ratio would be 8:1.

Given one small logarithmic histogram (buckets: <=0, <=1, <=2, <=4, ...
<=1024, >1024) that would be a waste of:
next_power_of_2(13 buckets * 8 bytes/bucket) * 8 = 1kB.

>> Is it be preferable to add something like set_per_cpu_ptr() to the API?
> 
> hm.  Add a generic extension to a generic interface within a specific
> subsystem versus doing it generically.  Hard call ;)

pretty warm outside... making me lazy ;)

> I'd suggest that you:
> 
> - Create a new __alloc_percpu_mask(size_t size, cpumask_t cpus)
> 
> - Make that function use your newly added
> 
> 	percpu_data_populate(struct percpu_data *p, int cpu, size_t size, gfp_t gfp);
> 
> 	(maybe put `size' into 'struct percpu_data'?)
> 
> - implement __alloc_percpu() as __alloc_percpu_mask(size, cpu_possible_map)

Getting at the root of the problem. I will have a shot at it.
(It will take til next week, though - pretty warm outside...)

A question:
For symmetry's sake, should I add __free_percpu_mask(), which would
put NULL where __alloc_percpu_mask() has put a valid address earlier?
Otherwise, per_cpu_ptr() would return !NULL for an object released
during cpu hotunplug handling.
Or, is this not an issue because some cpu mask indicates that the cpu
is offline anyway, and that the contents of the pointer is not valid.

> - hack around madly until it compiles on uniprocessor.

;)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch] statistics infrastructure - update 10
  2006-07-13 11:12       ` Martin Peschke
@ 2006-07-13 14:43         ` Andrew Morton
  2006-07-24 17:15           ` Martin Peschke
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2006-07-13 14:43 UTC (permalink / raw)
  To: Martin Peschke; +Cc: linux-kernel

On Thu, 13 Jul 2006 13:12:27 +0200
Martin Peschke <mp3@de.ibm.com> wrote:

> > I'd suggest that you:
> > 
> > - Create a new __alloc_percpu_mask(size_t size, cpumask_t cpus)
> > 
> > - Make that function use your newly added
> > 
> > 	percpu_data_populate(struct percpu_data *p, int cpu, size_t size, gfp_t gfp);
> > 
> > 	(maybe put `size' into 'struct percpu_data'?)
> > 
> > - implement __alloc_percpu() as __alloc_percpu_mask(size, cpu_possible_map)
> 
> Getting at the root of the problem. I will have a shot at it.
> (It will take til next week, though - pretty warm outside...)
> 
> A question:
> For symmetry's sake, should I add __free_percpu_mask(), which would
> put NULL where __alloc_percpu_mask() has put a valid address earlier?
> Otherwise, per_cpu_ptr() would return !NULL for an object released
> during cpu hotunplug handling.
> Or, is this not an issue because some cpu mask indicates that the cpu
> is offline anyway, and that the contents of the pointer is not valid.

Sure, we need a way of freeing a cpu's storage and of zapping that CPU's
slot.  Whether that's mask-based or just operates on a single CPU is
debatable.  Probably the latter, given the do-it-at-hotplug-time usage
model.


It could be argued that the whole idea is wrong - that we're putting
restrictions upon the implementation of alloc_percpu().  After all, an
implementation at present could do

alloc_percpu(size):
	size = roundup(size, L1_CACHE_SIZE);
	ret = kmalloc(size*NR_CPUS + sizeof(int));
	*(int *)ret = size;

per_cpu_ptr(ptr, cpu):
	(void *)((int *)ptr + (*((int *)ptr) * cpu))

or whatever.  The API additions which are being proposed here make that
impossible.  Or at least, more complex and slower.

Is it reasonable to assume that all implementations will, for all time,
include at least one layer of indirection?  After all, the above would be a
feasible implementation for non-NUMA SMP.  It's just as many derefs though.

hmm.  1k of memory isn't much.  How much memory will all this _really_ save?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch] statistics infrastructure - update 10
  2006-07-13 14:43         ` Andrew Morton
@ 2006-07-24 17:15           ` Martin Peschke
  0 siblings, 0 replies; 7+ messages in thread
From: Martin Peschke @ 2006-07-24 17:15 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

On Thu, 2006-07-13 at 07:43 -0700, Andrew Morton wrote:
> On Thu, 13 Jul 2006 13:12:27 +0200
> Martin Peschke <mp3@de.ibm.com> wrote:
> 
> > > I'd suggest that you:
> > > 
> > > - Create a new __alloc_percpu_mask(size_t size, cpumask_t cpus)
> > > 
> > > - Make that function use your newly added
> > > 
> > > 	percpu_data_populate(struct percpu_data *p, int cpu, size_t size, gfp_t gfp);
> > > 
> > > 	(maybe put `size' into 'struct percpu_data'?)
> > > 
> > > - implement __alloc_percpu() as __alloc_percpu_mask(size, cpu_possible_map)
> > 
> > Getting at the root of the problem. I will have a shot at it.
> > (It will take til next week, though - pretty warm outside...)
> > 
> > A question:
> > For symmetry's sake, should I add __free_percpu_mask(), which would
> > put NULL where __alloc_percpu_mask() has put a valid address earlier?
> > Otherwise, per_cpu_ptr() would return !NULL for an object released
> > during cpu hotunplug handling.
> > Or, is this not an issue because some cpu mask indicates that the cpu
> > is offline anyway, and that the contents of the pointer is not valid.
> 
> Sure, we need a way of freeing a cpu's storage and of zapping that CPU's
> slot.  Whether that's mask-based or just operates on a single CPU is
> debatable.  Probably the latter, given the do-it-at-hotplug-time usage
> model.

My initial patch provides both (cpu-based and mask-based). I will remove
one method if feedback seconds this assumption.

> It could be argued that the whole idea is wrong - that we're putting
> restrictions upon the implementation of alloc_percpu().  After all, an
> implementation at present could do
> 
> alloc_percpu(size):
> 	size = roundup(size, L1_CACHE_SIZE);
> 	ret = kmalloc(size*NR_CPUS + sizeof(int));
> 	*(int *)ret = size;
> 
> per_cpu_ptr(ptr, cpu):
> 	(void *)((int *)ptr + (*((int *)ptr) * cpu))
> 
> or whatever.  The API additions which are being proposed here make that
> impossible.  Or at least, more complex and slower.

I don't think so.

> Is it reasonable to assume that all implementations will, for all time,
> include at least one layer of indirection?  After all, the above would be a
> feasible implementation for non-NUMA SMP.  It's just as many derefs though.

An enhanced API doesn't need to expose how per-cpu data is actually
organised, i.e. struct percpu_data. That is, archs are still free to
implement per-cpu data in different ways.

The patch allows to make the dynamic allocation, or removal,
respectively, of per-cpu data a two-stage process:

a1) initial (partial) allocation
a2) further population as needed (i.e. when CPU is coming online)

b1) (partial) depopulation as needed (i.e. when CPU is going offline)
b2) final removal

If some arch-optimisation hindered changing objects at run time, well,
a2) and b1 would be just NOPs, leaving all the work to a1) and b2). The
performance of per_cpu_ptr() won't be affected, anyway.

Does this address your qualms?

> hmm.  1k of memory isn't much.  How much memory will all this _really_ save?

1k for a single (sample) object doesn't sound that much.
But how big will (statistic) ojects really be? I can't tell.
And how many objects will be there? I don't know.

'grep -r CPU_UP_PREPARE' indicates that there are people who are worried
about memory consumption of (unused) per-cpu data.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2006-07-24 17:15 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-07-12 12:27 [Patch] statistics infrastructure - update 10 Martin Peschke
2006-07-12 16:10 ` Andrew Morton
2006-07-12 16:45   ` Martin Peschke
2006-07-13  8:00     ` Andrew Morton
2006-07-13 11:12       ` Martin Peschke
2006-07-13 14:43         ` Andrew Morton
2006-07-24 17:15           ` Martin Peschke

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox