Re: [PATCH 1/7] si time accounting accounts bh_disable'd time to si -v3

[Peter Zijlstra <peterz@infradead.org>]

On Thu, 2010-09-30 at 09:26 -0700, Venkatesh Pallipadi wrote:
> > One nit: in_serving_softirq() doesn't seem right as either:
> >
> >  - we're not accounting ksoftirq in it, or
> >  - we're are and VIRT_CPU_ACCOUNTING is again broken ;-)
> >
> > So only the softirq from irq tails wants to have SOFTIRQ_OFFSET set, the
> > ksoftirqd stuff can be tested for using PF_flags or something (ksoftirq
> > doesn't currently have a PF_SOFTIRQ flag, but -rt does and we could
> > bring that over).
> >
> 
> The problem is that ksoftirqd is also handling softirq's and some
> eventual users of in_serving_softirq (like the network code in this
> patch) want to differentiate between whether it is the softirq thats
> running or some real process (!ksoftirqd) context.

The make in_serving_softirq() be something like:
  (preempt_count() & SOFTIRQ_OFFSET) || (current->flags & PF_SOFTIRQ)


> Also, ksoftirqd adds to softirq counts, does trace softirq, etc. So,
> it kind of made sense to add the time also to softirq stats as well.
> If we dont account time to softirq stats, then if some user is looking
> at say time per softirq using the softirq count will be misled. No?

Simply add back the task accounting when you report it?

> In the other thread you mentioned doing that will cause problems. Were
> you thinking of scheduler issues or are there other problems charging
> softirq time this way?

Of course there are.. you're double accounting the time of ksoftirqd,
and worse, you're adding that back into the equation as part of the !
sched_fair time.