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

Groups > linux.kernel > #1621359 > unrolled thread

Re: [BUG nohz]: wrong user and system time accounting

Started byThomas Gleixner <tglx@linutronix.de>
First post2017-04-11 16:30 +0200
Last post2017-04-13 15:40 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [BUG nohz]: wrong user and system time accounting Thomas Gleixner <tglx@linutronix.de> - 2017-04-11 16:30 +0200
    Re: [BUG nohz]: wrong user and system time accounting Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-12 15:20 +0200
      Re: [BUG nohz]: wrong user and system time accounting Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 17:00 +0200
        Re: [BUG nohz]: wrong user and system time accounting Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-12 17:20 +0200
        Re: [BUG nohz]: wrong user and system time accounting Wanpeng Li <kernellwp@gmail.com> - 2017-04-13 06:40 +0200
          Re: [BUG nohz]: wrong user and system time accounting Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-13 15:40 +0200

#1621359 — Re: [BUG nohz]: wrong user and system time accounting

FromThomas Gleixner <tglx@linutronix.de>
Date2017-04-11 16:30 +0200
SubjectRe: [BUG nohz]: wrong user and system time accounting
Message-ID<tv4XV-8aE-43@gated-at.bofh.it>
On Thu, 30 Mar 2017, Wanpeng Li wrote:
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index f3778e2b..f1ee393 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -676,18 +676,21 @@ void thread_group_cputime_adjusted(struct
> task_struct *p, u64 *ut, u64 *st)
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
>  static u64 vtime_delta(struct task_struct *tsk)
>  {
> -    unsigned long now = READ_ONCE(jiffies);
> +    u64 now = local_clock();
> +    u64 delta;
> +
> +    delta = now - tsk->vtime_snap;
> 
> -    if (time_before(now, (unsigned long)tsk->vtime_snap))
> +    if (delta < TICK_NSEC)
>          return 0;
> 
> -    return jiffies_to_nsecs(now - tsk->vtime_snap);
> +    return jiffies_to_nsecs(delta / TICK_NSEC);

So you replaced a jiffies based approach with a jiffies based approach.

>  }
> 
>  static u64 get_vtime_delta(struct task_struct *tsk)
>  {
> -    unsigned long now = READ_ONCE(jiffies);
> -    u64 delta, other;
> +    u64 delta = vtime_delta(tsk);
> +    u64 other;
> 
>      /*
>       * Unlike tick based timing, vtime based timing never has lost
> @@ -696,10 +699,9 @@ static u64 get_vtime_delta(struct task_struct *tsk)
>       * elapsed time. Limit account_other_time to prevent rounding
>       * errors from causing elapsed vtime to go negative.
>       */
> -    delta = jiffies_to_nsecs(now - tsk->vtime_snap);
>      other = account_other_time(delta);
>      WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE);
> -    tsk->vtime_snap = now;
> +    tsk->vtime_snap += delta;

Here is how it works^Wfails

	       For simplicity tsk->vtime_snap starts at 0
	       HZ = 1000

CPU0 	       CPU1
	       sysexit()
		 account_system()
		   now == 0
		   delta = vtime_delta()	<- 0ns
	           tsk->vtime_snap += delta;	== 0ns

	       busy_loop(995us)

	       sysenter()
		 now == 996us
		 account_user()
		   delta = vtime_delta()	<- 0ns
		   tsk->vtime_snap += delta	== 0ns
		   
	       sysexit()
		 account_system()
		   now == 1001us
		   delta = vtime_delta()	<- 10000000ns

		   ^^^^ Gets accounted to system

	           tsk->vtime_snap += delta;	== 10000000ns

It's not different from the current jiffies based stuff at all. Same
failure mode.

Thanks,

	tglx

[toc] | [next] | [standalone]

#1622129

FromFrederic Weisbecker <fweisbec@gmail.com>
Date2017-04-12 15:20 +0200
Message-ID<tvqlI-4Uw-17@gated-at.bofh.it>
In reply to#1621359
On Tue, Apr 11, 2017 at 04:22:48PM +0200, Thomas Gleixner wrote:
> On Thu, 30 Mar 2017, Wanpeng Li wrote:
> > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> > index f3778e2b..f1ee393 100644
> > --- a/kernel/sched/cputime.c
> > +++ b/kernel/sched/cputime.c
> > @@ -676,18 +676,21 @@ void thread_group_cputime_adjusted(struct
> > task_struct *p, u64 *ut, u64 *st)
> >  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> >  static u64 vtime_delta(struct task_struct *tsk)
> >  {
> > -    unsigned long now = READ_ONCE(jiffies);
> > +    u64 now = local_clock();
> > +    u64 delta;
> > +
> > +    delta = now - tsk->vtime_snap;
> > 
> > -    if (time_before(now, (unsigned long)tsk->vtime_snap))
> > +    if (delta < TICK_NSEC)
> >          return 0;
> > 
> > -    return jiffies_to_nsecs(now - tsk->vtime_snap);
> > +    return jiffies_to_nsecs(delta / TICK_NSEC);
> 
> So you replaced a jiffies based approach with a jiffies based approach.
> 
> >  }
> > 
> >  static u64 get_vtime_delta(struct task_struct *tsk)
> >  {
> > -    unsigned long now = READ_ONCE(jiffies);
> > -    u64 delta, other;
> > +    u64 delta = vtime_delta(tsk);
> > +    u64 other;
> > 
> >      /*
> >       * Unlike tick based timing, vtime based timing never has lost
> > @@ -696,10 +699,9 @@ static u64 get_vtime_delta(struct task_struct *tsk)
> >       * elapsed time. Limit account_other_time to prevent rounding
> >       * errors from causing elapsed vtime to go negative.
> >       */
> > -    delta = jiffies_to_nsecs(now - tsk->vtime_snap);
> >      other = account_other_time(delta);
> >      WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE);
> > -    tsk->vtime_snap = now;
> > +    tsk->vtime_snap += delta;
> 
> Here is how it works^Wfails
> 
> 	       For simplicity tsk->vtime_snap starts at 0
> 	       HZ = 1000
> 
> CPU0 	       CPU1
> 	       sysexit()
> 		 account_system()
> 		   now == 0
> 		   delta = vtime_delta()	<- 0ns
> 	           tsk->vtime_snap += delta;	== 0ns
> 
> 	       busy_loop(995us)
> 
> 	       sysenter()
> 		 now == 996us
> 		 account_user()
> 		   delta = vtime_delta()	<- 0ns
> 		   tsk->vtime_snap += delta	== 0ns
> 		   
> 	       sysexit()
> 		 account_system()
> 		   now == 1001us
> 		   delta = vtime_delta()	<- 10000000ns
> 
> 		   ^^^^ Gets accounted to system
> 
> 	           tsk->vtime_snap += delta;	== 10000000ns
> 
> It's not different from the current jiffies based stuff at all. Same
> failure mode.

Yes you're right, I got confused again. So to fix this we could do our snapshots
at a frequency lower than HZ but still high enough to avoid overhead.

Something like TICK_NSEC / 2 ?

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

#1622217

FromThomas Gleixner <tglx@linutronix.de>
Date2017-04-12 17:00 +0200
Message-ID<tvrUv-5IE-37@gated-at.bofh.it>
In reply to#1622129
On Wed, 12 Apr 2017, Frederic Weisbecker wrote:
> On Tue, Apr 11, 2017 at 04:22:48PM +0200, Thomas Gleixner wrote:
> > It's not different from the current jiffies based stuff at all. Same
> > failure mode.
> 
> Yes you're right, I got confused again. So to fix this we could do our snapshots
> at a frequency lower than HZ but still high enough to avoid overhead.
> 
> Something like TICK_NSEC / 2 ?

If you are using TSC anyway then you can do proper accumulation for both
system and user and only account the data when the accumulation is more
than a jiffie.

Thanks,

	tglx

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

#1622235

FromFrederic Weisbecker <fweisbec@gmail.com>
Date2017-04-12 17:20 +0200
Message-ID<tvsdQ-64C-31@gated-at.bofh.it>
In reply to#1622217
On Wed, Apr 12, 2017 at 04:57:58PM +0200, Thomas Gleixner wrote:
> On Wed, 12 Apr 2017, Frederic Weisbecker wrote:
> > On Tue, Apr 11, 2017 at 04:22:48PM +0200, Thomas Gleixner wrote:
> > > It's not different from the current jiffies based stuff at all. Same
> > > failure mode.
> > 
> > Yes you're right, I got confused again. So to fix this we could do our snapshots
> > at a frequency lower than HZ but still high enough to avoid overhead.
> > 
> > Something like TICK_NSEC / 2 ?
> 
> If you are using TSC anyway then you can do proper accumulation for both
> system and user and only account the data when the accumulation is more
> than a jiffie.

Sounds nice, and accumulation shouldn't introduce too much overhead. Let's try that.

Thanks.

> 
> Thanks,
> 
> 	tglx
>

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

#1622713

FromWanpeng Li <kernellwp@gmail.com>
Date2017-04-13 06:40 +0200
Message-ID<tvEI2-662-13@gated-at.bofh.it>
In reply to#1622217
2017-04-12 22:57 GMT+08:00 Thomas Gleixner <tglx@linutronix.de>:
> On Wed, 12 Apr 2017, Frederic Weisbecker wrote:
>> On Tue, Apr 11, 2017 at 04:22:48PM +0200, Thomas Gleixner wrote:
>> > It's not different from the current jiffies based stuff at all. Same
>> > failure mode.
>>
>> Yes you're right, I got confused again. So to fix this we could do our snapshots
>> at a frequency lower than HZ but still high enough to avoid overhead.
>>
>> Something like TICK_NSEC / 2 ?
>
> If you are using TSC anyway then you can do proper accumulation for both
> system and user and only account the data when the accumulation is more
> than a jiffie.

So I implement it as below:

- HZ=1000.
  1) two cpu hogs on cpu in nohz_full mode, 100% user time
  2) Luzi's testcase, ~95% user time, ~5% idle time (as we expected)
- HZ=250
   1) two cpu hogs on cpu in nohz_full mode, 100% user time
   2) Luzi's testcase, 100% idle

So the codes below still not work correctly for HZ=250, any suggestions?

