Messages in this thread | | | Date | Thu, 5 Feb 2009 22:30:59 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/2] timers: split process wide cpu clocks/timers |
| |
On 02/05, Peter Zijlstra wrote: > > Change the process wide cpu timers/clocks so that we: > > 1) don't mess up the kernel with too many threads, > 2) don't have a per-cpu allocation for each process, > 3) have no impact when not used. > > In order to accomplish this we're going to split it into two parts: > > - clocks; which can take all the time they want since they run > from user context -- ie. sys_clock_gettime(CLOCK_PROCESS_CPUTIME_ID) > > - timers; which need constant time sampling but since they're > explicity used, the user can pay the overhead. > > The clock readout will go back to a full sum of the thread group, while the > timers will run of a global 'clock' that only runs when needed, so only > programs that make use of the facility pay the price.
Ah, personally I think this is a very nice idea!
A couple of minor questions, before I try to read this patch more...
> static inline > -void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) > +void thread_group_cputimer(struct task_struct *tsk, struct task_cputime *times)
I know, it is silly to complain about the naming, but can't resist.
Now we have both thread_group_cputime() and thread_group_cputimer(). But it is not possible to distinguish them while reading the code. For example, looks like posix_cpu_timers_exit_group() needs thread_group_cputimer, not thread_group_cputime, no? But then we can hit the WARN_ON(!cputimer->running). Afaics.
Hmm... Can't fastpath_timer_check()->thread_group_cputimer() have the false warning too? Suppose we had the timer, then posix_cpu_timer_del() removes this timer, but task_cputime_zero(&sig->cputime_expires) still not true.
> +void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times) > +{ > + struct sighand_struct *sighand; > + struct signal_struct *sig; > + struct task_struct *t; > + > + *times = INIT_CPUTIME; > + > + rcu_read_lock(); > + sighand = rcu_dereference(tsk->sighand); > + if (!sighand) > + goto out; > + > + sig = tsk->signal;
I am afraid to be wrong, but it looks like we always call this function when we know we must have a valid ->sighand/siglock. Perhaps we do not need any check?
IOW, unless I missed something we should not just return if there is no ->sighand or ->signal, this just hides the problem while we should fix the caller.
> + * Enable the process wide cpu timer accounting. > + * > + * serialized using ->sighand->siglock > + */ > +static void start_process_timers(struct task_struct *tsk) > +{ > + tsk->signal->cputimer.running = 1; > + barrier(); > +} > + > +/* > + * Release the process wide timer accounting -- timer stops ticking when > + * nobody cares about it. > + * > + * serialized using ->sighand->siglock > + */ > +static void stop_process_timers(struct task_struct *tsk) > +{ > + tsk->signal->cputimer.running = 0; > + barrier(); > +}
Could you explain these barriers?
And, I am a bit lost... set_process_cpu_timer() does cpu_timer_sample_group(), but posix_cpu_timer_set() does cpu_clock_sample_group() in !CPUCLOCK_PERTHREAD case, could you confirm this is correct?
Oleg.
| |