Re: [PATCH] Avoid process cputimer state oscillation

[Olivier Langlois]

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@xxxxxxxxxxxxxx>
> ---
>  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;
>  }
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/