Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1524626 > unrolled thread

[PATCH 00/36] cputime: Convert core use of cputime_t to nsecs

Started byFrederic Weisbecker <fweisbec@gmail.com>
First post2016-11-17 19:10 +0100
Last post2016-11-21 17:30 +0100
Articles 6 on this page of 46 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 17/36] cputime: Increment kcpustat directly on irqtime account Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 25/36] cputime: Push time to account_system_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 02/36] time: Introduce jiffies64_to_nsecs() Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 07/36] cputime: Special API to return old-typed cputime Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 19/36] posix-timers: Convert internals to use nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 22/36] cputime: Push time to account_user_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:10 +0100
    [PATCH 23/36] cputime: Push time to account_steal_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 29/36] ia64: Move nsecs based cputime headers to the last arch using it Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 04/36] cputime: Convert kcpustat to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 01/36] jiffies: Reuse TICK_NSEC instead of NSEC_PER_JIFFY Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 34/36] s390: Remove unused cputime definitions Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 30/36] ia64: Convert vtime to use nsec units directly Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 33/36] powerpc: Remove unused cputime definitions Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 14/36] delaycct: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 09/36] alpha: Convert obsolete cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 31/36] ia64: Remove unused cputime definitions Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 27/36] vtime: Return nsecs instead of cputime_t to account Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 24/36] cputime: Push time to account_idle_time() in nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 36/36] cputime: Remove asm generic headers Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 05/36] macintosh/rack-meter: Remove cputime_t internal use Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 13/36] acct: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 28/36] cputime: Remove jiffies based cputime Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 03/36] sched: Remove unused INIT_CPUTIME macro Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 26/36] cputime: Complete nsec conversion of tick based accounting Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 20/36] itimer: Convert internal cputime_t units to nsec Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 06/36] cputime: Convert guest time accounting to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 10/36] x86: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 35/36] cputime: Remove unused nsec_to_cputime Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 12/36] binfmt: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 18/36] posix-timers: Use TICK_NSEC instead of a dynamically ad-hoc calculated version Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 21/36] sched: Remove temporary cputime_t accessors Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 32/36] s390: Make arch_cpu_idle_time() to return nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 15/36] tsacct: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 16/36] signal: Convert obsolete cputime type to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    [PATCH 08/36] cputime: Convert task/group cputime to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-17 19:20 +0100
    Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-11-18 13:10 +0100
      Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-18 16:10 +0100
        Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-11-21 08:10 +0100
          Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-11-21 11:20 +0100
            Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-22 14:50 +0100
              Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-11-22 15:30 +0100
          Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-21 17:30 +0100
            Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Martin Schwidefsky <schwidefsky@de.ibm.com> - 2016-11-22 07:20 +0100
        Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Ingo Molnar <mingo@kernel.org> - 2016-11-21 10:50 +0100
          Re: [PATCH 00/36] cputime: Convert core use of cputime_t to nsecs Frederic Weisbecker <fweisbec@gmail.com> - 2016-11-21 17:30 +0100

Page 3 of 3 — ← Prev page 1 2 [3]

#1527516

FromFrederic Weisbecker <fweisbec@gmail.com>
Date2016-11-22 14:50 +0100
Message-ID<sGjCp-7LM-27@gated-at.bofh.it>
In reply to#1526520
On Mon, Nov 21, 2016 at 11:17:28AM +0100, Martin Schwidefsky wrote:
> On Mon, 21 Nov 2016 07:59:56 +0100
> Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
> 
> > On Fri, 18 Nov 2016 15:47:02 +0100
> > Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > On Fri, Nov 18, 2016 at 01:08:46PM +0100, Martin Schwidefsky wrote:
> > > > On Thu, 17 Nov 2016 19:08:07 +0100
> > > > Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > > 
> > > > Now it has been proposed to implement lazy accounting to accumulate deltas
> > > > and do the expensive conversions only infrequently. This is pretty straight-
> > > > forward for account_user_time but to do this for the account_system_time
> > > > function is more complicated. The function has to differentiate between
> > > > guest/hardirq/softirq and pure system time. We would need to keep sums for
> > > > each bucket and provide a separate function to add to each bucket. Like
> > > > account_guest_time(), account_hardirq_time(), account_softirq_time() and
> > > > account_system_time(). Then it is up to the arch code to sort out the details
> > > > and call the accounting code once per jiffy for each of the buckets.
> > > 
> > > That wouldn't be too hard really. The s390 code in vtime.c already does that.
> > 
> > Yes, I agree that the accumulating change would not be too hard. Can I make the
> > request that we try to get that done first before doing the cleanup ?
> 
> Played with the idea a bit, here is a prototype patch to do the delay system time
> accounting for s390. It applies against the latest s390 features tree which you'll
> find here
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
> 
> The details probably needs some more work but it works.
> 
> --
> From 1b5ef9ddf899da81a48de826f783b15e6fc45d25 Mon Sep 17 00:00:00 2001
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date: Mon, 21 Nov 2016 10:44:10 +0100
> Subject: [PATCH] s390/cputime: delayed accounting of system time
> 
> 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.
> 
> Make account_guest_time non-static and add account_sys_time,
> account_hardirq_time and account_softirq_time. With these functions
> 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>

