Re: + posix-timer-cleanup-common_timer_get.patch added to -mm tree

Re: + posix-timer-cleanup-common_timer_get.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-03-12 at 03:37 -0800, akpm@osdl.org wrote:
> The patch titled
> 
>      hrtimers: posix-timer: cleanup common_timer_get()
> 
> has been added to the -mm tree.  Its filename is
> 
>      posix-timer-cleanup-common_timer_get.patch
> 
> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> out what to do about this

The patch breaks the return values for SIGEV_NONE timers. Fix below

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Index: linux-2.6.16-updates/kernel/posix-timers.c
===================================================================
--- linux-2.6.16-updates.orig/kernel/posix-timers.c
+++ linux-2.6.16-updates/kernel/posix-timers.c
@@ -606,36 +606,41 @@ static struct k_itimer * lock_timer(time
 static void
 common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
 {
-	ktime_t now, remaining;
+	ktime_t now, remaining, iv;
 	struct hrtimer *timer = &timr->it.real.timer;
 
 	memset(cur_setting, 0, sizeof(struct itimerspec));
 
+	iv = timr->it.real.interval;
+
 	/* interval timer ? */
-	if (timr->it.real.interval.tv64 == 0) {
-		cur_setting->it_interval =
-			ktime_to_timespec(timr->it.real.interval);
-	} else if (!hrtimer_active(timer))
+	if (iv.tv64)
+		cur_setting->it_interval = ktime_to_timespec(iv);
+	else if (!hrtimer_active(timer) &&
+		 (timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE)
 		return;
 
 	now = timer->base->get_time();
 
 	/*
-	 * When a requeue is pending or this is a SIGEV_NONE timer
-	 * move the expiry time forward by intervals, so expiry is >
-	 * now.
+	 * When a requeue is pending or this is a SIGEV_NONE
+	 * timer move the expiry time forward by intervals, so
+	 * expiry is > now.
 	 */
-	if (timr->it_requeue_pending & REQUEUE_PENDING ||
-	    (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE) {
-		timr->it_overrun += hrtimer_forward(timer, now,
-						    timr->it.real.interval);
-	}
+	if (iv.tv64 && (timr->it_requeue_pending & REQUEUE_PENDING ||
+	    (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE))
+		timr->it_overrun += hrtimer_forward(timer, now, iv);
 
 	remaining = ktime_sub(timer->expires, now);
 	/* Return 0 only, when the timer is expired and not pending */
-	if (remaining.tv64 <= 0)
-		cur_setting->it_value.tv_nsec = 1;
-	else
+	if (remaining.tv64 <= 0) {
+		/*
+		 * A single shot SIGEV_NONE timer must return 0, when
+		 * it is expired !
+		 */
+		if ((timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE)
+			cur_setting->it_value.tv_nsec = 1;
+	} else
 		cur_setting->it_value = ktime_to_timespec(remaining);
 }
 

Re: + posix-timer-cleanup-common_timer_get.patch added to -mm tree

[Roman Zippel]

Hi,

On Wed, 15 Mar 2006, Thomas Gleixner wrote:

> -	if (timr->it.real.interval.tv64 == 0) {
> +	if (iv.tv64)

Just for the record: this wasn't my bug... :)

> +	if (iv.tv64 && (timr->it_requeue_pending & REQUEUE_PENDING ||
> +	    (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE))
> +		timr->it_overrun += hrtimer_forward(timer, now, iv);

The iv.tv64 test is only needed for the SIGEV_NONE case, so one could also 
move it to the end:

	if ((timr->it_requeue_pending & REQUEUE_PENDING) ||
	    ((timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE && iv.tv64))

OTOH since it_requeue_pending is unused for SIGEV_NONE, we could as well 
initialize it during timer_set:

	if (timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE)
		timr->it_requeue_pending == interval.tv64 ? REQUEUE_PENDING : 0;

and make the test even simpler.

> +	if (remaining.tv64 <= 0) {
> +		/*
> +		 * A single shot SIGEV_NONE timer must return 0, when
> +		 * it is expired !
> +		 */
> +		if ((timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE)
> +			cur_setting->it_value.tv_nsec = 1;

It's maybe practically not relevant, but theoretically a (iv.tv64) would 
be more correct and then we could also move it up to the initial (iv.tv64) 
test and leave the (remaining.tv64 > 0) case here.

bye, Roman