On Thu, 2015-08-06 at 16:18 +0200, Oleg Nesterov wrote:
> On 08/04, Jason Low wrote:
> >
> > @@ -973,13 +981,6 @@ static void check_process_timers(struct task_struct
> > *tsk,
> > virt_expires = check_timers_list(++timers, firing, utime);
> > sched_expires = check_timers_list(++timers, firing, sum_sched_runtime);
> >
> > - /*
> > - * Check for the special case process timers.
> > - */
> > - check_cpu_itimer(tsk, &sig->it[CPUCLOCK_PROF], &prof_expires, ptime,
> > - SIGPROF);
> > - check_cpu_itimer(tsk, &sig->it[CPUCLOCK_VIRT], &virt_expires, utime,
> > - SIGVTALRM);
> > soft = READ_ONCE(sig->rlim[RLIMIT_CPU].rlim_cur);
> > if (soft != RLIM_INFINITY) {
> > unsigned long psecs = cputime_to_secs(ptime);
> > @@ -1010,11 +1011,21 @@ static void check_process_timers(struct task_struct
> > *tsk,
> > }
> > }
> >
> > + /*
> > + * Check for the special case process timers.
> > + */
> > + check_cpu_itimer(tsk, &sig->it[CPUCLOCK_PROF], &prof_expires, ptime,
> > + SIGPROF);
> > + check_cpu_itimer(tsk, &sig->it[CPUCLOCK_VIRT], &virt_expires, utime,
> > + SIGVTALRM);
> > +
>
> Not sure I understand this part... looks wrong actually, please note
> that RLIMIT_CPU block above may need to update prof_expires _after_
> check_cpu_itimer(), or I am totally confused.
This change isn't critical to the patch, so we can delete this from the
patch. Though from my understanding, the purpose of prof_expires is to
collect the earliest prof expire time for when we update
"sig->cputime_expires.prof_exp". So I think it wouldn't matter which
order prof_expire gets updated (as long as check_timers_list() is called
first, since prof_expires gets directly assigned there).
> > if (READ_ONCE(sig->cputimer.running)) {
> > struct task_cputime group_sample;
> >
> > + /*
> > + * If another thread in the group is already checking
> > + * for the thread group cputimer, then we will skip that.
> > + */
> > + if (READ_ONCE(sig->cputimer.is_checking_timer))
> > + return 0;
> > +
>
> Cosmetic, I won't insist, but this is not symmetrical to ->running check,
>
> if (READ_ONCE(sig->cputimer.running) &&
> !READ_ONCE(sig->cputimer.is_checking_timer))
>
> looks a littke bit better to me.
I agree, I will be making that change.
--
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/