-------------------------------------->8-----------------------------------------------------

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee8..6a11771 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -668,6 +668,8 @@ struct task_struct {
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
     seqcount_t            vtime_seqcount;
     unsigned long long        vtime_snap;
+    u64                vtime_acct_utime;
+    u64                vtime_acct_stime;
     enum {
         /* Task is sleeping or running in a CPU with VTIME inactive: */
         VTIME_INACTIVE = 0,
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index f3778e2b..f8e54ba 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -674,20 +674,41 @@ void thread_group_cputime_adjusted(struct
task_struct *p, u64 *ut, u64 *st)
 #endif /* !CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */

 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
-static u64 vtime_delta(struct task_struct *tsk)
+static u64 vtime_delta(struct task_struct *tsk, bool user)
 {
-    unsigned long now = READ_ONCE(jiffies);
+    u64 delta, ret = 0;

-    if (time_before(now, (unsigned long)tsk->vtime_snap))
-        return 0;
+    delta = sched_clock() - tsk->vtime_snap;

-    return jiffies_to_nsecs(now - tsk->vtime_snap);
+    if (is_idle_task(tsk)) {
+        if (delta >= TICK_NSEC)
+            ret = delta;
+    } else {
+        if (user) {
+            tsk->vtime_acct_utime += delta;
+            if (tsk->vtime_acct_utime >= TICK_NSEC)
+                ret = tsk->vtime_acct_utime;
+        } else {
+            tsk->vtime_acct_stime += delta;
+            if (tsk->vtime_acct_utime >= TICK_NSEC)
+                ret = tsk->vtime_acct_stime;
+        }
+    }
+
+    return ret;
 }

-static u64 get_vtime_delta(struct task_struct *tsk)
+static u64 get_vtime_delta(struct task_struct *tsk, bool user)
 {
-    unsigned long now = READ_ONCE(jiffies);
-    u64 delta, other;
+    u64 delta = vtime_delta(tsk, user);
+    u64 other;
+
+    if (!is_idle_task(tsk)) {
+        if (user)
+            tsk->vtime_acct_utime = 0;
+        else
+            tsk->vtime_acct_stime = 0;
+    }

     /*
      * Unlike tick based timing, vtime based timing never has lost
@@ -696,22 +717,21 @@ static u64 get_vtime_delta(struct task_struct *tsk)
      * elapsed time. Limit account_other_time to prevent rounding
      * errors from causing elapsed vtime to go negative.
      */
-    delta = jiffies_to_nsecs(now - tsk->vtime_snap);
     other = account_other_time(delta);
     WARN_ON_ONCE(tsk->vtime_snap_whence == VTIME_INACTIVE);
-    tsk->vtime_snap = now;
+    tsk->vtime_snap += delta;

     return delta - other;
 }

 static void __vtime_account_system(struct task_struct *tsk)
 {
-    account_system_time(tsk, irq_count(), get_vtime_delta(tsk));
+    account_system_time(tsk, irq_count(), get_vtime_delta(tsk, false));
 }

 void vtime_account_system(struct task_struct *tsk)
 {
-    if (!vtime_delta(tsk))
+    if (!vtime_delta(tsk, false))
         return;

     write_seqcount_begin(&tsk->vtime_seqcount);
@@ -723,15 +743,15 @@ void vtime_account_user(struct task_struct *tsk)
 {
     write_seqcount_begin(&tsk->vtime_seqcount);
     tsk->vtime_snap_whence = VTIME_SYS;
-    if (vtime_delta(tsk))
-        account_user_time(tsk, get_vtime_delta(tsk));
+    if (vtime_delta(tsk, true))
+        account_user_time(tsk, get_vtime_delta(tsk, true));
     write_seqcount_end(&tsk->vtime_seqcount);
 }

 void vtime_user_enter(struct task_struct *tsk)
 {
     write_seqcount_begin(&tsk->vtime_seqcount);
-    if (vtime_delta(tsk))
+    if (vtime_delta(tsk, false))
         __vtime_account_system(tsk);
     tsk->vtime_snap_whence = VTIME_USER;
     write_seqcount_end(&tsk->vtime_seqcount);
@@ -747,7 +767,7 @@ void vtime_guest_enter(struct task_struct *tsk)
      * that can thus safely catch up with a tickless delta.
      */
     write_seqcount_begin(&tsk->vtime_seqcount);
-    if (vtime_delta(tsk))
+    if (vtime_delta(tsk, false))
         __vtime_account_system(tsk);
     current->flags |= PF_VCPU;
     write_seqcount_end(&tsk->vtime_seqcount);
@@ -765,7 +785,7 @@ EXPORT_SYMBOL_GPL(vtime_guest_exit);

 void vtime_account_idle(struct task_struct *tsk)
 {
-    account_idle_time(get_vtime_delta(tsk));
+    account_idle_time(get_vtime_delta(tsk, false));
 }

 void arch_vtime_task_switch(struct task_struct *prev)
@@ -776,7 +796,7 @@ void arch_vtime_task_switch(struct task_struct *prev)

     write_seqcount_begin(&current->vtime_seqcount);
     current->vtime_snap_whence = VTIME_SYS;
-    current->vtime_snap = jiffies;
+    current->vtime_snap = sched_clock_cpu(smp_processor_id());
     write_seqcount_end(&current->vtime_seqcount);
 }

@@ -787,7 +807,7 @@ void vtime_init_idle(struct task_struct *t, int cpu)
     local_irq_save(flags);
     write_seqcount_begin(&t->vtime_seqcount);
     t->vtime_snap_whence = VTIME_SYS;
-    t->vtime_snap = jiffies;
+    t->vtime_snap = sched_clock_cpu(cpu);
     write_seqcount_end(&t->vtime_seqcount);
     local_irq_restore(flags);
 }
@@ -805,7 +825,7 @@ u64 task_gtime(struct task_struct *t)

         gtime = t->gtime;
         if (t->vtime_snap_whence == VTIME_SYS && t->flags & PF_VCPU)
-            gtime += vtime_delta(t);
+            gtime += vtime_delta(t, false);

     } while (read_seqcount_retry(&t->vtime_seqcount, seq));

@@ -819,7 +839,6 @@ u64 task_gtime(struct task_struct *t)
  */
 void task_cputime(struct task_struct *t, u64 *utime, u64 *stime)
 {
-    u64 delta;
     unsigned int seq;

     if (!vtime_accounting_enabled()) {
@@ -838,16 +857,14 @@ void task_cputime(struct task_struct *t, u64
*utime, u64 *stime)
         if (t->vtime_snap_whence == VTIME_INACTIVE || is_idle_task(t))
             continue;

-        delta = vtime_delta(t);
-
         /*
          * Task runs either in user or kernel space, add pending nohz time to
          * the right place.
          */
         if (t->vtime_snap_whence == VTIME_USER || t->flags & PF_VCPU)
-            *utime += delta;
+            *utime += vtime_delta(t, true);
         else if (t->vtime_snap_whence == VTIME_SYS)
-            *stime += delta;
+            *stime += vtime_delta(t, false);
     } while (read_seqcount_retry(&t->vtime_seqcount, seq));
 }
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_GEN */

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

#1623026

FromFrederic Weisbecker <fweisbec@gmail.com>
Date2017-04-13 15:40 +0200
Message-ID<tvN8C-3CF-23@gated-at.bofh.it>
In reply to#1622713
On Thu, Apr 13, 2017 at 12:31:12PM +0800, Wanpeng Li wrote:
> 2017-04-12 22:57 GMT+08:00 Thomas Gleixner <tglx@linutronix.de>:
> > On Wed, 12 Apr 2017, Frederic Weisbecker wrote:
> >> On Tue, Apr 11, 2017 at 04:22:48PM +0200, Thomas Gleixner wrote:
> >> > It's not different from the current jiffies based stuff at all. Same
> >> > failure mode.
> >>
> >> Yes you're right, I got confused again. So to fix this we could do our snapshots
> >> at a frequency lower than HZ but still high enough to avoid overhead.
> >>
> >> Something like TICK_NSEC / 2 ?
> >
> > If you are using TSC anyway then you can do proper accumulation for both
> > system and user and only account the data when the accumulation is more
> > than a jiffie.
> 
> So I implement it as below:
> 
> - HZ=1000.
>   1) two cpu hogs on cpu in nohz_full mode, 100% user time
>   2) Luzi's testcase, ~95% user time, ~5% idle time (as we expected)
> - HZ=250
>    1) two cpu hogs on cpu in nohz_full mode, 100% user time
>    2) Luzi's testcase, 100% idle
> 
> So the codes below still not work correctly for HZ=250, any suggestions?

