Messages in this thread | | | Date | Tue, 1 Dec 2020 15:35:45 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 4/5] irqtime: Move irqtime entry accounting after irq offset incrementation |
| |
On Tue, Dec 01, 2020 at 02:34:49PM +0100, Thomas Gleixner wrote: > On Tue, Dec 01 2020 at 12:40, Frederic Weisbecker wrote: > > On Tue, Dec 01, 2020 at 12:33:26PM +0100, Thomas Gleixner wrote: > >> > /* > >> > * We do not account for softirq time from ksoftirqd here. > >> > * We want to continue accounting softirq time to ksoftirqd thread > >> > * in that case, so as not to confuse scheduler with a special task > >> > * that do not consume any time, but still wants to run. > >> > */ > >> > if (pc & HARDIRQ_MASK) > >> > irqtime_account_delta(irqtime, delta, CPUTIME_IRQ); > >> > else if ((pc & SOFTIRQ_OFFSET) && curr != this_cpu_ksoftirqd()) > >> > irqtime_account_delta(irqtime, delta, CPUTIME_SOFTIRQ); > >> > } > >> > >> Why not making all of this explicit instead of these conditionals? > > > > Hmm, I'm not sure I get what you suggest? > > Instead of playing games with preeempt count and offsets and checking > for ksoftirqd, can't you just have: > > account_hardirqtime() > account_softirqtime() > > and call them from the right spots. See the below for illustration (it's > obviously incomplete). > > Thanks, > > tglx > --- > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -377,6 +377,7 @@ static inline void invoke_softirq(void) > return; > > if (!force_irqthreads) { > + account_softirq_enter_time(current); > #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK > /* > * We can safely execute softirq on the current stack if > @@ -391,6 +392,7 @@ static inline void invoke_softirq(void) > * to prevent from any overrun. > */ > do_softirq_own_stack(); > + account_softirq_exit_time(current);
Indeed for the softirq part it simplifies things.
> #endif > } else { > wakeup_softirqd(); > @@ -417,7 +419,7 @@ static inline void __irq_exit_rcu(void) > #else > lockdep_assert_irqs_disabled(); > #endif > - account_irq_exit_time(current); > + account_hardirq_exit_time(current);
And that one too makes things simple. But note that
account_hardirq_enter_time()
will still need some preempt count checks to see if this is a nested hardirq, a hardirq interrupting a softirq or a hardirq interrupting a task.
But I think it's a win in the end. I'll try that.
Thanks.
> preempt_count_sub(HARDIRQ_OFFSET); > if (!in_interrupt() && local_softirq_pending()) > invoke_softirq();
| |