Thanks a lot for taking care of that! I'll give a try to do the same
on powerpc.

A few comments below:

> ---
>  arch/s390/include/asm/lowcore.h   |  65 ++++++++++++-----------
>  arch/s390/include/asm/processor.h |   3 ++
>  arch/s390/kernel/vtime.c          | 106 ++++++++++++++++++++++----------------
>  include/linux/kernel_stat.h       |  13 +++--
>  kernel/sched/cputime.c            |  22 +++++++-
>  5 files changed, 129 insertions(+), 80 deletions(-)
> 
> diff --git a/arch/s390/include/asm/lowcore.h b/arch/s390/include/asm/lowcore.h
> index 62a5cf1..8a5b082 100644
> --- a/arch/s390/include/asm/lowcore.h
> +++ b/arch/s390/include/asm/lowcore.h
[...]
> @@ -110,34 +119,48 @@ 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;
> +	else if (in_serving_softirq())
> +		S390_lowcore.softirq_timer += timer;
> +	else
> +		S390_lowcore.system_timer += timer;

I initially thought that some code could be shared for that whole accumulation. Now I
don't know if it would be a good idea. An example would be to deal with the contexts above
in order to store the accumulation to the appropriate place.

>  
>  	/* 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;
> -
> -	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);
> -
> -		user_scaled = (user_scaled * mult) / div;
> -		system_scaled = (system_scaled * mult) / div;
> -	}
> -	account_user_time(tsk, user, user_scaled);
> -	account_system_time(tsk, hardirq_offset, system, system_scaled);
> +	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;
> +
> +	/* Push account value */
> +	if (user)
> +		account_user_time(tsk, user, scale_vtime(user));
> +	if (guest)
> +		account_guest_time(tsk, guest, scale_vtime(guest));
> +	if (system)
> +		account_sys_time(tsk, system, scale_vtime(system));
> +	if (hardirq)
> +		account_hardirq_time(tsk, hardirq, scale_vtime(hardirq));
> +	if (softirq)
> +		account_softirq_time(tsk, softirq, scale_vtime(softirq));

And doing that would be another part of the shared code.

>  
>  	steal = S390_lowcore.steal_timer;
>  	if ((s64) steal > 0) {
> @@ -145,16 +168,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);
>  	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;
>  }

Ditto.

>  
>  /*
> @@ -174,31 +203,22 @@ void vtime_account_user(struct task_struct *tsk)
>   */
>  void vtime_account_irq_enter(struct task_struct *tsk)
>  {
> -	u64 timer, system, system_scaled;
> +	u64 timer;
>  
>  	timer = S390_lowcore.last_update_timer;
>  	S390_lowcore.last_update_timer = get_vtimer();
> -	S390_lowcore.system_timer += timer - S390_lowcore.last_update_timer;
> -
> -	/* Update MT utilization calculation */
> -	if (smp_cpu_mtid &&
> -	    time_after64(jiffies_64, this_cpu_read(mt_scaling_jiffies)))
> -		update_mt_scaling();
> -
> -	system = S390_lowcore.system_timer - tsk->thread.system_timer;
> -	S390_lowcore.steal_timer -= system;
> -	tsk->thread.system_timer = S390_lowcore.system_timer;
> -	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);
> -
> -		system_scaled = (system_scaled * mult) / div;
> -	}
> -	account_system_time(tsk, 0, system, system_scaled);
> -
> -	virt_timer_forward(system);
> +	timer -= S390_lowcore.last_update_timer;
> +
> +	if ((tsk->flags & PF_VCPU) && (irq_count() == 0))
> +		S390_lowcore.guest_timer += timer;
> +	else if (hardirq_count())
> +		S390_lowcore.hardirq_timer += timer;
> +	else if (in_serving_softirq())
> +		S390_lowcore.softirq_timer += timer;
> +	else
> +		S390_lowcore.system_timer += timer;

And Ditto.

We could put together the accumulation in a common struct in s390_lowcore,
and its mirror in thread struct then have helpers take care of the contexts.

How does that sound to you, would it help or hurt?

Thanks.

[toc] | [prev] | [next] | [standalone]

#1527560

