Re: [Patch] statistics infrastructure - update 9

[Martin Peschke]

Andi Kleen wrote:
> Martin Peschke <mp3@xxxxxxxxxx> 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);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/