Messages in this thread | | | Date | Mon, 28 Dec 2020 02:15:29 +0000 | From | Qais Yousef <> | Subject | Re: [PATCH 4/5] irqtime: Move irqtime entry accounting after irq offset incrementation |
| |
Hi Frederic
On 12/02/20 12:57, Frederic Weisbecker wrote: > #endif /* _LINUX_KERNEL_VTIME_H */ > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > index 02163d4260d7..5f611658eeab 100644 > --- a/kernel/sched/cputime.c > +++ b/kernel/sched/cputime.c > @@ -44,12 +44,13 @@ static void irqtime_account_delta(struct irqtime *irqtime, u64 delta, > } > > /* > - * Called before incrementing preempt_count on {soft,}irq_enter > + * Called after incrementing preempt_count on {soft,}irq_enter > * and before decrementing preempt_count on {soft,}irq_exit. > */ > -void irqtime_account_irq(struct task_struct *curr) > +void irqtime_account_irq(struct task_struct *curr, unsigned int offset) > { > struct irqtime *irqtime = this_cpu_ptr(&cpu_irqtime); > + unsigned int pc; > s64 delta; > int cpu; > > @@ -59,6 +60,7 @@ void irqtime_account_irq(struct task_struct *curr) > cpu = smp_processor_id(); > delta = sched_clock_cpu(cpu) - irqtime->irq_start_time; > irqtime->irq_start_time += delta; > + pc = preempt_count() - offset; > > /* > * We do not account for softirq time from ksoftirqd here. > @@ -66,9 +68,9 @@ void irqtime_account_irq(struct task_struct *curr) > * 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 (hardirq_count()) > + if (pc & HARDIRQ_MASK) > irqtime_account_delta(irqtime, delta, CPUTIME_IRQ); > - else if (in_serving_softirq() && curr != this_cpu_ksoftirqd()) > + else if ((pc & SOFTIRQ_OFFSET) && curr != this_cpu_ksoftirqd())
Noob question. Why for SOFTIRQs we do sofirq_count() & *SOFTIRQ_OFFSET*? It seems we're in-softirq only if the count is odd numbered.
/me tries to dig more
Hmm could it be because the softirq count is actually 1 bit and the rest is for SOFTIRQ_DISABLE_OFFSET (BH disabled)?
IOW, 1 bit is for we're in softirq context, and the remaining 7 bits are to count BH disable nesting, right?
I guess this would make sense; we don't nest softirqs processing AFAIK. But I could be misreading the code too :-)
> irqtime_account_delta(irqtime, delta, CPUTIME_SOFTIRQ); > } > > @@ -417,11 +419,13 @@ void vtime_task_switch(struct task_struct *prev) > } > # endif > > -void vtime_account_irq(struct task_struct *tsk) > +void vtime_account_irq(struct task_struct *tsk, unsigned int offset) > { > - if (hardirq_count()) { > + unsigned int pc = preempt_count() - offset; > + > + if (pc & HARDIRQ_OFFSET) {
Shouldn't this be HARDIRQ_MASK like above?
> vtime_account_hardirq(tsk); > - } else if (in_serving_softirq()) { > + } else if (pc & SOFTIRQ_OFFSET) { > vtime_account_softirq(tsk); > } else if (!IS_ENABLED(CONFIG_HAVE_VIRT_CPU_ACCOUNTING_IDLE) && > is_idle_task(tsk)) {
Thanks
-- Qais Yousef
| |