Re: [patch V2 2/9] irqtime: Make accounting correct on RT

[Frederic Weisbecker]

On Fri, Dec 04, 2020 at 06:01:53PM +0100, Thomas Gleixner wrote:
> vtime_account_irq and irqtime_account_irq() base checks on preempt_count()
> which fails on RT because preempt_count() does not contain the softirq
> accounting which is seperate on RT.
> 
> These checks do not need the full preempt count as they only operate on the
> hard and softirq sections.
> 
> Use irq_count() instead which provides the correct value on both RT and non
> RT kernels. The compiler is clever enough to fold the masking for !RT:
> 
>        99b:	65 8b 05 00 00 00 00 	mov    %gs:0x0(%rip),%eax
>  -     9a2:	25 ff ff ff 7f       	and    $0x7fffffff,%eax
>  +     9a2:	25 00 ff ff 00       	and    $0xffff00,%eax
> 
> Reported-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
> V2: New patch
> ---
>  kernel/sched/cputime.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -60,7 +60,7 @@ void irqtime_account_irq(struct task_str
>  	cpu = smp_processor_id();
>  	delta = sched_clock_cpu(cpu) - irqtime->irq_start_time;
>  	irqtime->irq_start_time += delta;
> -	pc = preempt_count() - offset;
> +	pc = irq_count() - offset;

There are many preempt_count() users all around waiting for similar issues.
Wouldn't it be more reasonable to have current->softirq_disable_cnt just saving
the softirq count on context switch?

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d2003a7d5ab5..6c899c35d6ba 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3469,6 +3469,10 @@ static inline void prepare_task(struct task_struct *next)
 
 static inline void finish_task(struct task_struct *prev)
 {
+#ifdef CONFIG_PREEMPT_RT
+	prev->softirq_disable_cnt = softirq_count();
+	__preempt_count_sub(prev->softirq_disable_cnt);
+#endif
 #ifdef CONFIG_SMP
 	/*
 	 * This must be the very last reference to @prev from this CPU. After
@@ -3610,6 +3614,9 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	vtime_task_switch(prev);
 	perf_event_task_sched_in(prev, current);
 	finish_task(prev);
+#ifdef CONFIG_PREEMPT_RT	
+	__preempt_count_add(current->softirq_disable_cnt);
+#endif
 	finish_lock_switch(rq);
 	finish_arch_post_lock_switch();
 	kcov_finish_switch(current);


And I expect a few adjustments in schedule_debug() and atomic checks more
generally but that should be it and it would probably be less error-prone.
Although I'm probably overlooking other issues on the way.

Thanks.

>  
>  	/*
>  	 * We do not account for softirq time from ksoftirqd here.
> @@ -421,7 +421,7 @@ void vtime_task_switch(struct task_struc
>  
>  void vtime_account_irq(struct task_struct *tsk, unsigned int offset)
>  {
> -	unsigned int pc = preempt_count() - offset;
> +	unsigned int pc = irq_count() - offset;
>  
>  	if (pc & HARDIRQ_OFFSET) {
>  		vtime_account_hardirq(tsk);
>