Messages in this thread Patch in this message | | | Subject | Re: [PATCH] Avoid process cputimer state oscillation | From | Olivier Langlois <> | Date | Fri, 29 Nov 2013 12:15:40 -0500 |
| |
One more detail useful to note to appreciate this patch is that the call to posix_cpu_timer_schedule() from cpu_timer_fire() is done exceptionnaly when the timer signal generation fails.
The normal processing will call posix_cpu_timer_schedule() from a different context in do_schedule_next_timer() (posix-timers.c)
I believe this misunderstanding may partly explain the current state of the code.
On Mon, 2013-11-25 at 14:25 -0500, Olivier Langlois wrote: > When a periodic process timer is fired, a signal is generated. > Rearming the timer, if necessary, will be performed in a second step > when the signal will be delivered. > > Hence, checking the list of process timers list to decide to stop to cpu > timer right after generating the signal is premature and is leading the > cputimer to oscillate between the on/off states when a single process timer > is present. > > The following posix-cpu-timers module intrumentation > > In void thread_group_cputimer(struct task_s > raw_spin_lock_irqsave(&cputimer->lock, flags); > cputimer->running = 1; > update_gt_cputime(&cputimer->cputime, &sum); > + printk(KERN_DEBUG "cputimer started\n"); > } else > raw_spin_lock_irqsave(&cputimer->lock, flags); > *times = cputimer->cputime; > and in static void stop_process_timers(struct s > > raw_spin_lock_irqsave(&cputimer->lock, flags); > cputimer->running = 0; > + printk(KERN_DEBUG "cputimer stopped\n"); > raw_spin_unlock_irqrestore(&cputimer->lock, flags); > } > > with the small program: > > \#include <signal.h> > \#include <time.h> > \#include <stddef.h> > \#include <stdio.h> > \#include <string.h> > \#include <fcntl.h> > \#include <unistd.h> > > static void chew_cpu() > { > static volatile char buf[4096]; > for (size_t i = 0; i < 100; ++i) > for (size_t j = 0; j < sizeof buf; ++j) > buf[j] = 0xaa; > int nullfd = open("/dev/null", O_WRONLY); > for (size_t i = 0; i < 100; ++ i) > for (size_t j = 0; j < sizeof buf; ++j) > buf[j] = 0xbb; > write(nullfd, (char*)buf, sizeof buf); > close(nullfd); > } > > timer_t t; > volatile int sig_cnt; > > static void > sig_handler (int sig, siginfo_t *info, void *ctx) > { > if (sig_cnt >= 5) { > struct itimerspec it = {}; > timer_settime(t, 0, &it, NULL); > } > ++sig_cnt; > } > > int main( int argc, char *argv[] ) > { > clockid_t my_process_clock; > int e; > > struct sigaction sa = { .sa_sigaction = sig_handler, > .sa_flags = SA_SIGINFO }; > sigemptyset(&sa.sa_mask); > sigaction(SIGRTMIN, &sa, NULL); > > struct sigevent ev; > memset( &ev, 0, sizeof (ev)); > ev.sigev_notify = SIGEV_SIGNAL; > ev.sigev_signo = SIGRTMIN; > ev.sigev_value.sival_ptr = &ev; > > e = clock_getcpuclockid( 0, &my_process_clock); > if (e) { > printf("clock_getcpuclockid: (%d) %s\n", e, strerror(e)); > return 1; > } > if (timer_create(my_process_clock, &ev, &t) != 0) { > printf("timer_create: %m\n"); > return 1; > } > > struct itimerspec it; > it.it_value.tv_sec = 0; > it.it_value.tv_nsec = 100000; > it.it_interval.tv_sec = 0; > it.it_interval.tv_nsec = 100000; > > timer_settime(t,0,&it,NULL); > > while (sig_cnt < 5) > chew_cpu(); > > // process timer should stop > struct timespec req = { .tv_sec = 0, .tv_nsec = 400000 }; > nanosleep(&req,NULL); > > // restart timer. > sig_cnt = 0; > timer_settime(t,0,&it,NULL); > while (sig_cnt < 5) > chew_cpu(); > > timer_delete(t); > > return 0; > } > > demonstrate the issue. Here are the results before and after applying the patch: > > Before: > > [ 181.904571] cputimer started > [ 181.905013] cputimer stopped > [ 181.905027] cputimer started > [ 181.906009] cputimer stopped > [ 181.906016] cputimer started > [ 181.907008] cputimer stopped > [ 181.907014] cputimer started > [ 181.908008] cputimer stopped > [ 181.908013] cputimer started > [ 181.909008] cputimer stopped > [ 181.909020] cputimer started > [ 181.910007] cputimer stopped > [ 181.910050] cputimer started > [ 181.911011] cputimer stopped > [ 181.913619] cputimer started > [ 181.914007] cputimer stopped > [ 181.914015] cputimer started > [ 181.915006] cputimer stopped > [ 181.915011] cputimer started > [ 181.916006] cputimer stopped > [ 181.916010] cputimer started > [ 181.917008] cputimer stopped > [ 181.917058] cputimer started > [ 181.918009] cputimer stopped > [ 181.918029] cputimer started > [ 181.919006] cputimer stopped > [ 181.919010] cputimer started > [ 181.920006] cputimer stopped > > After: > > [ 859.722119] cputimer started > [ 859.730004] cputimer stopped > [ 859.731354] cputimer started > [ 859.739004] cputimer stopped > > Signed-off-by: Olivier Langlois <olivier@trillion01.com> > --- > kernel/posix-cpu-timers.c | 34 +++++++++++++++------------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > > diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c > index c7f31aa..972f5cb 100644 > --- a/kernel/posix-cpu-timers.c > +++ b/kernel/posix-cpu-timers.c > @@ -1049,8 +1049,6 @@ static void check_process_timers(struct task_struct *tsk, > sig->cputime_expires.prof_exp = expires_to_cputime(prof_expires); > sig->cputime_expires.virt_exp = expires_to_cputime(virt_expires); > sig->cputime_expires.sched_exp = sched_expires; > - if (task_cputime_zero(&sig->cputime_expires)) > - stop_process_timers(sig); > } > > /* > @@ -1158,34 +1156,32 @@ static inline int task_cputime_expired(const struct task_cputime *sample, > */ > static inline int fastpath_timer_check(struct task_struct *tsk) > { > - struct signal_struct *sig; > - cputime_t utime, stime; > + struct signal_struct *sig = tsk->signal; > > - task_cputime(tsk, &utime, &stime); > + if (sig->cputimer.running) { > + if (likely(!task_cputime_zero(&sig->cputime_expires))) { > + struct task_cputime group_sample; > + > + raw_spin_lock(&sig->cputimer.lock); > + group_sample = sig->cputimer.cputime; > + raw_spin_unlock(&sig->cputimer.lock); > + > + if (task_cputime_expired(&group_sample, &sig->cputime_expires)) > + return 1; > + } else > + stop_process_timers(sig); > + } > > if (!task_cputime_zero(&tsk->cputime_expires)) { > struct task_cputime task_sample = { > - .utime = utime, > - .stime = stime, > .sum_exec_runtime = tsk->se.sum_exec_runtime > }; > + task_cputime(tsk, &task_sample.utime, &task_sample.stime); > > if (task_cputime_expired(&task_sample, &tsk->cputime_expires)) > return 1; > } > > - sig = tsk->signal; > - if (sig->cputimer.running) { > - struct task_cputime group_sample; > - > - raw_spin_lock(&sig->cputimer.lock); > - group_sample = sig->cputimer.cputime; > - raw_spin_unlock(&sig->cputimer.lock); > - > - if (task_cputime_expired(&group_sample, &sig->cputime_expires)) > - return 1; > - } > - > return 0; > } >
| |