Re: [PATCH] CPU time clock support in clock_* syscalls

[Roland McGrath <roland@redhat.com>]

> Roland McGrath wrote:
> 
> >  /*
> > + * This is called on clock ticks and on context switches.
> > + * Bank in p->sched_time the ns elapsed since the last tick or switch.
> > + */
> > +static void update_cpu_clock(task_t *p, runqueue_t *rq,
> > +			     unsigned long long now)
> > +{
> > +	unsigned long long last = max(p->timestamp, rq->timestamp_last_tick);
> > +	p->sched_time += now - last;
> > +}
> 
> This looks wrong. But update_cpu_clock is never called from another
> CPU. In which case you don't need to worry about timestamp_last_tick.

I don't really understand this comment.  update_cpu_clock is called from
schedule and from scheduler_tick.  When it was last called by schedule,
p->timestamp will mark this time.  When it was last called by
p->scheduler_tick, rq->timestamp_last_tick will mark this time.
Hence the max of the two is the last time update_cpu_clock was called.

> This doesn't perform the timestamp_last_tick "normalisation" properly
> either.

I don't know what you think is missing.  If the "normalization" you are
talking about is this kind of thing:

		p->timestamp = p->timestamp - rq_src->timestamp_last_tick
				+ rq_dest->timestamp_last_tick;

then that is not relevant here.  That normalizes the timestamp to the new
CPU when changing from one CPU to another.  This is not something that
matters for the sched_time tracking, because that only uses the difference
between a timestamp when the thread went on a CPU and the timestamp when it
went off.  When a thread switches CPUs without yielding, this normalization
will happen to its ->timestamp, and then the next update_cpu_clock will be
taking the difference of the newly-appropriate ->timestamp value against
the current CPU's sched_clock value.

> It also seems to conveniently ignore locking when reading those values
> off another CPU. Not a big deal for dynamic load calculations, but I'm
> not so sure about your usage...?

Here again I don't know what you are talking about.  Nothing is ever read
"off another CPU".  A thread maintains its own sched_time counter while it
is running on a CPU.

> Lastly, even when using timestamp_last_tick correctly, I think sched_clock
> will still drift around slightly, especially if a task switches CPUs a lot
> (but not restricted to moving CPUs). 

Please explain.



Thanks,
Roland