Messages in this thread | | | Date | Mon, 12 Dec 2016 16:02:30 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 09/10] s390/cputime: delayed accounting of system time |
| |
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.
| |