Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1272156 > unrolled thread

[PATCH v3 01/22] timer: Allow to check when the timer callback has not finished yet

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 01/22] timer: Allow to check when the timer callback has not finished yet Petr Mladek <pmladek@suse.com> - 2015-11-18 14:30 +0100
    Re: [PATCH v3 01/22] timer: Allow to check when the timer callback  has not finished yet Thomas Gleixner <tglx@linutronix.de> - 2015-11-18 23:40 +0100
      Re: [PATCH v3 01/22] timer: Allow to check when the timer callback  has not finished yet Petr Mladek <pmladek@suse.com> - 2015-11-19 13:50 +0100

#1272156 — [PATCH v3 01/22] timer: Allow to check when the timer callback has not finished yet

timer_pending() checks whether the list of callbacks is empty.
Each callback is removed from the list before it is called,
see call_timer_fn() in __run_timers().

Sometimes we need to make sure that the callback has finished.
For example, if we want to free some resources that are accessed
by the callback.

For this purpose, this patch adds timer_active(). It checks both
the list of callbacks and the running_timer. It takes the base_lock
to see a consistent state.

I plan to use it to implement delayed works in kthread worker.
But I guess that it will have wider use. In fact, I wonder if
timer_pending() is misused in some situations.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/timer.h |  2 ++
 kernel/time/timer.c   | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index 61aa61dc410c..237b7c3e2b4e 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -165,6 +165,8 @@ static inline int timer_pending(const struct timer_list * timer)
 	return timer->entry.pprev != NULL;
 }
 
+extern int timer_active(struct timer_list *timer);
+
 extern void add_timer_on(struct timer_list *timer, int cpu);
 extern int del_timer(struct timer_list * timer);
 extern int mod_timer(struct timer_list *timer, unsigned long expires);
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index bbc5d1114583..1c16f3230771 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -778,6 +778,30 @@ static struct tvec_base *lock_timer_base(struct timer_list *timer,
 	}
 }
 
+/**
+ * timer_active - is a timer still in use?
+ * @timer: the timer in question
+ *
+ * timer_in_use() will tell whether the timer is pending or if the callback
+ * is curretly running.
+ *
+ * Use this function if you want to make sure that some resources
+ * will not longer get accessed by the timer callback. timer_pending()
+ * is not safe in this case.
+ */
+int timer_active(struct timer_list *timer)
+{
+	struct tvec_base *base;
+	unsigned long flags;
+	int ret;
+
+	base = lock_timer_base(timer, &flags);
+	ret = timer_pending(timer) || base->running_timer == timer;
+	spin_unlock_irqrestore(&base->lock, flags);
+
+	return ret;
+}
+
 static inline int
 __mod_timer(struct timer_list *timer, unsigned long expires,
 	    bool pending_only, int pinned)
-- 
1.8.5.6

--
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/

[toc] | [next] | [standalone]

#1272634 — Re: [PATCH v3 01/22] timer: Allow to check when the timer callback has not finished yet

On Wed, 18 Nov 2015, Petr Mladek wrote:
> timer_pending() checks whether the list of callbacks is empty.
> Each callback is removed from the list before it is called,
> see call_timer_fn() in __run_timers().
> 
> Sometimes we need to make sure that the callback has finished.
> For example, if we want to free some resources that are accessed
> by the callback.
> 
> For this purpose, this patch adds timer_active(). It checks both
> the list of callbacks and the running_timer. It takes the base_lock
> to see a consistent state.
> 
> I plan to use it to implement delayed works in kthread worker.
> But I guess that it will have wider use. In fact, I wonder if
> timer_pending() is misused in some situations.

Well. That's nice and good. But how will that new function solve
anything? After you drop the lock the state is not longer valid.
 
Thanks,

	tglx
--
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/

[toc] | [prev] | [next] | [standalone]

#1273112 — Re: [PATCH v3 01/22] timer: Allow to check when the timer callback has not finished yet

On Wed 2015-11-18 23:32:28, Thomas Gleixner wrote:
> On Wed, 18 Nov 2015, Petr Mladek wrote:
> > timer_pending() checks whether the list of callbacks is empty.
> > Each callback is removed from the list before it is called,
> > see call_timer_fn() in __run_timers().
> > 
> > Sometimes we need to make sure that the callback has finished.
> > For example, if we want to free some resources that are accessed
> > by the callback.
> > 
> > For this purpose, this patch adds timer_active(). It checks both
> > the list of callbacks and the running_timer. It takes the base_lock
> > to see a consistent state.
> > 
> > I plan to use it to implement delayed works in kthread worker.
> > But I guess that it will have wider use. In fact, I wonder if
> > timer_pending() is misused in some situations.
> 
> Well. That's nice and good. But how will that new function solve
> anything? After you drop the lock the state is not longer valid.

If we prevent anyone from setting up the timer and timer_pending()
returns false, we are sure that the timer will stay as is.

For example, I use it in the function try_to_cancel_kthread_work().
Any manipulation with the timer is protected by worker->lock.
If the timer is not pending but still active, I have to drop
the lock and busy wait for the timer callback. See
http://thread.gmane.org/gmane.linux.kernel.mm/141493/focus=141501


Also I wonder if the following usage in
drivers/infiniband/hw/nes/nes_cm.c is safe:

static int mini_cm_dealloc_core(struct nes_cm_core *cm_core)
{
	nes_debug(NES_DBG_CM, "De-Alloc CM Core (%p)\n", cm_core);

	if (!cm_core)
		return -EINVAL;

	barrier();

	if (timer_pending(&cm_core->tcp_timer))
		del_timer(&cm_core->tcp_timer);

	destroy_workqueue(cm_core->event_wq);
	destroy_workqueue(cm_core->disconn_wq);

We destroy the workqueue but the timer callback might still
be in progress and queue new work.


There are many more locations where I see the pattern:

      if (timer_pending())
		del_timer();
      clean_up_stuff();

IMHO, we should use:

      if (timer_active())
		del_timer_sync();
      /* really safe to free stuff */
      clean_up_stuff();

or just

   del_timer_sync();
   clean_up_stuff();


I wonder if timer_pending() is used in more racy scenarios. Or maybe,
I just miss something that makes it all safe.

Thanks,
Petr
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web