Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1539753
| From | Frederic Weisbecker <fweisbec@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 09/10] s390/cputime: delayed accounting of system time |
| Date | 2016-12-10 02:50 +0100 |
| Message-ID | <sMEXv-2e3-3@gated-at.bofh.it> (permalink) |
| References | <sLdPH-4nv-3@gated-at.bofh.it> <sLdPI-4nv-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Tue, Dec 06, 2016 at 03:32:22AM +0100, Frederic Weisbecker wrote:
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
>
> The account_system_time() function is called with a cputime that
> occurred while running in the kernel. The function detects which
> context the CPU is currently running in and accounts the time to
> the correct bucket. This forces the arch code to account the
> cputime for hardirq and softirq immediately.
>
> Such accounting function can be costly and perform unwelcome divisions
> and multiplications, among others.
>
> The arch code can delay the accounting for system time. For s390
> the accounting is done once per timer tick and for each task switch.
>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Wanpeng Li <wanpeng.li@hotmail.com>
> [rebase against latest cputime tree, massaged changelog accordingly]
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Looking at this patch again, I think I need to do another pass on it.
Comments below:
> /*
> * Update process times based on virtual cpu times stored by entry.S
> * to the lowcore fields user_timer, system_timer & steal_clock.
> */
> static int do_account_vtime(struct task_struct *tsk, int hardirq_offset)
> {
> - u64 timer, clock, user, system, steal;
> - u64 user_scaled, system_scaled;
> + u64 timer, clock, user, guest, system, hardirq, softirq, steal;
>
> timer = S390_lowcore.last_update_timer;
> clock = S390_lowcore.last_update_clock;
> @@ -110,36 +119,57 @@ static int do_account_vtime(struct task_struct *tsk, int hardirq_offset)
> #endif
> : "=m" (S390_lowcore.last_update_timer),
> "=m" (S390_lowcore.last_update_clock));
> - S390_lowcore.system_timer += timer - S390_lowcore.last_update_timer;
> - S390_lowcore.steal_timer += S390_lowcore.last_update_clock - clock;
> + clock = S390_lowcore.last_update_clock - clock;
> + timer -= S390_lowcore.last_update_timer;
> +
> + if ((tsk->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
> + S390_lowcore.guest_timer += timer;
> + else if (hardirq_count() - hardirq_offset)
> + S390_lowcore.hardirq_timer += timer;
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.
> + else if (in_serving_softirq())
> + S390_lowcore.softirq_timer += timer;
> + else
> + S390_lowcore.system_timer += timer;
>
> /* Update MT utilization calculation */
> if (smp_cpu_mtid &&
> time_after64(jiffies_64, this_cpu_read(mt_scaling_jiffies)))
> update_mt_scaling();
>
> + /* Calculate cputime delta */
> user = S390_lowcore.user_timer - tsk->thread.user_timer;
> - S390_lowcore.steal_timer -= user;
> tsk->thread.user_timer = S390_lowcore.user_timer;
> -
> + guest = S390_lowcore.guest_timer - tsk->thread.guest_timer;
> + tsk->thread.guest_timer = S390_lowcore.guest_timer;
> system = S390_lowcore.system_timer - tsk->thread.system_timer;
> - S390_lowcore.steal_timer -= system;
> tsk->thread.system_timer = S390_lowcore.system_timer;
> + hardirq = S390_lowcore.hardirq_timer - tsk->thread.hardirq_timer;
> + tsk->thread.hardirq_timer = S390_lowcore.hardirq_timer;
> + softirq = S390_lowcore.softirq_timer - tsk->thread.softirq_timer;
> + tsk->thread.softirq_timer = S390_lowcore.softirq_timer;
> + S390_lowcore.steal_timer +=
> + clock - user - guest - system - hardirq - softirq;
>
> - user_scaled = user;
> - system_scaled = system;
> - /* Do MT utilization scaling */
> - if (smp_cpu_mtid) {
> - u64 mult = __this_cpu_read(mt_scaling_mult);
> - u64 div = __this_cpu_read(mt_scaling_div);
> + /* Push account value */
> + if (user) {
> + account_user_time(tsk, user);
> + tsk->utimescaled += scale_vtime(user);
> + }
>
> - user_scaled = (user_scaled * mult) / div;
> - system_scaled = (system_scaled * mult) / div;
> + if (guest) {
> + account_guest_time(tsk, guest);
> + tsk->utimescaled += scale_vtime(guest);
> }
> - account_user_time(tsk, user);
> - tsk->utimescaled += user_scaled;
> - account_system_time(tsk, hardirq_offset, system);
> - tsk->stimescaled += system_scaled;
> +
> + if (system)
> + account_system_index_scaled(tsk, system, scale_vtime(system),
> + CPUTIME_SYSTEM);
> + if (hardirq)
> + account_system_index_scaled(tsk, hardirq, scale_vtime(hardirq),
> + CPUTIME_IRQ);
> + if (softirq)
> + account_system_index_scaled(tsk, softirq, scale_vtime(softirq),
> + CPUTIME_SOFTIRQ);
>
> steal = S390_lowcore.steal_timer;
> if ((s64) steal > 0) {
> @@ -147,16 +177,22 @@ static int do_account_vtime(struct task_struct *tsk, int hardirq_offset)
> account_steal_time(steal);
> }
>
> - return virt_timer_forward(user + system);
> + return virt_timer_forward(user + guest + system + hardirq + softirq);
> }
>
> 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().
> prev->thread.user_timer = S390_lowcore.user_timer;
> + prev->thread.guest_timer = S390_lowcore.guest_timer;
> prev->thread.system_timer = S390_lowcore.system_timer;
> + prev->thread.hardirq_timer = S390_lowcore.hardirq_timer;
> + prev->thread.softirq_timer = S390_lowcore.softirq_timer;
> S390_lowcore.user_timer = current->thread.user_timer;
> + S390_lowcore.guest_timer = current->thread.guest_timer;
> S390_lowcore.system_timer = current->thread.system_timer;
> + S390_lowcore.hardirq_timer = current->thread.hardirq_timer;
> + S390_lowcore.softirq_timer = current->thread.softirq_timer;
> }
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 00/10] vtime: Delay cputime accounting to tick Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[FIX][PATCH 02/10] ia64: Fix wrong start cputime assignment on task switch Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[PATCH 09/10] s390/cputime: delayed accounting of system time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-10 02:50 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-12-12 11:30 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-12 16:10 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-12-13 12:20 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-12-13 14:30 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-14 03:40 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-12-20 15:20 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-20 15:40 +0100
Re: [PATCH 09/10] s390/cputime: delayed accounting of system time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-13 15:40 +0100
[PATCH 06/10] powerpc: Migrate stolen_time field to accounting structure Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[PATCH 03/10] cputime: Allow accounting system time using cpustat index Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[PATCH 10/10] vtime: Rename vtime_account_user() to vtime_flush() Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[PATCH 07/10] powerpc/vtime: Accumulate cputime and account only on tick/task switch Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[PATCH 08/10] ia64: Accumulate cputime and account only on tick/task switch Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
[PATCH 04/10] cputime: Export account_guest_time Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 03:40 +0100
Re: [PATCH 00/10] vtime: Delay cputime accounting to tick Paul Mackerras <paulus@ozlabs.org> - 2016-12-06 05:30 +0100
Re: [PATCH 00/10] vtime: Delay cputime accounting to tick Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-12-06 08:10 +0100
Re: [PATCH 00/10] vtime: Delay cputime accounting to tick Frederic Weisbecker <fweisbec@gmail.com> - 2016-12-06 15:40 +0100
Re: [PATCH 00/10] vtime: Delay cputime accounting to tick Christian Borntraeger <borntraeger@de.ibm.com> - 2016-12-06 09:50 +0100
csiph-web