Messages in this thread | | | Date | Mon, 20 Sep 2010 10:16:50 -0700 | Subject | Re: [PATCH 2/6] Add IRQ_TIME_ACCOUNTING, finer accounting of CPU irq time | From | Venkatesh Pallipadi <> |
| |
On Mon, Sep 20, 2010 at 2:27 AM, Peter Zijlstra <peterz@infradead.org> wrote: > 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? >
Yes. I inherited the API from VIRT_CPU_ACCOUNTING along with this local_bh_disable bug. Agree that we need one extra bit to handle this case. I will take a stab at fixing this along with refresh of this patchset if no one else has beaten me to it until then.
Thanks, Venki
| |