> 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.
>
> 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?
thanks,
Hiroshi
>
> 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
> > > >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