Right, so first lets reorder that code a bit so we can see clear inside :-)

> 
> -------------------------------------->8-----------------------------------------------------
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee8..6a11771 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -668,6 +668,8 @@ struct task_struct {
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
>      seqcount_t            vtime_seqcount;
>      unsigned long long        vtime_snap;
> +    u64                vtime_acct_utime;
> +    u64                vtime_acct_stime;

You need to accumulate guest and steal time as well.

>      enum {
>          /* Task is sleeping or running in a CPU with VTIME inactive: */
>          VTIME_INACTIVE = 0,
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index f3778e2b..f8e54ba 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -674,20 +674,41 @@ void thread_group_cputime_adjusted(struct
> task_struct *p, u64 *ut, u64 *st)
>  #endif /* !CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
> 
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> -static u64 vtime_delta(struct task_struct *tsk)
> +static u64 vtime_delta(struct task_struct *tsk, bool user)
>  {
> -    unsigned long now = READ_ONCE(jiffies);
> +    u64 delta, ret = 0;
> 
> -    if (time_before(now, (unsigned long)tsk->vtime_snap))
> -        return 0;
> +    delta = sched_clock() - tsk->vtime_snap;
> 
> -    return jiffies_to_nsecs(now - tsk->vtime_snap);
> +    if (is_idle_task(tsk)) {
> +        if (delta >= TICK_NSEC)
> +            ret = delta;
> +    } else {
> +        if (user) {
> +            tsk->vtime_acct_utime += delta;
> +            if (tsk->vtime_acct_utime >= TICK_NSEC)
> +                ret = tsk->vtime_acct_utime;
> +        } else {
> +            tsk->vtime_acct_stime += delta;
> +            if (tsk->vtime_acct_utime >= TICK_NSEC)
> +                ret = tsk->vtime_acct_stime;
> +        }

We already have vtime_account_idle, vtime_account_user, etc...
The accumulation should be done by these functions that know what and where
to account. vtime_delta() should really just return the difference against vtime_snap,
it's too low level to care about these details.

> +    }
> +
> +    return ret;
>  }
> 
> -static u64 get_vtime_delta(struct task_struct *tsk)
> +static u64 get_vtime_delta(struct task_struct *tsk, bool user)
>  {
> -    unsigned long now = READ_ONCE(jiffies);
> -    u64 delta, other;
> +    u64 delta = vtime_delta(tsk, user);
> +    u64 other;
> +
> +    if (!is_idle_task(tsk)) {
> +        if (user)
> +            tsk->vtime_acct_utime = 0;
> +        else
> +            tsk->vtime_acct_stime = 0;
> +    }

Like vtime_delta(), get_vtime_delta() shouldn't touch these accumulators.
Reset and accounting really should be done by the upper level functions
vtime_account_*()

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web