Date: Mon, 12 Dec 2016 16:02:30 +0100
From: Frederic Weisbecker <fweisbec@gmail.com>
To: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Tony Luck <tony.luck@intel.com>,
        Wanpeng Li <wanpeng.li@hotmail.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Heiko Carstens <heiko.carstens@de.ibm.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Paul Mackerras <paulus@samba.org>, Ingo Molnar <mingo@kernel.org>,
        Fenghua Yu <fenghua.yu@intel.com>, Rik van Riel <riel@redhat.com>,
        Stanislaw Gruszka <sgruszka@redhat.com>
Subject: Re: [PATCH 09/10] s390/cputime: delayed accounting of system time
Message-ID: <20161212150228.GA17032@lerouge>
References: <1480991543-6557-1-git-send-email-fweisbec@gmail.com>
 <1480991543-6557-10-git-send-email-fweisbec@gmail.com>
 <20161210014804.GA3023@lerouge>
 <20161212112754.5ad104cf@mschwideX1>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161212112754.5ad104cf@mschwideX1>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2096
Lines: 46

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.