FromMartin Schwidefsky <schwidefsky@de.ibm.com>
Date2016-11-22 15:30 +0100
Message-ID<sGkf8-8lc-49@gated-at.bofh.it>
In reply to#1527516
On Tue, 22 Nov 2016 14:45:56 +0100
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Mon, Nov 21, 2016 at 11:17:28AM +0100, Martin Schwidefsky wrote:
> > On Mon, 21 Nov 2016 07:59:56 +0100
> > Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
> [...]
> > @@ -110,34 +119,48 @@ 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;
> > +	else if (in_serving_softirq())
> > +		S390_lowcore.softirq_timer += timer;
> > +	else
> > +		S390_lowcore.system_timer += timer;
> 
> I initially thought that some code could be shared for that whole accumulation. Now I
> don't know if it would be a good idea. An example would be to deal with the contexts above
> in order to store the accumulation to the appropriate place.

I thought about a common code inline function that returns the index
(CPUTIME_SYSTEM, CPUTIME_IRQ, ..) for the current context. Did not look
too appealing anymore after I type it down.
 
> > -	account_user_time(tsk, user, user_scaled);
> > -	account_system_time(tsk, hardirq_offset, system, system_scaled);
> > +	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;
> > +
> > +	/* Push account value */
> > +	if (user)
> > +		account_user_time(tsk, user, scale_vtime(user));
> > +	if (guest)
> > +		account_guest_time(tsk, guest, scale_vtime(guest));
> > +	if (system)
> > +		account_sys_time(tsk, system, scale_vtime(system));
> > +	if (hardirq)
> > +		account_hardirq_time(tsk, hardirq, scale_vtime(hardirq));
> > +	if (softirq)
> > +		account_softirq_time(tsk, softirq, scale_vtime(softirq));
> 
> And doing that would be another part of the shared code.

Right now I would feel more comfortable if that stays architecture code.
The calculation up to the point where accout_xxx_time function can be
called is definitely arch specific. Why try to do the accumulation in
common code? I have the feeling that would just complicate the code for
no good reason.

> >  
> >  	steal = S390_lowcore.steal_timer;
> >  	if ((s64) steal > 0) {
> > @@ -145,16 +168,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);
> >  	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;
> >  }
> 
> Ditto.

Same here. The lowcore fields are too arch specific.

> >  
> >  /*
> > @@ -174,31 +203,22 @@ void vtime_account_user(struct task_struct *tsk)
> >   */
> >  void vtime_account_irq_enter(struct task_struct *tsk)
> >  {
> > -	u64 timer, system, system_scaled;
> > +	u64 timer;
> >  
> >  	timer = S390_lowcore.last_update_timer;
> >  	S390_lowcore.last_update_timer = get_vtimer();
> > -	S390_lowcore.system_timer += timer - S390_lowcore.last_update_timer;
> > -
> > -	/* Update MT utilization calculation */
> > -	if (smp_cpu_mtid &&
> > -	    time_after64(jiffies_64, this_cpu_read(mt_scaling_jiffies)))
> > -		update_mt_scaling();
> > -
> > -	system = S390_lowcore.system_timer - tsk->thread.system_timer;
> > -	S390_lowcore.steal_timer -= system;
> > -	tsk->thread.system_timer = S390_lowcore.system_timer;
> > -	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);
> > -
> > -		system_scaled = (system_scaled * mult) / div;
> > -	}
> > -	account_system_time(tsk, 0, system, system_scaled);
> > -
> > -	virt_timer_forward(system);
> > +	timer -= S390_lowcore.last_update_timer;
> > +
> > +	if ((tsk->flags & PF_VCPU) && (irq_count() == 0))
> > +		S390_lowcore.guest_timer += timer;
> > +	else if (hardirq_count())
> > +		S390_lowcore.hardirq_timer += timer;
> > +	else if (in_serving_softirq())
> > +		S390_lowcore.softirq_timer += timer;
> > +	else
> > +		S390_lowcore.system_timer += timer;
> 
> And Ditto.

It would be nice if we can find a solution to make the decision
tree where to put the cputime delta into common code.

> We could put together the accumulation in a common struct in s390_lowcore,
> and its mirror in thread struct then have helpers take care of the contexts.
> 
> How does that sound to you, would it help or hurt?

My gut feeling is that the try to make the accumulation code common will
hurt more than it helps. But we can certainly try and look at the result.

I spent some more time on this, here is my current patch. For my part
the patch is close to the final solution if we can agree on it.
--
From a8f5d41df5f32897335567ea9f5a61a716855d5d Mon Sep 17 00:00:00 2001
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date: Mon, 21 Nov 2016 10:44:10 +0100
Subject: [PATCH] s390/cputime: delayed accounting of system time

The account_system_time() function is called with a cputime that
occurred while running in the kernel. The function detects the
current context of the CPU (system, guest, irq, or softirq) and
accounts the time to the correct bucket. This forces the arch code
to account the cputime for hardirq and softirq before entering
and after leaving the context in question.

