Re: [PATCH 2/2] posix-cpu-timers: fix wrong timer initialization

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Apr 29, 2013 at 02:26:02AM -0400, kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> Currently glibc's rt/tst-cputimer1 testcase is spradically fail because
> a timer created by timer_create() may faire earlier than an argument.
> 
> There are two faults. 1) cpu_timer_sample_group() adds task_delta_exec(current).
> But it is definity silly idea especially when multi thread. cputimer should
> be initialized by committed exec runtime. i.e. it should not be added
> scheduler delta. 2) expire time should be current time + timeout. In the other
> words, expire calculation should take care scheduler delta.

I'm sorry, that completely fails to parse.

> -/*
> - * Lock/unlock the current runqueue - to extract task statistics:
> - */
> -extern unsigned long long task_delta_exec(struct task_struct *);

Yay.. this thing dying is good -- it did seem strange to compute the current
delta but not also read sum_exec_runtime under the same lock.

> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index e56be4c..dc61bc3 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -203,12 +203,10 @@ posix_cpu_clock_set(const clockid_t which_clock, const struct timespec *tp)
>  	return error;
>  }
>  
> -
> -/*
> - * Sample a per-thread clock for the given task.
> - */
> -static int cpu_clock_sample(const clockid_t which_clock, struct task_struct *p,
> -			    union cpu_time_count *cpu)
> +static int do_cpu_clock_sample(const clockid_t which_clock,
> +			       struct task_struct *p,
> +			       bool add_delta,
> +			       union cpu_time_count *cpu)

Would not thread_cputime() (to mirror thread_group_cputime()) be a better name?

Also, I would think both these functions would be a good place to insert a
comment explaining the difference between timer and clock.

> +static int cpu_clock_sample(const clockid_t which_clock, struct task_struct *p,
> +			    union cpu_time_count *cpu)
> +{
> +	return do_cpu_clock_sample(which_clock, p, true, cpu);
> +}

> @@ -700,7 +707,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int flags,
>  	 * check if it's already passed.  In short, we need a sample.
>  	 */
>  	if (CPUCLOCK_PERTHREAD(timer->it_clock)) {
> -		cpu_clock_sample(timer->it_clock, p, &val);
> +		do_cpu_clock_sample(timer->it_clock, p, false, &val);
>  	} else {
>  		cpu_timer_sample_group(timer->it_clock, p, &val);
>  	}

This would suggest:

static inline int cpu_timer_sample(const clockid_t which_clock, struct task_struct *p, union cpu_time_count *cpu)
{
	return do_cpu_clock_sample(which_clock, p, false, cpu);
}

That would preserve the: cpu_{timer,clock}_sample{,_group}() form.

> @@ -749,7 +756,13 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int flags,
>  	}
>  
>  	if (new_expires.sched != 0 && !(flags & TIMER_ABSTIME)) {
> -		cpu_time_add(timer->it_clock, &new_expires, val);
> +		union cpu_time_count now;
> +
> +		if (CPUCLOCK_PERTHREAD(timer->it_clock))
> +			cpu_clock_sample(timer->it_clock, p, &now);
> +		else
> +			cpu_clock_sample_group(timer->it_clock, p, &now);

This triggered a pattern match against earlier in this function; but they're
different now; timer vs clock. So nothing to merge...


So I don't mind the code changes, although its still not entirely clear to me
what exact problem is fixed how; and thus the Changelog needs TLC.