[Patch] statistics infrastructure - update 9

[Patch] statistics infrastructure - update 9

[Martin Peschke]

I broke !CONFIG_STATISTICS. This is the fix.

drivers/s390/built-in.o(.text+0x67cae): In function
`zfcp_ccw_set_online':
zfcp_ccw.c: undefined reference to `statistic_create'
drivers/s390/built-in.o(.text+0x67cf0):zfcp_ccw.c: undefined reference
to `statistic_remove'
<snip>

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

 include/linux/statistic.h |   63 +++++++++++++++++++++++++++++++++++++---------
 lib/statistic.c           |   10 -------
 2 files changed, 51 insertions(+), 22 deletions(-)

diff -urp a/include/linux/statistic.h b/include/linux/statistic.h
--- a/include/linux/statistic.h	2006-07-03 17:39:48.000000000 +0200
+++ b/include/linux/statistic.h	2006-07-03 17:37:24.000000000 +0200
@@ -125,6 +125,8 @@ struct statistic_interface {
 	void			*pull_private;
 };
 
+#ifdef CONFIG_STATISTICS
+
 extern int statistic_create(struct statistic_interface *, const char *);
 extern int statistic_remove(struct statistic_interface *);
 
@@ -133,12 +135,6 @@ extern void statistic_set(struct statist
 extern void _statistic_add(struct statistic *, int, s64, u64);
 extern void statistic_add(struct statistic *, int, s64, u64);
 
-#define _statistic_inc(stat, i, value) \
-	_statistic_add(stat, i, value, 1)
-
-#define statistic_inc(stat, i, value) \
-	statistic_add(stat, i, value, 1)
-
 /*
  * Clients are not supposed to call these directly.
  * The declarations are needed to allow optimisation of _statistic_add_as()
@@ -173,9 +169,8 @@ extern void statistic_add_sparse(struct 
  * You may want to use _statistic_inc_as() for (X, 1) data pairs.
  */
 static inline void _statistic_add_as(int type, struct statistic *stat, int i,
-		       s64 value, u64 incr)
+				     s64 value, u64 incr)
 {
-#ifdef CONFIG_STATISTICS
 	if (stat[i].state == STATISTIC_STATE_ON) {
 		switch (type) {
 		case STAT_CNTR_INC:
@@ -198,7 +193,6 @@ static inline void _statistic_add_as(int
 			break;
 		}
 	}
-#endif
 }
 
 /**
@@ -223,16 +217,61 @@ static inline void _statistic_add_as(int
  * You may want to use statistic_inc() for (X, 1) data pairs.
  */
 static inline void statistic_add_as(int type, struct statistic *stat, int i,
-		      s64 value, u64 incr)
+				    s64 value, u64 incr)
 {
-#ifdef CONFIG_STATISTICS
 	unsigned long flags;
 	local_irq_save(flags);
 	_statistic_add_as(type, stat, i, value, incr);
 	local_irq_restore(flags);
-#endif
 }
 
+#else /* !CONFIG_STATISTICS */
+/* These NOP functions unburden clients from handling !CONFIG_STATISTICS. */
+
+static inline int statistic_create(struct statistic_interface *interface,
+				   const char *name)
+{
+	return 0;
+}
+
+static inline int statistic_remove(struct statistic_interface *interface)
+{
+	return 0;
+}
+
+static inline void statistic_set(struct statistic *stat, int i,
+				 s64 value, u64 total)
+{
+}
+
+static inline void _statistic_add(struct statistic *stat, int i,
+				  s64 value, u64 incr)
+{
+}
+
+static inline void statistic_add(struct statistic *stat, int i,
+				 s64 value, u64 incr)
+{
+}
+
+static inline void _statistic_add_as(int type, struct statistic *stat, int i,
+				     s64 value, u64 incr)
+{
+}
+
+static inline void statistic_add_as(int type, struct statistic *stat, int i,
+				    s64 value, u64 incr)
+{
+}
+
+#endif /* CONFIG_STATISTICS */
+
+#define _statistic_inc(stat, i, value) \
+	_statistic_add(stat, i, value, 1)
+
+#define statistic_inc(stat, i, value) \
+	statistic_add(stat, i, value, 1)
+
 #define _statistic_inc_as(type, stat, i, value) \
 	_statistic_add_as(type, stat, i, value, 1)
 
diff -urp a/lib/statistic.c b/lib/statistic.c
--- a/lib/statistic.c	2006-07-03 17:39:48.000000000 +0200
+++ b/lib/statistic.c	2006-07-03 14:49:51.000000000 +0200
@@ -1454,7 +1454,6 @@ static struct statistic_discipline stati
  */
 int statistic_create(struct statistic_interface *interface, const char *name)
 {
-#ifdef CONFIG_STATISTICS
 	struct statistic *stat = interface->stat;
 	struct statistic_info *info = interface->info;
 	int i;
@@ -1491,7 +1490,6 @@ int statistic_create(struct statistic_in
 	mutex_lock(&statistic_list_mutex);
 	list_add(&interface->list, &statistic_list);
 	mutex_unlock(&statistic_list_mutex);
-#endif
 	return 0;
 }
 EXPORT_SYMBOL_GPL(statistic_create);
@@ -1510,7 +1508,6 @@ EXPORT_SYMBOL_GPL(statistic_create);
  */
 int statistic_remove(struct statistic_interface *interface)
 {
-#ifdef CONFIG_STATISTICS
 	struct statistic *stat = interface->stat;
 	struct statistic_info *info = interface->info;
 	int i;
@@ -1526,7 +1523,6 @@ int statistic_remove(struct statistic_in
 	debugfs_remove(interface->def_file);
 	debugfs_remove(interface->debugfs_dir);
 	interface->debugfs_dir = NULL;
-#endif
 	return 0;
 }
 EXPORT_SYMBOL_GPL(statistic_remove);
@@ -1548,10 +1544,8 @@ EXPORT_SYMBOL_GPL(statistic_remove);
  */
 void _statistic_add(struct statistic *stat, int i, s64 value, u64 incr)
 {
-#ifdef CONFIG_STATISTICS
 	if (stat[i].state == STATISTIC_STATE_ON)
 		stat[i].add(&stat[i], value, incr);
-#endif
 }
 EXPORT_SYMBOL_GPL(_statistic_add);
 
@@ -1572,12 +1566,10 @@ EXPORT_SYMBOL_GPL(_statistic_add);
  */
 void statistic_add(struct statistic *stat, int i, s64 value, u64 incr)
 {
-#ifdef CONFIG_STATISTICS
 	unsigned long flags;
 	local_irq_save(flags);
 	_statistic_add(stat, i, value, incr);
 	local_irq_restore(flags);
-#endif
 }
 EXPORT_SYMBOL_GPL(statistic_add);
 
@@ -1601,11 +1593,9 @@ EXPORT_SYMBOL_GPL(statistic_add);
  */
 void statistic_set(struct statistic *stat, int i, s64 value, u64 total)
 {
-#ifdef CONFIG_STATISTICS
 	struct statistic_discipline *disc = &statistic_discs[stat[i].type];
 	if (stat[i].state == STATISTIC_STATE_ON)
 		disc->set(&stat[i], value, total);
-#endif
 }
 EXPORT_SYMBOL_GPL(statistic_set);
 

Re: [Patch] statistics infrastructure - update 9

[Cedric Le Goater]

Martin Peschke wrote:
> I broke !CONFIG_STATISTICS. This is the fix.

thanks !

C.

Re: [Patch] statistics infrastructure - update 9

[Andi Kleen]

Martin Peschke <mp3@de.ibm.com> writes:
>  {
> -#ifdef CONFIG_STATISTICS
>  	unsigned long flags;
>  	local_irq_save(flags);
>  	_statistic_add_as(type, stat, i, value, incr);
>  	local_irq_restore(flags);


Is there a particular reason you can't use local_t with cpu_local_*?
It would be faster on many architectures than local_irq_save/restore

-Andi

Re: [Patch] statistics infrastructure - update 9

[Heiko Carstens]

> +#else /* !CONFIG_STATISTICS */
> +/* These NOP functions unburden clients from handling !CONFIG_STATISTICS. */
> +
> +static inline int statistic_create(struct statistic_interface *interface,
> +				   const char *name)
> +{
> +	return 0;
> +}
> +
> +static inline int statistic_remove(struct statistic_interface *interface)
> +{
> +	return 0;
> +}
> +
> +static inline void statistic_set(struct statistic *stat, int i,
> +				 s64 value, u64 total)
> +{
> +}
> +
> +static inline void _statistic_add(struct statistic *stat, int i,
> +				  s64 value, u64 incr)
> +{
> +}
> +
> +static inline void statistic_add(struct statistic *stat, int i,
> +				 s64 value, u64 incr)
> +{
> +}
> +
> +static inline void _statistic_add_as(int type, struct statistic *stat, int i,
> +				     s64 value, u64 incr)
> +{
> +}
> +
> +static inline void statistic_add_as(int type, struct statistic *stat, int i,
> +				    s64 value, u64 incr)
> +{
> +}
> +
> +#endif /* CONFIG_STATISTICS */

Why not have something like:

#define statistic_create(interface, name) 	({ 0; })
#define statistic_remove(interface)		({ 0; })
#define statistic_set(stat, i, value, total)	do { } while (0)
...

That would be much shorter and easier to read. But maybe it's just me :)

Re: [Patch] statistics infrastructure - update 9

[Andrew Morton]

On Tue, 4 Jul 2006 08:17:54 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> > +
> > +static inline void statistic_add_as(int type, struct statistic *stat, int i,
> > +				    s64 value, u64 incr)
> > +{
> > +}
> > +
> > +#endif /* CONFIG_STATISTICS */
> 
> Why not have something like:
> 
> #define statistic_create(interface, name) 	({ 0; })
> #define statistic_remove(interface)		({ 0; })
> #define statistic_set(stat, i, value, total)	do { } while (0)
> ...
> 
> That would be much shorter and easier to read. But maybe it's just me :)

typechecking.

	struct scsi_device s;
	int foo;

	statistic_create(s, foo_with_a_typo);	/* whee */

Re: [Patch] statistics infrastructure - update 9

[Martin Peschke]

Andi Kleen wrote:
> Martin Peschke <mp3@de.ibm.com> writes:
>>  {
>> -#ifdef CONFIG_STATISTICS
>>  	unsigned long flags;
>>  	local_irq_save(flags);
>>  	_statistic_add_as(type, stat, i, value, incr);
>>  	local_irq_restore(flags);
> 
> 
> Is there a particular reason you can't use local_t with cpu_local_*?
> It would be faster on many architectures than local_irq_save/restore
> 
> -Andi

Good question. Btw. - faster by what order of magnitude?
local_irq_save/restore seems to be fine for kernel/profile.c


Reason 1:
cpu_local_* uses __get_cpu_var, which conflicts with struct statistic
being embedded into struct xyz that is allocated whenever the client
needs it.

I could try to use local_t in conjunction with local_add etc.
(as seen in include/linux/dmaengine.h in 2.6.17-mm6).
Does this also yield a performance gain worth consideration?


Reason 2:
Then, the other use of local_irq_save/restore is to avoid races
regarding statistics being switched on or off, and it's underlying
buffers being released:

void _statistic_add(struct statistic *stat, int i, s64 value, u64 incr)
{
         if (stat[i].state == STATISTIC_STATE_ON)
                 stat[i].add(&stat[i], value, incr);
}

void statistic_add(struct statistic *stat, int i, s64 value, u64 incr)
{
         unsigned long flags;
         local_irq_save(flags);
         _statistic_add(stat, i, value, incr);
         local_irq_restore(flags);
}

static int statistic_stop(struct statistic *stat)
{
         stat->stopped = timestamp_clock();
         stat->state = STATISTIC_STATE_OFF;
         /* ensures that all CPUs have ceased updating statistics */
         smp_mb();
         on_each_cpu(_statistic_barrier, NULL, 0, 1);
         return 0;
}

So, removing local_irq_save/restore would require statistics to be
switched on and their buffers being available all the time. That is,
buffers holding counters etc. can't be allocated at run time - what
if allocation fails? (Should I leave this issue to clients?).

All the buffers for per-cpu counters etc. would need to be embedded
into the client's struct xyz. There would be no way that users
could change, for example, the number of buckets of a histogram.

Everything would be fixed. Which might be fine for some purposes;
particularly for a plain counter which will only be used as a
counter, and which will never be inflated to a histogram or whatever.

In short, I could try to add a per-cpu array of local_t's to struct
statistic and write up another statistic_add()-variant, which would be
limited to aggregating data into a counter using local_add(), without
doing local_irq_save/restore, and without checking whether data
gathering has been turned on. Which would resemble Christoph
Lameter's light-weight VM counters to some degree. With the downside
of struct statistic being inflated for everyone else.


Reason 3:
local_add() & friends won't suffice for some algorithms:

void statistic_add_util(struct statistic *stat, s64 value, u64 incr)
{
	/*...snip...*/
         if (unlikely(value < util->min))
                 util->min = value;
         if (unlikely(value > util->max))
                 util->max = value;
}

static void _statistic_add_sparse(struct statistic_sparse_list *slist,
                                   s64 value, u64 incr)
{
         struct list_head *head = &slist->entry_lh;
         struct statistic_entry_sparse *entry;

         list_for_each_entry(entry, head, list) {
                 if (likely(entry->value == value)) {
                         entry->hits += incr;
                         statistic_add_sparse_sort(head, entry);
                         return;
                 }
         }
         if (unlikely(statistic_add_sparse_new(slist, value, incr)))
                 slist->hits_missed += incr;
}


Reason 4:
The alleged overhead of local_irq_save/restore (as compared
to atomic operations) might be less significant for clients updating
a bunch of statistics in one go:

	unsigned long flags;

	local_irq_save(flags);
	_statistic_inc(dev->stat, MYSTAT_SIZE, size);
	_statistic_inc(dev->stat, MYSTAT_LATENCY, latency);
	_statistic_inc(dev->stat, MYSTAT_RESULT, result);
	local_irq_restore(flags);

Re: [Patch] statistics infrastructure - update 9

[Andi Kleen]

> Good question. Btw. - faster by what order of magnitude?

pushf + popf is on K8 at least ~18 cycles, on P4 it is much more
because they synchronize the pipeline there (hundreds of cycles)

cpu local add would be a few cycles at best and doesn't have
any impact on the pipeline


> local_irq_save/restore seems to be fine for kernel/profile.c
> 
> 
> Reason 1:
> cpu_local_* uses __get_cpu_var, which conflicts with struct statistic
> being embedded into struct xyz that is allocated whenever the client
> needs it.
> 
> I could try to use local_t in conjunction with local_add etc.
> (as seen in include/linux/dmaengine.h in 2.6.17-mm6).
> Does this also yield a performance gain worth consideration?

Yes, but you would need preempt_disable() then. For non preemptible
kernels (far majority) that would be already a big win.


> So, removing local_irq_save/restore would require statistics to be
> switched on and their buffers being available all the time. That is,
> buffers holding counters etc. can't be allocated at run time - what
> if allocation fails? (Should I leave this issue to clients?).

Can't you use RCU for this?


> Reason 4:
> The alleged overhead of local_irq_save/restore (as compared
> to atomic operations) 

local_* doesn't need to be atomic. IT isn't on x86 at least.
On some other architectures it can be, but i think it's just a SMOP
of fixing them.

-Andi

Re: [Patch] statistics infrastructure - update 9

[Martin Peschke]

Andi Kleen wrote:
>> Good question. Btw. - faster by what order of magnitude?
> 
> pushf + popf is on K8 at least ~18 cycles, on P4 it is much more
> because they synchronize the pipeline there (hundreds of cycles)
> 
> cpu local add would be a few cycles at best and doesn't have
> any impact on the pipeline
> 
> 
>> local_irq_save/restore seems to be fine for kernel/profile.c
>>
>>
>> Reason 1:
>> cpu_local_* uses __get_cpu_var, which conflicts with struct statistic
>> being embedded into struct xyz that is allocated whenever the client
>> needs it.
>>
>> I could try to use local_t in conjunction with local_add etc.
>> (as seen in include/linux/dmaengine.h in 2.6.17-mm6).
>> Does this also yield a performance gain worth consideration?
> 
> Yes, but you would need preempt_disable() then. For non preemptible
> kernels (far majority) that would be already a big win.
> 
> 
>> So, removing local_irq_save/restore would require statistics to be
>> switched on and their buffers being available all the time. That is,
>> buffers holding counters etc. can't be allocated at run time - what
>> if allocation fails? (Should I leave this issue to clients?).
> 
> Can't you use RCU for this?
> 
> 
>> Reason 4:
>> The alleged overhead of local_irq_save/restore (as compared
>> to atomic operations) 
> 
> local_* doesn't need to be atomic. IT isn't on x86 at least.
> On some other architectures it can be, but i think it's just a SMOP
> of fixing them.
> 
> -Andi

Thanks. I am seriously considering these techniques.

If I manage to use RCU for most of the read-mostly struct statistic
and to push any other locking issues down into individual statistic
disciplines (utilisation indicator, histogram and so on), I should be
able to use local_t for some disciplines (particularyl counter and
histogram) without needing other locking primitives, like
local_irq_save/restore currently currently found in the code.

Not a change that can be done this afternoon, though.
And it will require careful review.

Martin