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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 12 Dec 2016 16:02:30 +0100
From:   Frederic Weisbecker <fweisbec@...il.com>
To:     Martin Schwidefsky <schwidefsky@...ibm.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Tony Luck <tony.luck@...el.com>,
        Wanpeng Li <wanpeng.li@...mail.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Paul Mackerras <paulus@...ba.org>,
        Ingo Molnar <mingo@...nel.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Rik van Riel <riel@...hat.com>,
        Stanislaw Gruszka <sgruszka@...hat.com>
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@...il.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.