Messages in this thread | | | Date | Tue, 23 Sep 2008 17:59:17 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2.6.27-rc5 re-resubmit] Fix itimer/many thread hang. |
| |
On 09/23, Ingo Molnar wrote: > > below is the delta patch i've applied to tip/timers/posixtimers - might > be easier to review for those who've seen v1 and who'd like to check the > changes. > ... > > +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;
Looks like task_delta_exec() forgets to unlock rq.
> +static inline void thread_group_cputime_account_exec_runtime( > + struct thread_group_cputime *tgtimes, > + unsigned long long ns) > +{ > + if (tgtimes->totals) { > + struct task_cputime *times; > + > + times = per_cpu_ptr(tgtimes->totals, get_cpu()); > + times->sum_exec_runtime += ns; > + put_cpu_no_resched(); > + } > +} > + > +#else /* CONFIG_SMP */ > + > +static inline void thread_group_cputime_account_user( > + struct thread_group_cputime *tgtimes, > + cputime_t cputime) > +{ > + tgtimes->totals->utime = cputime_add(tgtimes->totals->utime, cputime); > +}
Confused. Unless I misread the patch, tgtimes->totals can be NULL on UP too?
Do we really need the special version for !CONFIG_SMP ? per_cpu_ptr() just returns the pointer. Yes, get_cpu() disables preemption, but perhaps this is tolerable?
(of course, this can't explain the crash)
Oleg.
| |