* Re: + itimers-fix-itimer-many-thread-hang.patch added to -mm tree
[not found] <200809122353.m8CNrCmR011310@imap1.linux-foundation.org>
@ 2008-09-14 17:50 ` Oleg Nesterov
2008-09-14 18:39 ` Ingo Molnar
2008-09-15 17:49 ` Frank Mayhar
2008-09-17 8:21 ` KAMEZAWA Hiroyuki
1 sibling, 2 replies; 7+ messages in thread
From: Oleg Nesterov @ 2008-09-14 17:50 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel, fmayhar, adobriyan, mingo, roland, tglx
s/mm-commits/lkml/
(Frank, please don't forget to CC me ;)
On 09/12, Andrew Morton wrote:
>
> Subject: itimers: fix itimer/many thread hang
> From: Frank Mayhar <fmayhar@google.com>
Still can't read this patch thoroughly, but imho it looks good...
A couple of random nits.
> +static inline int fastpath_timer_check(struct task_struct *tsk,
> + struct signal_struct *sig)
> +{
> + struct task_cputime task_sample = {
> + .utime = tsk->utime,
> + .stime = tsk->stime,
> + .sum_exec_runtime = tsk->se.sum_exec_runtime
> + };
> + struct task_cputime group_sample;
> +
> + if (task_cputime_zero(&tsk->cputime_expires) &&
> + task_cputime_zero(&sig->cputime_expires))
> + return 0;
> + if (task_cputime_expired(&task_sample, &tsk->cputime_expires))
> + return 1;
> + thread_group_cputime(tsk, &group_sample);
> + return task_cputime_expired(&group_sample, &sig->cputime_expires);
> +}
Really minor nit. Suppose that task_cputime_zero(tsk) == T and
task_cputime_zero(sig) == F. In that case task_cputime_expired(&task_sample)
is not needed, perhaps it makes sense to reformat this function a bit
static inline int fastpath_timer_check(struct task_struct *tsk,
struct signal_struct *sig)
{
if (!task_cputime_zero(&tsk->cputime_expires)) {
struct task_cputime task_sample = {
.utime = tsk->utime,
.stime = tsk->stime,
.sum_exec_runtime = tsk->se.sum_exec_runtime
};
if (task_cputime_expired(&task_sample, &tsk->cputime_expires))
return 1;
}
if (!task_cputime_zero(&sig->cputime_expires)) {
struct task_cputime group_sample;
thread_group_cputime(tsk, &group_sample);
if (task_cputime_expired(&group_sample, &sig->cputime_expires))
return 1;
}
return 0;
}
this way it also looks more symmetrical.
> @@ -1323,30 +1358,29 @@ void run_posix_cpu_timers(struct task_st
> {
> LIST_HEAD(firing);
> struct k_itimer *timer, *next;
> + struct signal_struct *sig;
> + struct sighand_struct *sighand;
> + unsigned long flags;
>
> BUG_ON(!irqs_disabled());
>
> -#define UNEXPIRED(clock) \
> - (cputime_eq(tsk->it_##clock##_expires, cputime_zero) || \
> - cputime_lt(clock##_ticks(tsk), tsk->it_##clock##_expires))
> -
> - if (UNEXPIRED(prof) && UNEXPIRED(virt) &&
> - (tsk->it_sched_expires == 0 ||
> - tsk->se.sum_exec_runtime < tsk->it_sched_expires))
> - return;
> -
> -#undef UNEXPIRED
> -
> + /* Pick up tsk->signal and make sure it's valid. */
> + sig = tsk->signal;
> /*
> - * Double-check with locks held.
> + * The fast path checks that there are no expired thread or thread
> + * group timers. If that's so, just return. Also check that
> + * tsk->signal is non-NULL; this probably can't happen but cover the
^^^^^^^^^^^^^^^^^^^^^^^^^^
No, no, the comment is wrong. This certainly can happen if the timer
tick happens after exit_notify(). I meant that thread_group_cputime()
can't hit ->signal == NULL.
> + * possibility anyway.
> */
> - read_lock(&tasklist_lock);
> - if (likely(tsk->signal != NULL)) {
> - spin_lock(&tsk->sighand->siglock);
> -
> + if (unlikely(!sig) || !fastpath_timer_check(tsk, sig)) {
> + return;
> + }
I'd suggest to move the "if (!sig)" check into fastpath_timer_check(),
run_posix_cpu_timers() doesn't use sig, but this is matter a of taste.
OK, I did the patch on top of the change in fastpath_timer_check(),
just for illustration:
--- CPU-TIMERS-2.6.27-rc5/kernel/posix-cpu-timers.c~2_ftc_sig 2008-09-14 20:17:51.000000000 +0400
+++ CPU-TIMERS-2.6.27-rc5/kernel/posix-cpu-timers.c 2008-09-14 20:39:45.000000000 +0400
@@ -1323,15 +1323,18 @@ static inline int task_cputime_expired(c
* fastpath_timer_check - POSIX CPU timers fast path.
*
* @tsk: The task (thread) being checked.
- * @sig: The signal pointer for that task.
*
* If there are no timers set return false. Otherwise snapshot the task and
* thread group timers, then compare them with the corresponding expiration
# times. Returns true if a timer has expired, else returns false.
*/
-static inline int fastpath_timer_check(struct task_struct *tsk,
- struct signal_struct *sig)
+static inline int fastpath_timer_check(struct task_struct *tsk)
{
+ struct signal_struct *sig = tsk->signal;
+
+ if (unlikely(!sig))
+ return 0;
+
if (!task_cputime_zero(&tsk->cputime_expires)) {
struct task_cputime task_sample = {
.utime = tsk->utime,
@@ -1361,22 +1364,17 @@ void run_posix_cpu_timers(struct task_st
{
LIST_HEAD(firing);
struct k_itimer *timer, *next;
- struct signal_struct *sig;
struct sighand_struct *sighand;
unsigned long flags;
BUG_ON(!irqs_disabled());
-
- /* Pick up tsk->signal and make sure it's valid. */
- sig = tsk->signal;
/*
* The fast path checks that there are no expired thread or thread
- * group timers. If that's so, just return. Also check that
- * tsk->signal is non-NULL; this probably can't happen but cover the
- * possibility anyway.
+ * group timers. If that's so, just return.
*/
- if (unlikely(!sig) || !fastpath_timer_check(tsk, sig))
+ if (!fastpath_timer_check(tsk))
return;
+
sighand = lock_task_sighand(tsk, &flags);
if (likely(sighand)) {
/*
------------------------------------------------------------------------------
> + sighand = lock_task_sighand(tsk, &flags);
> + if (likely(sighand)) {
This is a bit misleading, lock_task_sighand() can't fail or we have a bug.
We already checked ->signal != NULL, and the task is current, we can use
spin_lock(&tsk->sighand->siglock).
To clarify, if lock_task_sighand() could fail, fastpath_timer_check()
is not safe. So I'd suggest the next patch:
--- CPU-TIMERS-2.6.27-rc5/kernel/posix-cpu-timers.c~3_rpct_siglock 2008-09-14 20:39:45.000000000 +0400
+++ CPU-TIMERS-2.6.27-rc5/kernel/posix-cpu-timers.c 2008-09-14 20:46:22.000000000 +0400
@@ -1364,8 +1364,6 @@ void run_posix_cpu_timers(struct task_st
{
LIST_HEAD(firing);
struct k_itimer *timer, *next;
- struct sighand_struct *sighand;
- unsigned long flags;
BUG_ON(!irqs_disabled());
/*
@@ -1375,26 +1373,24 @@ void run_posix_cpu_timers(struct task_st
if (!fastpath_timer_check(tsk))
return;
- sighand = lock_task_sighand(tsk, &flags);
- if (likely(sighand)) {
- /*
- * Here we take off tsk->signal->cpu_timers[N] and
- * tsk->cpu_timers[N] all the timers that are firing, and
- * put them on the firing list.
- */
- check_thread_timers(tsk, &firing);
- check_process_timers(tsk, &firing);
+ spin_lock(&tsk->sighand->siglock);
+ /*
+ * Here we take off tsk->signal->cpu_timers[N] and
+ * tsk->cpu_timers[N] all the timers that are firing, and
+ * put them on the firing list.
+ */
+ check_thread_timers(tsk, &firing);
+ check_process_timers(tsk, &firing);
- /*
- * We must release these locks before taking any timer's lock.
- * There is a potential race with timer deletion here, as the
- * siglock now protects our private firing list. We have set
- * the firing flag in each timer, so that a deletion attempt
- * that gets the timer lock before we do will give it up and
- * spin until we've taken care of that timer below.
- */
- }
- unlock_task_sighand(tsk, &flags);
+ /*
+ * We must release these locks before taking any timer's lock.
+ * There is a potential race with timer deletion here, as the
+ * siglock now protects our private firing list. We have set
+ * the firing flag in each timer, so that a deletion attempt
+ * that gets the timer lock before we do will give it up and
+ * spin until we've taken care of that timer below.
+ */
+ spin_unlock(&tsk->sighand->siglock);
/*
* Now that all the timers on our list have the firing flag,
-------------------------------------------------------------------------------
> +unsigned long long thread_group_sched_runtime(struct task_struct *p)
> +{
> + unsigned long flags;
> + u64 ns;
> + struct rq *rq;
> + struct task_cputime totals;
> +
> + rq = task_rq_lock(p, &flags);
> + thread_group_cputime(p, &totals);
> + ns = totals.sum_exec_runtime + task_delta_exec(p, rq);
> task_rq_unlock(rq, &flags);
Hmm. This is used by cpu_clock_sample_group_locked() which has already
called thread_group_cputime(). Yes, without task_rq_lock(), but
thread_group_cputime() is not "atomic" anyway. And please note that
thread_group_sched_runtime() is not that "group", we don't account
task_delta_exec() for other threads. Perhaps we can kill this function?
Or, at least, perhaps we can simplify this:
--- CPU-TIMERS-2.6.27-rc5/kernel/posix-cpu-timers.c~4_cpu_clock_sample_group 2008-09-14 20:46:22.000000000 +0400
+++ CPU-TIMERS-2.6.27-rc5/kernel/posix-cpu-timers.c 2008-09-14 21:29:20.000000000 +0400
@@ -299,7 +299,7 @@ static int cpu_clock_sample(const clocki
cpu->cpu = virt_ticks(p);
break;
case CPUCLOCK_SCHED:
- cpu->sched = task_sched_runtime(p);
+ cpu->sched = p->se.sum_exec_runtime + task_delta_exec(p);
break;
}
return 0;
@@ -327,7 +327,7 @@ static int cpu_clock_sample_group_locked
cpu->cpu = cputime.utime;
break;
case CPUCLOCK_SCHED:
- cpu->sched = thread_group_sched_runtime(p);
+ cpu->sched = cputime.sum_exec_runtime + task_delta_exec(p);
break;
}
return 0;
--- CPU-TIMERS-2.6.27-rc5/kernel/sched.c~4_cpu_clock_sample_group 2008-09-14 18:29:33.000000000 +0400
+++ CPU-TIMERS-2.6.27-rc5/kernel/sched.c 2008-09-14 21:29:30.000000000 +0400
@@ -4039,54 +4039,22 @@ EXPORT_PER_CPU_SYMBOL(kstat);
/*
* Return any ns on the sched_clock that have not yet been banked in
* @p in case that task is currently running.
- *
- * Called with task_rq_lock() held on @rq.
*/
-static unsigned long long task_delta_exec(struct task_struct *p, struct rq *rq)
+unsigned long long task_delta_exec(struct task_struct *p)
{
+ struct rq *rq;
+ unsigned long flags;
+ u64 ns = 0;
+
+ rq = task_rq_lock(p, &flags);
if (task_current(rq, p)) {
u64 delta_exec;
update_rq_clock(rq);
delta_exec = rq->clock - p->se.exec_start;
if ((s64)delta_exec > 0)
- return delta_exec;
+ ns = delta_exec;
}
- return 0;
-}
-
-/*
- * Return p->sum_exec_runtime plus any more ns on the sched_clock
- * that have not yet been banked in case the task is currently running.
- */
-unsigned long long task_sched_runtime(struct task_struct *p)
-{
- unsigned long flags;
- u64 ns;
- struct rq *rq;
-
- rq = task_rq_lock(p, &flags);
- ns = p->se.sum_exec_runtime + task_delta_exec(p, rq);
- task_rq_unlock(rq, &flags);
-
- return ns;
-}
-
-/*
- * Return sum_exec_runtime for the thread group plus any more ns on the
- * sched_clock that have not yet been banked in case the task is currently
- * running.
- */
-unsigned long long thread_group_sched_runtime(struct task_struct *p)
-{
- unsigned long flags;
- u64 ns;
- struct rq *rq;
- struct task_cputime totals;
-
- rq = task_rq_lock(p, &flags);
- thread_group_cputime(p, &totals);
- ns = totals.sum_exec_runtime + task_delta_exec(p, rq);
task_rq_unlock(rq, &flags);
return ns;
-------------------------------------------------------------------------------
BTW, with or without this patch cpu_clock_sample_group() doesn't need
->siglock, afaics.
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread