Re: [PATCH v2] cputime: fix invalid gtime

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Oct 29, 2015 at 04:30:20AM +0000, Hiroshi Shimamoto wrote:
> > Subject: Re: [PATCH v2] cputime: fix invalid gtime
> > 
> > On Thu, Oct 29, 2015 at 01:10:01AM +0000, Hiroshi Shimamoto wrote:
> > > > Obviously I completely messed up there. And task_cputime() has a similar issue
> > > > but it happens to work due to vtime_snap_whence set to VTIME_SLEEPING when vtime
> > > > doesn't run. Still it works at the cost of a seqcount read operation.
> > > >
> > > > Do you think you could fix it too (along with task_cputime_scaled())? I think those
> > > > patches will also need a stable tag.
> > >
> > > Do you mean that task_cputime() and task_cputime_scaled() don't hit invalid behavior
> > > but have some extra operation cost which could be removed?
> > 
> > Exactly.
> > 
> > >
> > > Will look into it, and send patches with stable tag.
> > 
> > Thanks a lot!
> > 
> > Oh and another detail: vtime_accounting_enabled() checks if vtime
> > accounting is done precisely on the current CPU. That's what we want to check
> > when we account the time but not when we want to read the cputime of a task.
> > 
> > For example, CPU 0 never has vtime_accounting_enabled() because it plays the
> > role of timekeeper and as such it keeps the tick periodic. So if task A runs on
> > CPU 1 that has vtime accounting on, and we read the cputime of task A from CPU 0,
> > vtime_accounting_enabled() will be false whereas we need to compute the delta.
> > 
> > So vtime_accounting_enabled() isn't suitable to check if vtime is running on _some_
> > CPU such that we can't return utime/stime with a raw read.
> 
> I see the point, vtime accounting can be enabled on dedicated cpu and there is no
> guarantee the reading thread is on the same state.

Exactly!

> 
> > 
> > Ideally we shoud rename vtime_accounting_enabled() to vtime_accounting_cpu_enabled()
> > and have vtime_accounting_enabled() to check if vtime runs somewhere. But that would
> > be too much an invasive change for a stable patch. So lets just use
> > context_tracking_is_enabled() for now instead.
> 
> I have dig the code.
> And my understanding is that vtime_accounting_enabled() does check global flag with
> context_tracking_is_enabled() and then check current cpu state with
> context_tracking_cpu_is_enabled(). For now, we just check global flag to fix current
> issue instead of checking both in vtime_accounting_enabled(). In future we should fix
> more precisely.
> Is that correct?

Perfectly correct :-)

Thanks!