Messages in this thread | | | Subject | Re: [PATCH 2/6] Add IRQ_TIME_ACCOUNTING, finer accounting of CPU irq time | From | Peter Zijlstra <> | Date | Mon, 20 Sep 2010 11:27:30 +0200 |
| |
On Mon, 2010-09-20 at 09:27 +0200, Martin Schwidefsky wrote:
> > OK, so by virtue of calling the same function on _enter and _exit its > > not incomplete, just weird. > > That is the same with CONFIG_VIRT_CPU_ACCOUNTING=y. irq_enter/irq_exit > call account_system_vtime, the function then uses the preempt/softirq/ > hardirq counter to find out which context is currently active.
Yeah, I realized that eventually, I've so far been able to mostly ignore all that VIRT_CPU_ACCOUNTING muck.
> > And it won't account time double, since it uses irq_start_time to > > compute deltas between invocations and will attribute that delta to only > > one state. > > irq_start_time is a bit misleading, it is a time stamp of the last update. > The confusing part (which deserves a comment) is the fact that the delta > is not added to anything if hardirq_count and softirq_count are zero.
Yeah, the name didn't help either, but I really expected to see two hooks: start/exit, I did eventually figure it all out, but its a bit daft.
If you would have had 4 hooks, the below problem would have been fixable within the implementation.
> > You still do have the problem with local_bh_disable() though, since you > > cannot distinguish between having bh disabled and processing softirq. > > > > So a hardirq that hits while you have bh disabled will inflate your > > softirq time. > > > > A possible solution is to have local_bh_{disable,enable} {add,sub} > > 2*SOFTIRQ_OFFSET and have the processing use SOFTIRQ_OFFSET, will need a > > bit of a code shuffle though. > > Hmm, that bug is valid for CONFIG_VIRT_CPU_ACCOUNTING=y as well.
And nobody ever noticed?
| |