* [PATCH] Avoid process cputimer state oscillation
@ 2013-11-25 19:25 Olivier Langlois
2013-11-29 17:15 ` Olivier Langlois
0 siblings, 1 reply; 2+ messages in thread
From: Olivier Langlois @ 2013-11-25 19:25 UTC (permalink / raw)
To: tglx; +Cc: linux-kernel, fweisbec, kosaki.motohiro
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;
}
--
1.8.4.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] Avoid process cputimer state oscillation
2013-11-25 19:25 [PATCH] Avoid process cputimer state oscillation Olivier Langlois
@ 2013-11-29 17:15 ` Olivier Langlois
0 siblings, 0 replies; 2+ messages in thread
From: Olivier Langlois @ 2013-11-29 17:15 UTC (permalink / raw)
To: tglx; +Cc: linux-kernel, fweisbec, kosaki.motohiro
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;
> }
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-11-29 17:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-25 19:25 [PATCH] Avoid process cputimer state oscillation Olivier Langlois
2013-11-29 17:15 ` Olivier Langlois
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox