[PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[Oleg Nesterov]

sys_timer_settime/sys_timer_delete needs to delete k_itimer->real.timer
synchronously while holding ->it_lock, which is also locked in posix_timer_fn.

This patch removes timer_active/set_timer_inactive which plays with
timer_list's internals in favour of using try_to_del_timer_sync(),
which was introduced in the previous patch.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.12-rc4-mm2/kernel/posix-timers.c~2_USE	2005-04-30 18:05:29.000000000 +0400
+++ 2.6.12-rc4-mm2/kernel/posix-timers.c	2005-05-22 18:34:10.000000000 +0400
@@ -89,23 +89,6 @@ static struct idr posix_timers_id;
 static DEFINE_SPINLOCK(idr_lock);
 
 /*
- * Just because the timer is not in the timer list does NOT mean it is
- * inactive.  It could be in the "fire" routine getting a new expire time.
- */
-#define TIMER_INACTIVE 1
-
-#ifdef CONFIG_SMP
-# define timer_active(tmr) \
-		((tmr)->it.real.timer.entry.prev != (void *)TIMER_INACTIVE)
-# define set_timer_inactive(tmr) \
-		do { \
-			(tmr)->it.real.timer.entry.prev = (void *)TIMER_INACTIVE; \
-		} while (0)
-#else
-# define timer_active(tmr) BARFY	// error to use outside of SMP
-# define set_timer_inactive(tmr) do { } while (0)
-#endif
-/*
  * we assume that the new SIGEV_THREAD_ID shares no bits with the other
  * SIGEV values.  Here we put out an error if this assumption fails.
  */
@@ -226,7 +209,6 @@ static inline int common_timer_create(st
 	init_timer(&new_timer->it.real.timer);
 	new_timer->it.real.timer.data = (unsigned long) new_timer;
 	new_timer->it.real.timer.function = posix_timer_fn;
-	set_timer_inactive(new_timer);
 	return 0;
 }
 
@@ -480,7 +462,6 @@ static void posix_timer_fn(unsigned long
 	int do_notify = 1;
 
 	spin_lock_irqsave(&timr->it_lock, flags);
- 	set_timer_inactive(timr);
 	if (!list_empty(&timr->it.real.abs_timer_entry)) {
 		spin_lock(&abs_list.lock);
 		do {
@@ -983,8 +964,8 @@ common_timer_set(struct k_itimer *timr, 
 	 * careful here.  If smp we could be in the "fire" routine which will
 	 * be spinning as we hold the lock.  But this is ONLY an SMP issue.
 	 */
+	if (try_to_del_timer_sync(&timr->it.real.timer) < 0) {
 #ifdef CONFIG_SMP
-	if (timer_active(timr) && !del_timer(&timr->it.real.timer))
 		/*
 		 * It can only be active if on an other cpu.  Since
 		 * we have cleared the interval stuff above, it should
@@ -994,11 +975,9 @@ common_timer_set(struct k_itimer *timr, 
 		 * a "retry" exit status.
 		 */
 		return TIMER_RETRY;
-
-	set_timer_inactive(timr);
-#else
-	del_timer(&timr->it.real.timer);
 #endif
+	}
+
 	remove_from_abslist(timr);
 
 	timr->it_requeue_pending = (timr->it_requeue_pending + 2) & 
@@ -1083,8 +1062,9 @@ retry:
 static inline int common_timer_del(struct k_itimer *timer)
 {
 	timer->it.real.incr = 0;
+
+	if (try_to_del_timer_sync(&timer->it.real.timer) < 0) {
 #ifdef CONFIG_SMP
-	if (timer_active(timer) && !del_timer(&timer->it.real.timer))
 		/*
 		 * It can only be active if on an other cpu.  Since
 		 * we have cleared the interval stuff above, it should
@@ -1094,9 +1074,9 @@ static inline int common_timer_del(struc
 		 * a "retry" exit status.
 		 */
 		return TIMER_RETRY;
-#else
-	del_timer(&timer->it.real.timer);
 #endif
+	}
+
 	remove_from_abslist(timer);
 
 	return 0;

Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[George Anzinger]

Oleg Nesterov wrote:
> sys_timer_settime/sys_timer_delete needs to delete k_itimer->real.timer
> synchronously while holding ->it_lock, which is also locked in posix_timer_fn.
> 
> This patch removes timer_active/set_timer_inactive which plays with
> timer_list's internals in favour of using try_to_del_timer_sync(),
> which was introduced in the previous patch.

Is there a particular reason for this, like it does not work, for example, or 
are you just trying to clean up code?

The reason I ask is that this code, to the best of my knowledge, works and it 
also works if the timer is queued on another list prior to its being handled by 
posix_timer_fn, which is exactly what happens in the HRT code.

We also note that this code is the subject of a patch to the RT patch to cover 
the same issue when softirqs are run from threads and therefor allow 
posix_timer_fn to be preempted.  (That fix being mainly to expand usage from 
just SMP to SMP || SOFTIRQ_PREEMPT.)

If this currently works, please leave it alone.

George
-- 
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- 2.6.12-rc4-mm2/kernel/posix-timers.c~2_USE	2005-04-30 18:05:29.000000000 +0400
> +++ 2.6.12-rc4-mm2/kernel/posix-timers.c	2005-05-22 18:34:10.000000000 +0400
> @@ -89,23 +89,6 @@ static struct idr posix_timers_id;
>  static DEFINE_SPINLOCK(idr_lock);
>  
>  /*
> - * Just because the timer is not in the timer list does NOT mean it is
> - * inactive.  It could be in the "fire" routine getting a new expire time.
> - */
> -#define TIMER_INACTIVE 1
> -
> -#ifdef CONFIG_SMP
> -# define timer_active(tmr) \
> -		((tmr)->it.real.timer.entry.prev != (void *)TIMER_INACTIVE)
> -# define set_timer_inactive(tmr) \
> -		do { \
> -			(tmr)->it.real.timer.entry.prev = (void *)TIMER_INACTIVE; \
> -		} while (0)
> -#else
> -# define timer_active(tmr) BARFY	// error to use outside of SMP
> -# define set_timer_inactive(tmr) do { } while (0)
> -#endif
> -/*
>   * we assume that the new SIGEV_THREAD_ID shares no bits with the other
>   * SIGEV values.  Here we put out an error if this assumption fails.
>   */
> @@ -226,7 +209,6 @@ static inline int common_timer_create(st
>  	init_timer(&new_timer->it.real.timer);
>  	new_timer->it.real.timer.data = (unsigned long) new_timer;
>  	new_timer->it.real.timer.function = posix_timer_fn;
> -	set_timer_inactive(new_timer);
>  	return 0;
>  }
>  
> @@ -480,7 +462,6 @@ static void posix_timer_fn(unsigned long
>  	int do_notify = 1;
>  
>  	spin_lock_irqsave(&timr->it_lock, flags);
> - 	set_timer_inactive(timr);
>  	if (!list_empty(&timr->it.real.abs_timer_entry)) {
>  		spin_lock(&abs_list.lock);
>  		do {
> @@ -983,8 +964,8 @@ common_timer_set(struct k_itimer *timr, 
>  	 * careful here.  If smp we could be in the "fire" routine which will
>  	 * be spinning as we hold the lock.  But this is ONLY an SMP issue.
>  	 */
> +	if (try_to_del_timer_sync(&timr->it.real.timer) < 0) {
>  #ifdef CONFIG_SMP
> -	if (timer_active(timr) && !del_timer(&timr->it.real.timer))
>  		/*
>  		 * It can only be active if on an other cpu.  Since
>  		 * we have cleared the interval stuff above, it should
> @@ -994,11 +975,9 @@ common_timer_set(struct k_itimer *timr, 
>  		 * a "retry" exit status.
>  		 */
>  		return TIMER_RETRY;
> -
> -	set_timer_inactive(timr);
> -#else
> -	del_timer(&timr->it.real.timer);
>  #endif
> +	}
> +
>  	remove_from_abslist(timr);
>  
>  	timr->it_requeue_pending = (timr->it_requeue_pending + 2) & 
> @@ -1083,8 +1062,9 @@ retry:
>  static inline int common_timer_del(struct k_itimer *timer)
>  {
>  	timer->it.real.incr = 0;
> +
> +	if (try_to_del_timer_sync(&timer->it.real.timer) < 0) {
>  #ifdef CONFIG_SMP
> -	if (timer_active(timer) && !del_timer(&timer->it.real.timer))
>  		/*
>  		 * It can only be active if on an other cpu.  Since
>  		 * we have cleared the interval stuff above, it should
> @@ -1094,9 +1074,9 @@ static inline int common_timer_del(struc
>  		 * a "retry" exit status.
>  		 */
>  		return TIMER_RETRY;
> -#else
> -	del_timer(&timer->it.real.timer);
>  #endif
> +	}
> +
>  	remove_from_abslist(timer);
>  
>  	return 0;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[Andrew Morton]

George Anzinger <george@mvista.com> wrote:
>
> Oleg Nesterov wrote:
> > sys_timer_settime/sys_timer_delete needs to delete k_itimer->real.timer
> > synchronously while holding ->it_lock, which is also locked in posix_timer_fn.
> > 
> > This patch removes timer_active/set_timer_inactive which plays with
> > timer_list's internals in favour of using try_to_del_timer_sync(),
> > which was introduced in the previous patch.
> 
> Is there a particular reason for this, like it does not work, for example, or 
> are you just trying to clean up code?

The posix-timers code is poking about in the internals of the timer_list
implementation.  It shouldn't, and finding some way to fix that up is
desirable.

Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[George Anzinger]

Andrew Morton wrote:
> George Anzinger <george@mvista.com> wrote:
> 
>>Oleg Nesterov wrote:
>>
>>>sys_timer_settime/sys_timer_delete needs to delete k_itimer->real.timer
>>>synchronously while holding ->it_lock, which is also locked in posix_timer_fn.
>>>
>>>This patch removes timer_active/set_timer_inactive which plays with
>>>timer_list's internals in favour of using try_to_del_timer_sync(),
>>>which was introduced in the previous patch.
>>
>>Is there a particular reason for this, like it does not work, for example, or 
>>are you just trying to clean up code?
> 
> 
> The posix-timers code is poking about in the internals of the timer_list
> implementation.  It shouldn't, and finding some way to fix that up is
> desirable.

I see, and agree.  Could you give me a bit to work on this.  I would like to 
come up with something that is compatable with the HRT patch.  At this time I am 
waiting for John Stultz's time keeping changes to get in before pushing HRT, but 
I would still like it to get it into the kernel.



-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[Oleg Nesterov]

George Anzinger wrote:
>
> Oleg Nesterov wrote:
> >
> > This patch removes timer_active/set_timer_inactive which plays with
> > timer_list's internals in favour of using try_to_del_timer_sync(),
> > which was introduced in the previous patch.
>
> Is there a particular reason for this, like it does not work, for example, or
> are you just trying to clean up code?

It's a cleanup, I think that current code is correct.

> If this currently works, please leave it alone.

Ok.

> We also note that this code is the subject of a patch to the RT patch to cover
> the same issue when softirqs are run from threads and therefor allow
> posix_timer_fn to be preempted.  (That fix being mainly to expand usage from
> just SMP to SMP || SOFTIRQ_PREEMPT.)

I guess you are talking about this patch:
http://marc.theaimsgroup.com/?l=linux-kernel&m=111566867218576

> Also, I think that del_timer_sync and friends need to be turned on if soft_irq
> is preemptable.

I agree completely.

> + * For RT the timer call backs are preemptable.  This means that folks
> + * trying to delete timers may run into timers that are "active" for
> + * long times.  To help out with this we provide a wake up function to
> + * wake up a caller who wants waking when a timer clears the call back.
> + * This is the same sort of thing that the del_timer_sync does, but we
> + * need (in the HRT case) to cover two lists and not just the one.
> + */
> +#ifdef CONFIG_PREEMPT_SOFTIRQS
> +#include <linux/wait.h>
> +static DECLARE_WAIT_QUEUE_HEAD(timer_wake_queue);
> +#define wake_timer_waiters() wake_up(&timer_wake_queue)
> +#define wait_for_timer(timer) wait_event(timer_wake_queue, !timer_active(timer))

I'm not an expert at all, so I may be wrong, but I don't think
it's a good idea.

I think it is bad if __run_timers() could be preempted while
->running_timer != NULL. This will interact badly with __mod_timer,
del_timer_sync. I think that __run_timers() should do:

	set_running_timer(base, timer);
	preempt_disable();
	spin_unlock_irq(&base->lock);

	timer->function();

	set_running_timer(base, NULL);
	preempt_enable();
	spin_lock_irq(&base->lock);

What do you think?

Oleg.

Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[George Anzinger]

Oleg Nesterov wrote:
> George Anzinger wrote:
> 
>>Oleg Nesterov wrote:
>>
>>>This patch removes timer_active/set_timer_inactive which plays with
>>>timer_list's internals in favour of using try_to_del_timer_sync(),
>>>which was introduced in the previous patch.
>>
>>Is there a particular reason for this, like it does not work, for example, or
>>are you just trying to clean up code?
> 
> 
> It's a cleanup, I think that current code is correct.
> 
> 
>>If this currently works, please leave it alone.
> 
> 
> Ok.
> 
> 
>>We also note that this code is the subject of a patch to the RT patch to cover
>>the same issue when softirqs are run from threads and therefor allow
>>posix_timer_fn to be preempted.  (That fix being mainly to expand usage from
>>just SMP to SMP || SOFTIRQ_PREEMPT.)
> 
> 
> I guess you are talking about this patch:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=111566867218576
> 
> 
>>Also, I think that del_timer_sync and friends need to be turned on if soft_irq
>>is preemptable.
> 
> 
> I agree completely.
> 
> 
>>+ * For RT the timer call backs are preemptable.  This means that folks
>>+ * trying to delete timers may run into timers that are "active" for
>>+ * long times.  To help out with this we provide a wake up function to
>>+ * wake up a caller who wants waking when a timer clears the call back.
>>+ * This is the same sort of thing that the del_timer_sync does, but we
>>+ * need (in the HRT case) to cover two lists and not just the one.
>>+ */
>>+#ifdef CONFIG_PREEMPT_SOFTIRQS
>>+#include <linux/wait.h>
>>+static DECLARE_WAIT_QUEUE_HEAD(timer_wake_queue);
>>+#define wake_timer_waiters() wake_up(&timer_wake_queue)
>>+#define wait_for_timer(timer) wait_event(timer_wake_queue, !timer_active(timer))
> 
> 
> I'm not an expert at all, so I may be wrong, but I don't think
> it's a good idea.
> 
> I think it is bad if __run_timers() could be preempted while
> ->running_timer != NULL. This will interact badly with __mod_timer,
> del_timer_sync. I think that __run_timers() should do:
> 
> 	set_running_timer(base, timer);
> 	preempt_disable();
> 	spin_unlock_irq(&base->lock);
> 
> 	timer->function();
> 
> 	set_running_timer(base, NULL);
> 	preempt_enable();
> 	spin_lock_irq(&base->lock);
> 
> What do you think?

First, I think we need to get Ingo in the discussion. :)

Second, the RT patch has been running this way with little problems, save a 
REALLY intense test we (Monta Vista) have run that, from time to time, shows 
this to be a problem in the posix-timer code that is fixed by including 
SOFTIRQ_PREEMPT as well as SMP in the timer ifdefs.

One thing I do see there (in the RT patch) is a change to del_timer_sync to wait 
for the timer call back to complete rather than to loop...
> 
-- 
George Anzinger   george@mvista.com
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

[PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[George Anzinger]

With respect to this patch:

http://www.uwsg.indiana.edu/hypermail/linux/kernel/0505.2/1537.html

I have looked at various ways of doing this and have concluded that this is the 
right patch.
Oleg, could you resend?

Thanks
-- 
George Anzinger   george@mvista.com
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/

Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[Oleg Nesterov]

George Anzinger wrote:
> 
> With respect to this patch:
> 
> http://www.uwsg.indiana.edu/hypermail/linux/kernel/0505.2/1537.html
> 
> I have looked at various ways of doing this and have concluded that this is the
> right patch.

Great!

> Oleg, could you resend?

It's in 2.6.12-rc5-mm1 tree already.

Oleg.