Make account_guest_time non-static and add account_system_time_native,
With these two functions 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>
---
 arch/s390/include/asm/lowcore.h   |  65 ++++++++++----------
 arch/s390/include/asm/processor.h |   3 +
 arch/s390/kernel/vtime.c          | 126 +++++++++++++++++++++++---------------
 include/linux/kernel_stat.h       |   4 ++
 kernel/sched/cputime.c            |  12 +++-
 5 files changed, 127 insertions(+), 83 deletions(-)

diff --git a/arch/s390/include/asm/lowcore.h b/arch/s390/include/asm/lowcore.h
index 62a5cf1..8a5b082 100644
--- a/arch/s390/include/asm/lowcore.h
+++ b/arch/s390/include/asm/lowcore.h
@@ -85,53 +85,56 @@ struct lowcore {
 	__u64	mcck_enter_timer;		/* 0x02c0 */
 	__u64	exit_timer;			/* 0x02c8 */
 	__u64	user_timer;			/* 0x02d0 */
-	__u64	system_timer;			/* 0x02d8 */
-	__u64	steal_timer;			/* 0x02e0 */
-	__u64	last_update_timer;		/* 0x02e8 */
-	__u64	last_update_clock;		/* 0x02f0 */
-	__u64	int_clock;			/* 0x02f8 */
-	__u64	mcck_clock;			/* 0x0300 */
-	__u64	clock_comparator;		/* 0x0308 */
+	__u64	guest_timer;			/* 0x02d8 */
+	__u64	system_timer;			/* 0x02e0 */
+	__u64	hardirq_timer;			/* 0x02e8 */
+	__u64	softirq_timer;			/* 0x02f0 */
+	__u64	steal_timer;			/* 0x02f8 */
+	__u64	last_update_timer;		/* 0x0300 */
+	__u64	last_update_clock;		/* 0x0308 */
+	__u64	int_clock;			/* 0x0310 */
+	__u64	mcck_clock;			/* 0x0318 */
+	__u64	clock_comparator;		/* 0x0320 */
 
 	/* Current process. */
-	__u64	current_task;			/* 0x0310 */
-	__u8	pad_0x318[0x320-0x318];		/* 0x0318 */
-	__u64	kernel_stack;			/* 0x0320 */
+	__u64	current_task;			/* 0x0328 */
+	__u8	pad_0x318[0x320-0x318];		/* 0x0330 */
+	__u64	kernel_stack;			/* 0x0338 */
 
 	/* Interrupt, panic and restart stack. */
-	__u64	async_stack;			/* 0x0328 */
-	__u64	panic_stack;			/* 0x0330 */
-	__u64	restart_stack;			/* 0x0338 */
+	__u64	async_stack;			/* 0x0340 */
+	__u64	panic_stack;			/* 0x0348 */
+	__u64	restart_stack;			/* 0x0350 */
 
 	/* Restart function and parameter. */
-	__u64	restart_fn;			/* 0x0340 */
-	__u64	restart_data;			/* 0x0348 */
-	__u64	restart_source;			/* 0x0350 */
+	__u64	restart_fn;			/* 0x0358 */
+	__u64	restart_data;			/* 0x0360 */
+	__u64	restart_source;			/* 0x0368 */
 
 	/* Address space pointer. */
-	__u64	kernel_asce;			/* 0x0358 */
-	__u64	user_asce;			/* 0x0360 */
+	__u64	kernel_asce;			/* 0x0370 */
+	__u64	user_asce;			/* 0x0378 */
 
 	/*
 	 * The lpp and current_pid fields form a
 	 * 64-bit value that is set as program
 	 * parameter with the LPP instruction.
 	 */
-	__u32	lpp;				/* 0x0368 */
-	__u32	current_pid;			/* 0x036c */
+	__u32	lpp;				/* 0x0380 */
+	__u32	current_pid;			/* 0x0384 */
 
 	/* SMP info area */
-	__u32	cpu_nr;				/* 0x0370 */
-	__u32	softirq_pending;		/* 0x0374 */
-	__u64	percpu_offset;			/* 0x0378 */
-	__u64	vdso_per_cpu_data;		/* 0x0380 */
-	__u64	machine_flags;			/* 0x0388 */
-	__u32	preempt_count;			/* 0x0390 */
-	__u8	pad_0x0394[0x0398-0x0394];	/* 0x0394 */
-	__u64	gmap;				/* 0x0398 */
-	__u32	spinlock_lockval;		/* 0x03a0 */
-	__u32	fpu_flags;			/* 0x03a4 */
-	__u8	pad_0x03a8[0x0400-0x03a8];	/* 0x03a8 */
+	__u32	cpu_nr;				/* 0x0388 */
+	__u32	softirq_pending;		/* 0x038c */
+	__u64	percpu_offset;			/* 0x0390 */
+	__u64	vdso_per_cpu_data;		/* 0x0398 */
+	__u64	machine_flags;			/* 0x03a0 */
+	__u32	preempt_count;			/* 0x03a8 */
+	__u8	pad_0x03ac[0x03b0-0x03ac];	/* 0x03ac */
+	__u64	gmap;				/* 0x03b0 */
+	__u32	spinlock_lockval;		/* 0x03b8 */
+	__u32	fpu_flags;			/* 0x03bc */
+	__u8	pad_0x03c0[0x0400-0x03c0];	/* 0x03c0 */
 
 	/* Per cpu primary space access list */
 	__u32	paste[16];			/* 0x0400 */
diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index bf8b2e2..0234eea 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -111,7 +111,10 @@ struct thread_struct {
 	unsigned int  acrs[NUM_ACRS];
         unsigned long ksp;              /* kernel stack pointer             */
 	unsigned long user_timer;	/* task cputime in user space */
+	unsigned long guest_timer;	/* task cputime in kvm guest */
 	unsigned long system_timer;	/* task cputime in kernel space */
+	unsigned long hardirq_timer;	/* task cputime in hardirq context */
+	unsigned long softirq_timer;	/* task cputime in softirq context */
 	unsigned long sys_call_table;	/* system call table address */
 	mm_segment_t mm_segment;
 	unsigned long gmap_addr;	/* address of last gmap fault. */
diff --git a/arch/s390/kernel/vtime.c b/arch/s390/kernel/vtime.c
index 9a6c957..b6de91e 100644
--- a/arch/s390/kernel/vtime.c
+++ b/arch/s390/kernel/vtime.c
@@ -90,14 +90,30 @@ static void update_mt_scaling(void)
 	__this_cpu_write(mt_scaling_jiffies, jiffies_64);
 }
 
+static inline u64 update_tsk_timer(unsigned long *tsk_vtime, u64 new)
+{
+	u64 delta;
+
+	delta = new - *tsk_vtime;
+	*tsk_vtime = new;
+	return delta;
+}
+
+static inline u64 scale_vtime(u64 vtime)
+{
+	u64 mult = __this_cpu_read(mt_scaling_mult);
+	u64 div = __this_cpu_read(mt_scaling_div);
+
+	return smp_cpu_mtid ? (vtime * mult / div) : vtime;
+}
+
 /*
  * 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)
+static int do_account_vtime(struct task_struct *tsk)
 {
-	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,34 +126,47 @@ 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 (hardirq_count())
+		S390_lowcore.hardirq_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();
 
-	user = S390_lowcore.user_timer - tsk->thread.user_timer;
-	S390_lowcore.steal_timer -= user;
-	tsk->thread.user_timer = S390_lowcore.user_timer;
-
-	system = S390_lowcore.system_timer - tsk->thread.system_timer;
-	S390_lowcore.steal_timer -= system;
-	tsk->thread.system_timer = S390_lowcore.system_timer;
-
-	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);
-
-		user_scaled = (user_scaled * mult) / div;
-		system_scaled = (system_scaled * mult) / div;
-	}
-	account_user_time(tsk, user, user_scaled);
-	account_system_time(tsk, hardirq_offset, system, system_scaled);
+	/* Calculate cputime delta */
+	user = update_tsk_timer(&tsk->thread.user_timer,
+				READ_ONCE(S390_lowcore.user_timer));
+	guest = update_tsk_timer(&tsk->thread.guest_timer,
+				 READ_ONCE(S390_lowcore.guest_timer));
+	system = update_tsk_timer(&tsk->thread.system_timer,
+				  READ_ONCE(S390_lowcore.system_timer));
+	hardirq = update_tsk_timer(&tsk->thread.hardirq_timer,
+				   READ_ONCE(S390_lowcore.hardirq_timer));
+	softirq = update_tsk_timer(&tsk->thread.softirq_timer,
+				   READ_ONCE(S390_lowcore.softirq_timer));
+	S390_lowcore.steal_timer +=
+		clock - user - guest - system - hardirq - softirq;
+
+	/* Push accounting values */
+	if (user)
+		account_user_time(tsk, user, scale_vtime(user));
+	if (guest)
+		account_guest_time(tsk, guest, scale_vtime(guest));
+	if (system)
+		account_system_time_native(tsk, system, scale_vtime(system),
+					   CPUTIME_SYSTEM);
+	if (hardirq)
+		account_system_time_native(tsk, hardirq, scale_vtime(hardirq),
+					   CPUTIME_IRQ);
+	if (softirq)
+		account_system_time_native(tsk, softirq, scale_vtime(softirq),
+					   CPUTIME_SOFTIRQ);
 
 	steal = S390_lowcore.steal_timer;
 	if ((s64) steal > 0) {
@@ -145,16 +174,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(timer);
 }
 
 void vtime_task_switch(struct task_struct *prev)
 {
-	do_account_vtime(prev, 0);
+	do_account_vtime(prev);
 	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;
 }
 
 /*
@@ -164,7 +199,7 @@ void vtime_task_switch(struct task_struct *prev)
  */
 void vtime_account_user(struct task_struct *tsk)
 {
-	if (do_account_vtime(tsk, HARDIRQ_OFFSET))
+	if (do_account_vtime(tsk))
 		virt_timer_expire();
 }
 
@@ -174,31 +209,22 @@ void vtime_account_user(struct task_struct *tsk)
  */
 void vtime_account_irq_enter(struct task_struct *tsk)
 {
-	u64 timer, system, system_scaled;
+	u64 timer;
 
 	timer = S390_lowcore.last_update_timer;
 	S390_lowcore.last_update_timer = get_vtimer();
-	S390_lowcore.system_timer += timer - S390_lowcore.last_update_timer;
-
-	/* Update MT utilization calculation */
-	if (smp_cpu_mtid &&
-	    time_after64(jiffies_64, this_cpu_read(mt_scaling_jiffies)))
-		update_mt_scaling();
-
-	system = S390_lowcore.system_timer - tsk->thread.system_timer;
-	S390_lowcore.steal_timer -= system;
-	tsk->thread.system_timer = S390_lowcore.system_timer;
-	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);
-
-		system_scaled = (system_scaled * mult) / div;
-	}
-	account_system_time(tsk, 0, system, system_scaled);
-
-	virt_timer_forward(system);
+	timer -= S390_lowcore.last_update_timer;
+
+	if ((tsk->flags & PF_VCPU) && (irq_count() == 0))
+		S390_lowcore.guest_timer += timer;
+	else if (hardirq_count())
+		S390_lowcore.hardirq_timer += timer;
+	else if (in_serving_softirq())
+		S390_lowcore.softirq_timer += timer;
+	else
+		S390_lowcore.system_timer += timer;
+
+	virt_timer_forward(timer);
 }
 EXPORT_SYMBOL_GPL(vtime_account_irq_enter);
 
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 44fda64..a7e7951 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -80,10 +80,14 @@ static inline unsigned int kstat_cpu_irqs_sum(unsigned int cpu)
 
 extern void account_user_time(struct task_struct *, cputime_t, cputime_t);
 extern void account_system_time(struct task_struct *, int, cputime_t, cputime_t);
