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

[Nick Piggin]

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.

> +
> +/*
> + * Return current->sched_time plus any more ns on the sched_clock
> + * that have not yet been banked.
> + */
> +unsigned long long current_sched_time(const task_t *tsk)
> +{
> +	unsigned long long ns;
> +	local_irq_disable();
> +	ns = max(tsk->timestamp, task_rq(tsk)->timestamp_last_tick);
> +	ns = tsk->sched_time + (sched_clock() - ns);
> +	local_irq_enable();
> +	return ns;
> +}
> +

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

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...?

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). Again this is something that I
haven't thought about much because it is not a big deal for current usage.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/