Re: [PATCH 2/7] posix-cpu-timers: fix acounting delta_exec twice

2013-05-10 Thread KOSAKI Motohiro 
>> @@ -250,8 +250,13 @@ void thread_group_cputimer(struct task_struct *tsk, 
>> struct task_cputime *times)
>>* values through the TIMER_ABSTIME flag, therefore we have
>>* to synchronize the timer to the clock every time we start
>>* it.
>> +  *
>> +  * Do not add the current delta, because
>> +  * account_group_exec_runtime() will also this delta and we
>> +  * wouldn't want to double account time and get ahead of
>> +  * ourselves.
>>*/
>> - thread_group_cputime(tsk, &sum);
>> + thread_group_cputime(tsk, false, &sum);
>>   raw_spin_lock_irqsave(&cputimer->lock, flags);
>
> I wonder if we should move thread_group_cputime() inside this lock.
> Otherwise we can miss some updates in-between.

Hmm..

I don't agree with this. Right, we can miss some updates. But 1)
cputimer->lock doesn't
prevent any update update_curr() only take rq_lock, and 2) POSIX timer
and sleeping
semantics allow longer sleep than an argument. Then, the missing is
safe, nobody
can observe which of the timer_setime() syscall and update_curr()
happened earlier.

Ah, I'm now finding when update_gt_cputime() effectively work. It
helps to avoid timer_settime() vs timer_settime() mess.
--
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/

Re: [PATCH 2/7] posix-cpu-timers: fix acounting delta_exec twice

2013-05-10 Thread Frederic Weisbecker 
On Fri, May 03, 2013 at 12:47:43AM -0400, kosaki.motoh...@gmail.com wrote:
> From: KOSAKI Motohiro 
> 
> Currently glibc rt/tst-cpuclock2 test(*) sporadically fails because
> scheduler delta can be accounted twice from thread_group_cputimer()
> and account_group_exec_runtime().
> 
> Finally, clock_nanosleep() wakes up before an argument. This is posix
> violation. This issue was introduced by commit d670ec1317 (posix-cpu-timers:
> Cure SMP wobbles).
> 
> (*) 
> http://sourceware.org/git/?p=glibc.git;a=blob;f=rt/tst-cpuclock2.c;h=6752721717f959e89c0d692b3f1ee082d507eec2;hb=HEAD
> 
> Cc: Olivier Langlois 
> Cc: Thomas Gleixner 
> Cc: Frederic Weisbecker 
> Cc: Ingo Molnar 
> Signed-off-by: Peter Zijlstra 
> Signed-off-by: KOSAKI Motohiro 
[...]
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index 8fd709c..e56be4c 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -220,7 +220,7 @@ static int cpu_clock_sample(const clockid_t which_clock, 
> struct task_struct *p,
>   cpu->cpu = virt_ticks(p);
>   break;
>   case CPUCLOCK_SCHED:
> - cpu->sched = task_sched_runtime(p);
> + cpu->sched = task_sched_runtime(p, true);
>   break;
>   }
>   return 0;
> @@ -250,8 +250,13 @@ void thread_group_cputimer(struct task_struct *tsk, 
> struct task_cputime *times)
>* values through the TIMER_ABSTIME flag, therefore we have
>* to synchronize the timer to the clock every time we start
>* it.
> +  *
> +  * Do not add the current delta, because
> +  * account_group_exec_runtime() will also this delta and we
> +  * wouldn't want to double account time and get ahead of
> +  * ourselves.
>*/
> - thread_group_cputime(tsk, &sum);
> + thread_group_cputime(tsk, false, &sum);
>   raw_spin_lock_irqsave(&cputimer->lock, flags);

I wonder if we should move thread_group_cputime() inside this lock.
Otherwise we can miss some updates in-between.
--
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/

[PATCH 2/7] posix-cpu-timers: fix acounting delta_exec twice

2013-05-02 Thread kosaki . motohiro 
From: KOSAKI Motohiro 

Currently glibc rt/tst-cpuclock2 test(*) sporadically fails because
scheduler delta can be accounted twice from thread_group_cputimer()
and account_group_exec_runtime().

Finally, clock_nanosleep() wakes up before an argument. This is posix
violation. This issue was introduced by commit d670ec1317 (posix-cpu-timers:
Cure SMP wobbles).

(*) 
http://sourceware.org/git/?p=glibc.git;a=blob;f=rt/tst-cpuclock2.c;h=6752721717f959e89c0d692b3f1ee082d507eec2;hb=HEAD

Cc: Olivier Langlois 
Cc: Thomas Gleixner 
Cc: Frederic Weisbecker 
Cc: Ingo Molnar 
Signed-off-by: Peter Zijlstra 
Signed-off-by: KOSAKI Motohiro 
---
 fs/binfmt_elf.c   |2 +-
 fs/binfmt_elf_fdpic.c |2 +-
 include/linux/sched.h |4 ++--
 kernel/posix-cpu-timers.c |   15 ++-
 kernel/sched/core.c   |6 --
 kernel/sched/cputime.c|8 
 6 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 86af964..fea51e7 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1322,7 +1322,7 @@ static void fill_prstatus(struct elf_prstatus *prstatus,
 * This is the record for the group leader.  It shows the
 * group-wide total, not its individual thread total.
 */
-   thread_group_cputime(p, &cputime);
+   thread_group_cputime(p, true, &cputime);
cputime_to_timeval(cputime.utime, &prstatus->pr_utime);
cputime_to_timeval(cputime.stime, &prstatus->pr_stime);
} else {
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 9c13e02..ab5b508 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1371,7 +1371,7 @@ static void fill_prstatus(struct elf_prstatus *prstatus,
 * This is the record for the group leader.  It shows the
 * group-wide total, not its individual thread total.
 */
-   thread_group_cputime(p, &cputime);
+   thread_group_cputime(p, true, &cputime);
cputime_to_timeval(cputime.utime, &prstatus->pr_utime);
cputime_to_timeval(cputime.stime, &prstatus->pr_stime);
} else {
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e692a02..7863d4b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2002,7 +2002,7 @@ static inline void disable_sched_clock_irqtime(void) {}
 #endif
 
 extern unsigned long long
-task_sched_runtime(struct task_struct *task);
+task_sched_runtime(struct task_struct *task, bool add_delta);
 
 /* sched_exec is called by processes performing an exec */
 #ifdef CONFIG_SMP
@@ -2625,7 +2625,7 @@ static inline int spin_needbreak(spinlock_t *lock)
 /*
  * Thread group CPU time accounting.
  */
-void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times);
+void thread_group_cputime(struct task_struct *tsk, bool add_delta, struct 
task_cputime *times);
 void thread_group_cputimer(struct task_struct *tsk, struct task_cputime 
*times);
 
 static inline void thread_group_cputime_init(struct signal_struct *sig)
diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 8fd709c..e56be4c 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -220,7 +220,7 @@ static int cpu_clock_sample(const clockid_t which_clock, 
struct task_struct *p,
cpu->cpu = virt_ticks(p);
break;
case CPUCLOCK_SCHED:
-   cpu->sched = task_sched_runtime(p);
+   cpu->sched = task_sched_runtime(p, true);
break;
}
return 0;
@@ -250,8 +250,13 @@ void thread_group_cputimer(struct task_struct *tsk, struct 
task_cputime *times)
 * values through the TIMER_ABSTIME flag, therefore we have
 * to synchronize the timer to the clock every time we start
 * it.
+*
+* Do not add the current delta, because
+* account_group_exec_runtime() will also this delta and we
+* wouldn't want to double account time and get ahead of
+* ourselves.
 */
-   thread_group_cputime(tsk, &sum);
+   thread_group_cputime(tsk, false, &sum);
raw_spin_lock_irqsave(&cputimer->lock, flags);
cputimer->running = 1;
update_gt_cputime(&cputimer->cputime, &sum);
@@ -275,15 +280,15 @@ static int cpu_clock_sample_group(const clockid_t 
which_clock,
default:
return -EINVAL;
case CPUCLOCK_PROF:
-   thread_group_cputime(p, &cputime);
+   thread_group_cputime(p, true, &cputime);
cpu->cpu = cputime.utime + cputime.stime;
break;
case CPUCLOCK_VIRT:
-   thread_group_cputime(p, &cputime);
+   thread_group_cputime(p, true, &cputime);
cpu->cpu = cputime.ut