+extern void account_guest_time(struct task_struct *, cputime_t, cputime_t);
 extern void account_steal_time(cputime_t);
 extern void account_idle_time(cputime_t);
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+extern void account_system_time_native(struct task_struct *, cputime_t,
+				       cputime_t, int);
+
 static inline void account_process_tick(struct task_struct *tsk, int user)
 {
 	vtime_account_user(tsk);
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 5ebee31..9e6c5aa 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -155,8 +155,8 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
  * @cputime: the cpu time spent in virtual machine since the last update
  * @cputime_scaled: cputime scaled by cpu frequency
  */
-static void account_guest_time(struct task_struct *p, cputime_t cputime,
-			       cputime_t cputime_scaled)
+void account_guest_time(struct task_struct *p, cputime_t cputime,
+			cputime_t cputime_scaled)
 {
 	u64 *cpustat = kcpustat_this_cpu->cpustat;
 
@@ -199,6 +199,14 @@ void __account_system_time(struct task_struct *p, cputime_t cputime,
 	acct_account_cputime(p);
 }
 
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+void account_system_time_native(struct task_struct *p, cputime_t cputime,
+			cputime_t cputime_scaled, int index)
+{
+	__account_system_time(p, cputime, cputime_scaled, index);
+}	
+#endif
+
 /*
  * Account system cpu time to a process.
  * @p: the process that the cpu time gets accounted to
-- 
2.8.4
-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1526879

FromFrederic Weisbecker <fweisbec@gmail.com>
Date2016-11-21 17:30 +0100
Message-ID<sFZDH-3yJ-21@gated-at.bofh.it>
In reply to#1526412
On Mon, Nov 21, 2016 at 07:59:56AM +0100, Martin Schwidefsky wrote:
> On Fri, 18 Nov 2016 15:47:02 +0100
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Just because some code isn't too complex doesn't mean we really want to keep it.
> > I get regular questions about what unit does cputime_t map to on a given
> > configuration. Everybody gets confused about that. On many of the
> > patches we got on cputime for the last years, I had to fix quite some issues
> > with bad granularity assumption. In fact most fixes that came to kernel/sched/cputime.c
> > recently, after merge or review, were about people getting confused with cputime_t granularity.
> 
> These regular question you get about the cputime_t is exactly what I was referring
> to. If the value would just be a u64 the guys asking the question about cputime_t
> would just assume the value to be nano-seconds and then go ahead and break things.

Sure, replacing cputime_t with u64 without changing the unit wouldn't help. But changing
it to nsecs and expect people to deduce it from the u64 type sounds a good direction.

> 
> > Especially for stats that come from nsecs clocks (steal and irqtime), we always have to maintain an
> > accumulator and make sure we don't lose some nanosec deltas.
> 
> Yes, for the CONFIG_IRQ_TIME_ACCOUNTING=y case.

Right.

> 
> > And we have to maintain several workarounds, sometimes even in the fastpath in
> > order to cope with the cputime_t random granularity all over.
> > 
> > Some fastpath examples:
> > 
> > * steal time accounting (need to convert nsecs to cputime then back)
> > * irqtime accounting (maintain accumulators)
> > * cputime_adjust, used on any user read of cputime (need to convert from nsecs
> >   to cputime on cputime_adjust)
> > 
> > But the worst really is about maintainance. This patchset removes around 600 lines.
> 
> Well 300 lines is from the powerpc and s390 cputime.h header and ~200 from
> the generic cputime_jiffies.h and cputime_nsecs.h.

Well, still worth it :-)

> > > The do_account_vtime function is called once per jiffy and once per task
> > > switch. HZ is usually set to 100 for s390, the conversion once per jiffy
> > > would not be so bad, but the call on the scheduling path *will* hurt.
> > 
> > I don't think we need to flush on task switch. If we maintain the accumulators
> > on the task/thread struct instead of per-cpu, then the remaining time after
> > task switch out will be accounted on next tick after after next task switch in.
> 
> You can not properly calculate steal time if you allow sleeping tasks to sit on
> up to 5*HZ worth of cpu time.

Ah, you mean that when the task goes to sleep, we shouldn't miss more than one
tick worth of system/user time but the steal time can be much higher, right?

> I think we *have* to do accounting on task switch.
> At least on s390, likely on powerpc as well. Why not make that an option for
> the architecture with the yet-to-be-written accumulating code.

Ok, how about doing the accumulation and always account on task switch for now,
we'll see later if it's worth having such an option.

> 
> > > What is even worse is the vtime_account_irq_enter path, that is call several
> > > times for each *interrupt*, at least two times for an interrupt without
> > > additional processing and four times if a softirq is triggered.
> > 
> > Actually maintaining an accumulator to flush on ticks is probably going to increase
> > the perf because of that. account_system_time() is called twice per interrupt, and
> > such function do much more than just account the time to the task_struct and cpustat
> > fields. The same applies to userspace boundaries and context switch. The account_*_time()
> > functions can be expensive.
> 
> The account_system_time twice per interrupt can be removed with the accumulation
> idea. We will have to see how expensive the accounting_xxx_time calls are on
> the context switch path.

Right.

> 
> > > 
> > > Now it has been proposed to implement lazy accounting to accumulate deltas
> > > and do the expensive conversions only infrequently. This is pretty straight-
> > > forward for account_user_time but to do this for the account_system_time
> > > function is more complicated. The function has to differentiate between
> > > guest/hardirq/softirq and pure system time. We would need to keep sums for
> > > each bucket and provide a separate function to add to each bucket. Like
> > > account_guest_time(), account_hardirq_time(), account_softirq_time() and
> > > account_system_time(). Then it is up to the arch code to sort out the details
> > > and call the accounting code once per jiffy for each of the buckets.
> > 
> > That wouldn't be too hard really. The s390 code in vtime.c already does that.
> 
> Yes, I agree that the accumulating change would not be too hard. Can I make the
> request that we try to get that done first before doing the cleanup ?

Of course. I see you started something, I'll be glad to help!

>  
> > > We still have to do the whole thing on each task switch though.
> > 
> > Not if we maintain the deltas in the task_struct.
> > 
> > > 
> > > But I am still not happy about the approach. What is the compelling reason
> > > for this change except for the "but it looks ugly"?
> > 
> > The diffstat (600 lines removed). Also the fact that we have all these workarounds
> > in the core code just for the special case of 1 arch (s390) and a half
> > (powerpc with CONFIG_VIRT_CPU_ACCOUNTING_NATIVE).
> > 
> > I'd much rather have all that complexity moved in a vtime_native.c shared by s390 and powerpc
> > that takes care of proper accumulation in cputime_t and flushes that on ticks in nsecs rather
> > than having all these cputime_t game all over the kernel.
> 
> The goal to have nano-seconds only in the core code is a good one. And with the
> accumulator I think s390 can live with it. The change would have a real upside
> too. There are these stupid divisions for scaled cputime that we have to calculate
> for every call to account_xxx_time(). These would not be done for the interrupts
> anymore.

Exactly!


Thanks.

[toc] | [prev] | [next] | [standalone]

#1527248

FromMartin Schwidefsky <schwidefsky@de.ibm.com>
Date2016-11-22 07:20 +0100
Message-ID<sGcAV-3v7-5@gated-at.bofh.it>
In reply to#1526879
On Mon, 21 Nov 2016 17:20:06 +0100
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Mon, Nov 21, 2016 at 07:59:56AM +0100, Martin Schwidefsky wrote:
> > On Fri, 18 Nov 2016 15:47:02 +0100
> > Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > > The do_account_vtime function is called once per jiffy and once per task
> > > > switch. HZ is usually set to 100 for s390, the conversion once per jiffy
> > > > would not be so bad, but the call on the scheduling path *will* hurt.
> > > 
> > > I don't think we need to flush on task switch. If we maintain the accumulators
> > > on the task/thread struct instead of per-cpu, then the remaining time after
> > > task switch out will be accounted on next tick after after next task switch in.
> > 
> > You can not properly calculate steal time if you allow sleeping tasks to sit on
> > up to 5*HZ worth of cpu time.
> 
> Ah, you mean that when the task goes to sleep, we shouldn't miss more than one
> tick worth of system/user time but the steal time can be much higher, right?

No, it is worse than that. Consider a task going to sleep just before a tick
arrives. It will have almost a full HZ time-slice in its task specific accounting
numbers. After the switch another task with a different set of accounting numbers
is running. The tick will not push the cputime for the work done in the last
HZ period. Dependent on what the new task has in its accounting number the steal
time calculation can give you anything. Repeat the whole thing with any number
of tasks and the missing cputime can get really large. Now get one of these
processes back at the beginning of a time slice and you can get nearly 200% worth
of cputime in one tick. Switch to the next task with missing cputime at the start
of the new tick and you can get many ticks with too much cputime.

Not doing accounting on task switch is just broken.

> > I think we *have* to do accounting on task switch.
> > At least on s390, likely on powerpc as well. Why not make that an option for
> > the architecture with the yet-to-be-written accumulating code.
> 
> Ok, how about doing the accumulation and always account on task switch for now,
> we'll see later if it's worth having such an option.

I am convinced that we need it. The prototype patch does it for s390.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

[toc] | [prev] | [next] | [standalone]

#1526491

FromIngo Molnar <mingo@kernel.org>
Date2016-11-21 10:50 +0100
Message-ID<sFToC-7Sv-21@gated-at.bofh.it>
In reply to#1525449
* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > But I am still not happy about the approach. What is the compelling reason for 
> > this change except for the "but it looks ugly"?
> 
> The diffstat (600 lines removed). Also the fact that we have all these 
> workarounds in the core code just for the special case of 1 arch (s390) and a 
> half (powerpc with CONFIG_VIRT_CPU_ACCOUNTING_NATIVE).
> 
> I'd much rather have all that complexity moved in a vtime_native.c shared by 
> s390 and powerpc that takes care of proper accumulation in cputime_t and flushes 
> that on ticks in nsecs rather than having all these cputime_t game all over the 
> kernel.

I agree - we really want to concentrate complexity in such a fashion and generally 
standardize on nanosecs, and the diffstat of the patchset is really nice.

The patchset obviously has to build (and work!) on s390/powerpc properly.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1526876

FromFrederic Weisbecker <fweisbec@gmail.com>
Date2016-11-21 17:30 +0100
Message-ID<sFZDH-3yJ-19@gated-at.bofh.it>
In reply to#1526491
On Mon, Nov 21, 2016 at 10:49:23AM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > > But I am still not happy about the approach. What is the compelling reason for 
> > > this change except for the "but it looks ugly"?
> > 
> > The diffstat (600 lines removed). Also the fact that we have all these 
> > workarounds in the core code just for the special case of 1 arch (s390) and a 
> > half (powerpc with CONFIG_VIRT_CPU_ACCOUNTING_NATIVE).
> > 
> > I'd much rather have all that complexity moved in a vtime_native.c shared by 
> > s390 and powerpc that takes care of proper accumulation in cputime_t and flushes 
> > that on ticks in nsecs rather than having all these cputime_t game all over the 
> > kernel.
> 
> I agree - we really want to concentrate complexity in such a fashion and generally 
> standardize on nanosecs, and the diffstat of the patchset is really nice.

Yeah, although the diffstat might become less impressive after the accumulator code,
but still worth it I think.

> The patchset obviously has to build (and work!) on s390/powerpc properly.

Of course!

Thanks.

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web