posix-timers: use try_to_del_timer_sync()

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 <[email protected]>

kernel/posix-timers.c renamed from kernel/posix-timers.c~2_USE +7 -28

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines · 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.

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

Quoted 9 lines · George Anzinger

Quoted 6 lines · 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.

Quoted 1 line

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.

Quoted 4 lines · Oleg Nesterov

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?

If this currently works, please leave it alone.

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.)

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

Quoted 8 lines · George Anzinger

Quoted 3 lines · Oleg Nesterov

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.

Quoted 1 line

If this currently works, please leave it alone.

Ok.

Quoted 4 lines

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

Quoted 2 lines

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

I agree completely.

Review

Quoted 12 lines

+ * 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.
+ */

(quoted diff) +5 -0

+#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?