(4/29/13 6:36 AM), Peter Zijlstra wrote:
> On Mon, Apr 29, 2013 at 02:26:02AM -0400, kosaki.motoh...@gmail.com wrote:
>> From: KOSAKI Motohiro <kosaki.motoh...@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?
agreed.
> Also, I would think both these functions would be a good place to insert a
> comment explaining the difference between timer and clock.
agreed.
>
>> +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.
Yeah, agreed.
And also, all timer function should use cpu_timer_sample() instead of
cpu_clock_sample().
check_thread_timers() uses p->se.sum_exec_runtime without delta. This is
consitency with
per-process timer. Thus, other functions (e.g. posix_cpu_timers_get) should
also use the
same.
>
>> @@ -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...
Not different, I think.
Relative timeout need to calculate "now + timeout" by definition.
But which time is "now"?
Example, thread1 has 10ms sum_exec_runtime and 4ms delta and call
timer_settime(4ms).
Old code calculate an expire is 10+4=14. New one calculate 10+4+4=18.
Which expire is correct? When using old one, timer will fire just after
syscall. This
is posix violation.
In the other words,
sighandler(){
t1 = clock_gettime()
}
t0 = clock_gettime()
timer_settime(timeout);
... wait to fire
assert (t1 - t0 >= timeout)
This pseudo code must be true. it is snippest what glibc rt/tst-cputimer1 test
and failed.
> 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.
>
--
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/
