Re: [PATCH 2/6] Add IRQ_TIME_ACCOUNTING, finer accounting of CPU irq time

[Peter Zijlstra <peterz@infradead.org>]

On Mon, 2010-09-20 at 10:13 -0700, Venkatesh Pallipadi wrote:

> >> +       local_irq_save(flags);
> >> +
> >> +       cpu = task_cpu(tsk);
> >
> > Can this ever be anything other can smp_processor_id() and current?
> >
> >> +       now = sched_clock();
> >
> > this should be using one of the kernel/sched_clock.c thingies, probably
> > local_clock(), or sched_clock_cpu(cpu).
> 
> I don't really need task there. I can use smp_processor_id() or
> task_cpu(tsk) and I think latter one would be cheaper.

Not sure, task_cpu() gets the cpu number from the task_info struct,
smp_processor_id() gets it from per-cpu storage, both are a single
memory read.

But I think its a tad confusing that this function has a task argument
at all, but if its always current, it would be slightly better to call
it 'curr' or something.

Also, local_irq_safe() followed by smp_processor_id() is clearly local,
task_cpu(tsk) can be anything.

> You mean sched_clock() is not the right interface to use here.
> sched_clock_cpu() uses either sched_clock or remote_cpu stuff which I
> don't really need here and local_clock() also has irq
> disable/smp_processor_id() and calls sched_clock_cpu in turn.
> sched_clock() seemed to be more appropriate.

yeah, use sched_clock_cpu(smp_processor_id()), sched_clock() can be
utter crap on x86, the code in kernel/sched_clock.c tries its best to
sanitize the crap we get from the hardware.