Date: Wed, 12 Apr 2017 15:18:19 +0200
From: Frederic Weisbecker <fweisbec@gmail.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Wanpeng Li <kernellwp@gmail.com>, Mike Galbraith <efault@gmx.de>,
        Rik van Riel <riel@redhat.com>,
        Luiz Capitulino <lcapitulino@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [BUG nohz]: wrong user and system time accounting
Message-ID: <20170412131818.GB21309@lerouge>
References: <20170323165512.60945ac6@redhat.com>
 <CANRm+CxcgSP2-x+A822DmHLvFLzFmTptS6oYwYtwVdErTpiB=Q@mail.gmail.com>
 <1490636129.8850.76.camel@redhat.com>
 <20170328132406.7d23579c@redhat.com>
 <20170329131656.1d6cb743@redhat.com>
 <1490818125.28917.11.camel@redhat.com>
 <1490848051.4167.57.camel@gmx.de>
 <CANRm+CzfqkU6iV1BFk3PmWvy7MOsYH1mSwejJXMkvWW9C5ngwg@mail.gmail.com>
 <alpine.DEB.2.20.1704111523100.1772@nanos>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.20.1704111523100.1772@nanos>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2630
Lines: 83

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 ?