Re: [PATCH v2] cputime: fix invalid gtime

[Frederic Weisbecker <fweisbec@gmail.com>]

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.

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 think task_gtime() is also buggy when the target task runs in a CPU that doesn't do
vtime accounting (whereas another CPU does vtime accounting). We should fix that with
using a new VTIME_GUEST value instead of (or along with) PF_VCPU. But that's another story,
your fix is much more important for now.

> 
> thanks,
> Hiroshi
> 
> > 
> > Thanks!
> > 
> > 
> > 
> > >  	do {
> > >  		seq = read_seqbegin(&t->vtime_seqlock);
> > >
> > > --
> > > 1.8.3.1
> > >