Re: [PATCH 09/10] s390/cputime: delayed accounting of system time

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Dec 12, 2016 at 11:27:54AM +0100, Martin Schwidefsky wrote:
> On Sat, 10 Dec 2016 02:48:06 +0100
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > We should get rid of the hardirq_offset argument, it doesn't really make sense
> > anymore. Also it makes the accounting buggy now. It's called from the tick
> > through account_user_time() with hardirq_offset=1, so the irq time is incorrectly
> > accumulated as system time. Guest time may be incorrect too.
> > 
> > In fact it may have been buggy even before this patchset because vtime_account_user()
> > isn't only called from the tick but also from task switch, and hardirq_offset remains 1
> > for those two cases. Not good.
> 
> For s390 the do_account_vtime function is called from vtime_task_switch and vtime_flush.
> 1) vtime_task_switch is exclusively called from finish_task_switch outside of irq context.
>    The call to do_account_vtime with hardirq_offset==0 from vtime_task_switch is correct.

Yes that one is fine.

> 2) The call to vtime_flush in vtime_common_task_switch is irrelevant for s390 as we
>    define __ARCH_HAS_VTIME_TASK_SWITCH

That's right, I missed that. And now I remember that special case remains because s390 has its
own way to account idle time.

> 3) The call to vtime_flush in account_process_tick is done in irq context from
>    update_process_times. hardirq_offset==1 is also correct.

Let's see this for example:

+       if ((tsk->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
+               S390_lowcore.guest_timer += timer;

If the tick is interrupting guest, we have accounted the guest time on tick IRQ entry.
Now we are in the middle of the tick interrupt and since hardirq_offset is 1, we
are taking the above path by accounting half of the tick-IRQ time as guest, which is wrong,
it's actually IRQ time.

> > >  void vtime_task_switch(struct task_struct *prev)
> > >  {
> > >  	do_account_vtime(prev, 0);  
> > 
> > This call should be removed, the task switch already calls vtime_account_user().
> 
> The vtime_account_user function is empty for s390..

That's right.