Re: [PATCH 4/5] irqtime: Move irqtime entry accounting after irq offset incrementation

[Qais Yousef]

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