RFC: better timer interface

RFC: better timer interface

[Christoph Hellwig]

Hi all,

this series attempts to provide a "modern" timer interface where the
callback gets the timer_list structure as an argument so that it
can use container_of instead of having to cast to/from unsigned long
all the time (or even worse use function pointer casts, we have quite
a few of those as well).

For that it steals another bit from the cpu mask to add a modern flag,
and if that flag is set the different new function prototype is used.
Last but least new helpers to initialize these modern timers are added.
Instead of having a larger number of initialization macros we simply
pass the timer flags to them.

[PATCH 1/9] timers: remove the fn and data arguments to call_timer_fn

[Christoph Hellwig]

And just move the dereferences inline, given that the timer gets
passed as an argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/time/timer.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 152a706ef8b8..c7978fcdbbea 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1240,8 +1240,7 @@ int del_timer_sync(struct timer_list *timer)
 EXPORT_SYMBOL(del_timer_sync);
 #endif
 
-static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
-			  unsigned long data)
+static void call_timer_fn(struct timer_list *timer)
 {
 	int count = preempt_count();
 
@@ -1265,14 +1264,14 @@ static void call_timer_fn(struct timer_list *timer, void (*fn)(unsigned long),
 	lock_map_acquire(&lockdep_map);
 
 	trace_timer_expire_entry(timer);
-	fn(data);
+	timer->function(timer->data);
 	trace_timer_expire_exit(timer);
 
 	lock_map_release(&lockdep_map);
 
 	if (count != preempt_count()) {
 		WARN_ONCE(1, "timer: %pF preempt leak: %08x -> %08x\n",
-			  fn, count, preempt_count());
+			  timer->function, count, preempt_count());
 		/*
 		 * Restore the preempt count. That gives us a decent
 		 * chance to survive and extract information. If the
@@ -1287,24 +1286,19 @@ static void expire_timers(struct timer_base *base, struct hlist_head *head)
 {
 	while (!hlist_empty(head)) {
 		struct timer_list *timer;
-		void (*fn)(unsigned long);
-		unsigned long data;
 
 		timer = hlist_entry(head->first, struct timer_list, entry);
 
 		base->running_timer = timer;
 		detach_timer(timer, true);
 
-		fn = timer->function;
-		data = timer->data;
-
 		if (timer->flags & TIMER_IRQSAFE) {
 			spin_unlock(&base->lock);
-			call_timer_fn(timer, fn, data);
+			call_timer_fn(timer);
 			spin_lock(&base->lock);
 		} else {
 			spin_unlock_irq(&base->lock);
-			call_timer_fn(timer, fn, data);
+			call_timer_fn(timer);
 			spin_lock_irq(&base->lock);
 		}
 	}
-- 
2.11.0

[PATCH 2/9] timers: provide a "modern" variant of timers

[Christoph Hellwig]

The new callback gets a pointer to the timer_list itself, which can
then be used to get the containing structure using container_of
instead of casting from and to unsigned long all the time.

The setup helpers take a flags argument instead of needing countless
variants.

Note: this further reduces space for the cpumask.  By the time we'll
need the additional cpumask space getting rid of the old-style timers
will hopefully be finished.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/timer.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 kernel/time/timer.c   | 24 ++++++++++++++----------
 2 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index e6789b8757d5..87afe52c8349 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -16,7 +16,10 @@ struct timer_list {
 	 */
 	struct hlist_node	entry;
 	unsigned long		expires;
-	void			(*function)(unsigned long);
+	union {
+		void		(*func)(struct timer_list *timer);
+		void		(*function)(unsigned long);
+	};
 	unsigned long		data;
 	u32			flags;
 
@@ -52,7 +55,8 @@ struct timer_list {
  * workqueue locking issues. It's not meant for executing random crap
  * with interrupts disabled. Abuse is monitored!
  */
-#define TIMER_CPUMASK		0x0003FFFF
+#define TIMER_CPUMASK		0x0001FFFF
+#define TIMER_MODERN		0x00020000
 #define TIMER_MIGRATING		0x00040000
 #define TIMER_BASEMASK		(TIMER_CPUMASK | TIMER_MIGRATING)
 #define TIMER_DEFERRABLE	0x00080000
@@ -63,6 +67,22 @@ struct timer_list {
 
 #define TIMER_TRACE_FLAGMASK	(TIMER_MIGRATING | TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
 
+#define INIT_TIMER(_func, _expires, _flags)		\
+{							\
+	.entry = { .next = TIMER_ENTRY_STATIC },	\
+	.func = (_func),				\
+	.expires = (_expires),				\
+	.flags = TIMER_MODERN | (_flags),		\
+	__TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \
+}
+
+#define DECLARE_TIMER(_name, _func, _expires, _flags)		\
+	struct timer_list _name = INIT_TIMER(_func, _expires, _flags)
+
+/*
+ * Don't use the macros below, use DECLARE_TIMER and INIT_TIMER with their
+ * improved callback signature above.
+ */
 #define __TIMER_INITIALIZER(_function, _expires, _data, _flags) { \
 		.entry = { .next = TIMER_ENTRY_STATIC },	\
 		.function = (_function),			\
@@ -126,6 +146,32 @@ static inline void init_timer_on_stack_key(struct timer_list *timer,
 	init_timer_on_stack_key((_timer), (_flags), NULL, NULL)
 #endif
 
+/**
+ * prepare_timer - initialize a timer before first use
+ * @timer:	timer structure to prepare
+ * @func:	callback to be called when the timer expires
+ * @flags	%TIMER_* flags that control timer behavior
+ *
+ * This function initializes a timer_list structure so that it can
+ * be used (by calling add_timer() or mod_timer()).
+ */
+static inline void prepare_timer(struct timer_list *timer,
+		void (*func)(struct timer_list *timer), u32 flags)
+{
+	__init_timer(timer, TIMER_MODERN | flags);
+	timer->func = func;
+}
+
+static inline void prepare_timer_on_stack(struct timer_list *timer,
+		void (*func)(struct timer_list *timer), u32 flags)
+{
+	__init_timer_on_stack(timer, TIMER_MODERN | flags);
+	timer->func = func;
+}
+
+/*
+ * Don't use - use prepare_timer above for new code instead.
+ */
 #define init_timer(timer)						\
 	__init_timer((timer), 0)
 #define init_timer_pinned(timer)					\
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index c7978fcdbbea..48d8450cfa5f 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -579,7 +579,7 @@ static struct debug_obj_descr timer_debug_descr;
 
 static void *timer_debug_hint(void *addr)
 {
-	return ((struct timer_list *) addr)->function;
+	return ((struct timer_list *) addr)->func;
 }
 
 static bool timer_is_static_object(void *addr)
@@ -930,7 +930,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
 	unsigned long clk = 0, flags;
 	int ret = 0;
 
-	BUG_ON(!timer->function);
+	BUG_ON(!timer->func && !timer->function);
 
 	/*
 	 * This is a common optimization triggered by the networking code - if
@@ -1064,12 +1064,12 @@ EXPORT_SYMBOL(mod_timer);
  * add_timer - start a timer
  * @timer: the timer to be added
  *
- * The kernel will do a ->function(->data) callback from the
- * timer interrupt at the ->expires point in the future. The
- * current time is 'jiffies'.
+ * The kernel will do a ->func (or ->function(->data) for legacy timers)
+ * callback from the timer interrupt at the ->expires point in the future.
+ * The current time is 'jiffies'.
  *
- * The timer's ->expires, ->function (and if the handler uses it, ->data)
- * fields must be set prior calling this function.
+ * The timer's ->expires, ->func / ->function (and if the handler uses it,
+ * ->data) fields must be set prior calling this function.
  *
  * Timers with an ->expires field in the past will be executed in the next
  * timer tick.
@@ -1093,7 +1093,8 @@ void add_timer_on(struct timer_list *timer, int cpu)
 	struct timer_base *new_base, *base;
 	unsigned long flags;
 
-	BUG_ON(timer_pending(timer) || !timer->function);
+	BUG_ON(timer_pending(timer));
+	BUG_ON(!timer->func && !timer->function);
 
 	new_base = get_timer_cpu_base(timer->flags, cpu);
 
@@ -1264,14 +1265,17 @@ static void call_timer_fn(struct timer_list *timer)
 	lock_map_acquire(&lockdep_map);
 
 	trace_timer_expire_entry(timer);
-	timer->function(timer->data);
+	if (timer->flags & TIMER_MODERN)
+		timer->func(timer);
+	else
+		timer->function(timer->data);
 	trace_timer_expire_exit(timer);
 
 	lock_map_release(&lockdep_map);
 
 	if (count != preempt_count()) {
 		WARN_ONCE(1, "timer: %pF preempt leak: %08x -> %08x\n",
-			  timer->function, count, preempt_count());
+			  timer->func, count, preempt_count());
 		/*
 		 * Restore the preempt count. That gives us a decent
 		 * chance to survive and extract information. If the
-- 
2.11.0

[PATCH 3/9] kthread: remove unused macros

[Christoph Hellwig]

KTHREAD_DELAYED_WORK_INIT and DEFINE_KTHREAD_DELAYED_WORK are unused
and are using a timer helper that's about to go away.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/kthread.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 4fec8b775895..acb6edb4b4b4 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -114,23 +114,12 @@ struct kthread_delayed_work {
 	.func = (fn),							\
 	}
 
-#define KTHREAD_DELAYED_WORK_INIT(dwork, fn) {				\
-	.work = KTHREAD_WORK_INIT((dwork).work, (fn)),			\
-	.timer = __TIMER_INITIALIZER(kthread_delayed_work_timer_fn,	\
-				     0, (unsigned long)&(dwork),	\
-				     TIMER_IRQSAFE),			\
-	}
-
 #define DEFINE_KTHREAD_WORKER(worker)					\
 	struct kthread_worker worker = KTHREAD_WORKER_INIT(worker)
 
 #define DEFINE_KTHREAD_WORK(work, fn)					\
 	struct kthread_work work = KTHREAD_WORK_INIT(work, fn)
 
-#define DEFINE_KTHREAD_DELAYED_WORK(dwork, fn)				\
-	struct kthread_delayed_work dwork =				\
-		KTHREAD_DELAYED_WORK_INIT(dwork, fn)
-
 /*
  * kthread_worker.lock needs its own lockdep class key when defined on
  * stack with lockdep enabled.  Use the following macros in such cases.
-- 
2.11.0

[PATCH 4/9] workqueue: switch to modern timers

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/workqueue.h                                | 16 ++++++----------
 kernel/workqueue.c                                       | 14 +++++++-------
 .../rcutorture/formal/srcu-cbmc/src/workqueues.h         |  2 +-
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index c102ef65cb64..59c889bf601e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -17,7 +17,7 @@ struct workqueue_struct;
 
 struct work_struct;
 typedef void (*work_func_t)(struct work_struct *work);
-void delayed_work_timer_fn(unsigned long __data);
+void delayed_work_timer_fn(struct timer_list *timer);
 
 /*
  * The first word is the work queue pointer and the flags rolled into
@@ -175,9 +175,8 @@ struct execute_work {
 
 #define __DELAYED_WORK_INITIALIZER(n, f, tflags) {			\
 	.work = __WORK_INITIALIZER((n).work, (f)),			\
-	.timer = __TIMER_INITIALIZER(delayed_work_timer_fn,		\
-				     0, (unsigned long)&(n),		\
-				     (tflags) | TIMER_IRQSAFE),		\
+	.timer = INIT_TIMER(delayed_work_timer_fn, 0,			\
+		(tflags) | TIMER_IRQSAFE), \
 	}
 
 #define DECLARE_WORK(n, f)						\
@@ -241,18 +240,15 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; }
 #define __INIT_DELAYED_WORK(_work, _func, _tflags)			\
 	do {								\
 		INIT_WORK(&(_work)->work, (_func));			\
-		__setup_timer(&(_work)->timer, delayed_work_timer_fn,	\
-			      (unsigned long)(_work),			\
+		prepare_timer(&(_work)->timer, delayed_work_timer_fn,	\
 			      (_tflags) | TIMER_IRQSAFE);		\
 	} while (0)
 
 #define __INIT_DELAYED_WORK_ONSTACK(_work, _func, _tflags)		\
 	do {								\
 		INIT_WORK_ONSTACK(&(_work)->work, (_func));		\
-		__setup_timer_on_stack(&(_work)->timer,			\
-				       delayed_work_timer_fn,		\
-				       (unsigned long)(_work),		\
-				       (_tflags) | TIMER_IRQSAFE);	\
+		prepare_timer_on_stack(&(_work)->timer, delayed_work_timer_fn, \
+			       (_tflags) | TIMER_IRQSAFE);		\
 	} while (0)
 
 #define INIT_DELAYED_WORK(_work, _func)					\
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c74bf39ef764..ba2cd509902f 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1492,9 +1492,10 @@ bool queue_work_on(int cpu, struct workqueue_struct *wq,
 }
 EXPORT_SYMBOL(queue_work_on);
 
-void delayed_work_timer_fn(unsigned long __data)
+void delayed_work_timer_fn(struct timer_list *timer)
 {
-	struct delayed_work *dwork = (struct delayed_work *)__data;
+	struct delayed_work *dwork =
+		container_of(timer, struct delayed_work, timer);
 
 	/* should have been called from irqsafe timer with irq already off */
 	__queue_work(dwork->cpu, dwork->wq, &dwork->work);
@@ -1508,8 +1509,7 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
 	struct work_struct *work = &dwork->work;
 
 	WARN_ON_ONCE(!wq);
-	WARN_ON_ONCE(timer->function != delayed_work_timer_fn ||
-		     timer->data != (unsigned long)dwork);
+	WARN_ON_ONCE(timer->func != delayed_work_timer_fn);
 	WARN_ON_ONCE(timer_pending(timer));
 	WARN_ON_ONCE(!list_empty(&work->entry));
 
@@ -5335,11 +5335,11 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq)	{ }
  */
 #ifdef CONFIG_WQ_WATCHDOG
 
-static void wq_watchdog_timer_fn(unsigned long data);
+static void wq_watchdog_timer_fn(struct timer_list *timer);
 
 static unsigned long wq_watchdog_thresh = 30;
 static struct timer_list wq_watchdog_timer =
-	TIMER_DEFERRED_INITIALIZER(wq_watchdog_timer_fn, 0, 0);
+	INIT_TIMER(wq_watchdog_timer_fn, 0, TIMER_DEFERRABLE);
 
 static unsigned long wq_watchdog_touched = INITIAL_JIFFIES;
 static DEFINE_PER_CPU(unsigned long, wq_watchdog_touched_cpu) = INITIAL_JIFFIES;
@@ -5353,7 +5353,7 @@ static void wq_watchdog_reset_touched(void)
 		per_cpu(wq_watchdog_touched_cpu, cpu) = jiffies;
 }
 
-static void wq_watchdog_timer_fn(unsigned long data)
+static void wq_watchdog_timer_fn(struct timer_list *timer)
 {
 	unsigned long thresh = READ_ONCE(wq_watchdog_thresh) * HZ;
 	bool lockup_detected = false;
diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/workqueues.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/workqueues.h
index e58c8dfd3e90..171982a32768 100644
--- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/workqueues.h
+++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/workqueues.h
@@ -13,7 +13,7 @@
 
 struct work_struct;
 typedef void (*work_func_t)(struct work_struct *work);
-void delayed_work_timer_fn(unsigned long __data);
+void delayed_work_timer_fn(struct timer_list *timer);
 
 struct work_struct {
 /*	atomic_long_t data; */
-- 
2.11.0

[PATCH 5/9] powerpc/numa: switch topology_timer to modern timer

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/powerpc/mm/numa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 371792e4418f..93a11227716b 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1437,7 +1437,7 @@ static void topology_schedule_update(void)
 	schedule_work(&topology_work);
 }
 
-static void topology_timer_fn(unsigned long ignored)
+static void topology_timer_fn(struct timer_list *timer)
 {
 	if (prrn_enabled && cpumask_weight(&cpu_associativity_changes_mask))
 		topology_schedule_update();
@@ -1447,8 +1447,7 @@ static void topology_timer_fn(unsigned long ignored)
 		reset_topology_timer();
 	}
 }
-static struct timer_list topology_timer =
-	TIMER_INITIALIZER(topology_timer_fn, 0, 0);
+static struct timer_list topology_timer = INIT_TIMER(topology_timer_fn, 0, 0);
 
 static void reset_topology_timer(void)
 {
-- 
2.11.0

[PATCH 6/9] s390: switch topology_timer to a modern timer

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/s390/kernel/topology.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index bb47c92476f0..4a0e867fca2b 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -289,7 +289,7 @@ void topology_schedule_update(void)
 	schedule_work(&topology_work);
 }
 
-static void topology_timer_fn(unsigned long ignored)
+static void topology_timer_fn(struct timer_list *timer)
 {
 	if (ptf(PTF_CHECK))
 		topology_schedule_update();
@@ -297,7 +297,7 @@ static void topology_timer_fn(unsigned long ignored)
 }
 
 static struct timer_list topology_timer =
-	TIMER_DEFERRED_INITIALIZER(topology_timer_fn, 0, 0);
+	INIT_TIMER(topology_timer_fn, 0, TIMER_DEFERRABLE);
 
 static atomic_t topology_poll = ATOMIC_INIT(0);
 
-- 
2.11.0

[PATCH 7/9] s390: switch lgr timer to a modern timer

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/s390/kernel/lgr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/lgr.c b/arch/s390/kernel/lgr.c
index ae7dff110054..147124c05f28 100644
--- a/arch/s390/kernel/lgr.c
+++ b/arch/s390/kernel/lgr.c
@@ -153,14 +153,14 @@ static void lgr_timer_set(void);
 /*
  * LGR timer callback
  */
-static void lgr_timer_fn(unsigned long ignored)
+static void lgr_timer_fn(struct timer_list *timer)
 {
 	lgr_info_log();
 	lgr_timer_set();
 }
 
 static struct timer_list lgr_timer =
-	TIMER_DEFERRED_INITIALIZER(lgr_timer_fn, 0, 0);
+	INIT_TIMER(lgr_timer_fn, 0, TIMER_DEFERRABLE);
 
 /*
  * Setup next LGR timer
-- 
2.11.0

[PATCH 8/9] tlclk: switch switchover_timer to a modern timer

[Christoph Hellwig]

And remove a superflous double-initialization.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/char/tlclk.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tlclk.c b/drivers/char/tlclk.c
index 572a51704e67..7144016da82c 100644
--- a/drivers/char/tlclk.c
+++ b/drivers/char/tlclk.c
@@ -184,10 +184,14 @@ static unsigned int telclk_interrupt;
 static int int_events;		/* Event that generate a interrupt */
 static int got_event;		/* if events processing have been done */
 
-static void switchover_timeout(unsigned long data);
-static struct timer_list switchover_timer =
-	TIMER_INITIALIZER(switchover_timeout , 0, 0);
-static unsigned long tlclk_timer_data;
+static void switchover_timeout(struct timer_list *timer);
+
+static struct switchover_timer {
+	struct timer_list timer;
+	unsigned long data;
+} switchover_timer = {
+	.timer = INIT_TIMER(switchover_timeout, 0, TIMER_DEFERRABLE),
+};
 
 static struct tlclk_alarms *alarm_events;
 
@@ -805,8 +809,6 @@ static int __init tlclk_init(void)
 		goto out3;
 	}
 
-	init_timer(&switchover_timer);
-
 	ret = misc_register(&tlclk_miscdev);
 	if (ret < 0) {
 		printk(KERN_ERR "tlclk: misc_register returns %d.\n", ret);
@@ -850,25 +852,26 @@ static void __exit tlclk_cleanup(void)
 	unregister_chrdev(tlclk_major, "telco_clock");
 
 	release_region(TLCLK_BASE, 8);
-	del_timer_sync(&switchover_timer);
+	del_timer_sync(&switchover_timer.timer);
 	kfree(alarm_events);
 
 }
 
-static void switchover_timeout(unsigned long data)
+static void switchover_timeout(struct timer_list *timer)
 {
-	unsigned long flags = *(unsigned long *) data;
+	struct switchover_timer *s =
+		container_of(timer, struct switchover_timer, timer);
 
-	if ((flags & 1)) {
-		if ((inb(TLCLK_REG1) & 0x08) != (flags & 0x08))
+	if ((s->data & 1)) {
+		if ((inb(TLCLK_REG1) & 0x08) != (s->data & 0x08))
 			alarm_events->switchover_primary++;
 	} else {
-		if ((inb(TLCLK_REG1) & 0x08) != (flags & 0x08))
+		if ((inb(TLCLK_REG1) & 0x08) != (s->data & 0x08))
 			alarm_events->switchover_secondary++;
 	}
 
 	/* Alarm processing is done, wake up read task */
-	del_timer(&switchover_timer);
+	del_timer(&switchover_timer.timer);
 	got_event = 1;
 	wake_up(&wq);
 }
@@ -920,10 +923,9 @@ static irqreturn_t tlclk_interrupt(int irq, void *dev_id)
 		alarm_events->pll_holdover++;
 
 		/* TIMEOUT in ~10ms */
-		switchover_timer.expires = jiffies + msecs_to_jiffies(10);
-		tlclk_timer_data = inb(TLCLK_REG1);
-		switchover_timer.data = (unsigned long) &tlclk_timer_data;
-		mod_timer(&switchover_timer, switchover_timer.expires);
+		switchover_timer.data = inb(TLCLK_REG1);
+		mod_timer(&switchover_timer.timer,
+				jiffies + msecs_to_jiffies(10));
 	} else {
 		got_event = 1;
 		wake_up(&wq);
-- 
2.11.0

[PATCH 9/9] timers: remove old timer initialization macros

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/timer.h | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index 87afe52c8349..9c6694d3f66a 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -80,35 +80,19 @@ struct timer_list {
 	struct timer_list _name = INIT_TIMER(_func, _expires, _flags)
 
 /*
- * Don't use the macros below, use DECLARE_TIMER and INIT_TIMER with their
+ * Don't use the macro below, use DECLARE_TIMER and INIT_TIMER with their
  * improved callback signature above.
  */
-#define __TIMER_INITIALIZER(_function, _expires, _data, _flags) { \
+#define DEFINE_TIMER(_name, _function, _expires, _data)		\
+	struct timer_list _name = {				\
 		.entry = { .next = TIMER_ENTRY_STATIC },	\
 		.function = (_function),			\
 		.expires = (_expires),				\
 		.data = (_data),				\
-		.flags = (_flags),				\
 		__TIMER_LOCKDEP_MAP_INITIALIZER(		\
 			__FILE__ ":" __stringify(__LINE__))	\
 	}
 
-#define TIMER_INITIALIZER(_function, _expires, _data)		\
-	__TIMER_INITIALIZER((_function), (_expires), (_data), 0)
-
-#define TIMER_PINNED_INITIALIZER(_function, _expires, _data)	\
-	__TIMER_INITIALIZER((_function), (_expires), (_data), TIMER_PINNED)
-
-#define TIMER_DEFERRED_INITIALIZER(_function, _expires, _data)	\
-	__TIMER_INITIALIZER((_function), (_expires), (_data), TIMER_DEFERRABLE)
-
-#define TIMER_PINNED_DEFERRED_INITIALIZER(_function, _expires, _data)	\
-	__TIMER_INITIALIZER((_function), (_expires), (_data), TIMER_DEFERRABLE | TIMER_PINNED)
-
-#define DEFINE_TIMER(_name, _function, _expires, _data)		\
-	struct timer_list _name =				\
-		TIMER_INITIALIZER(_function, _expires, _data)
-
 void init_timer_key(struct timer_list *timer, unsigned int flags,
 		    const char *name, struct lock_class_key *key);
 
-- 
2.11.0

Re: RFC: better timer interface

[Arnd Bergmann]

On Tue, May 16, 2017 at 1:48 PM, Christoph Hellwig <hch@lst.de> wrote:
> Hi all,
>
> this series attempts to provide a "modern" timer interface where the
> callback gets the timer_list structure as an argument so that it
> can use container_of instead of having to cast to/from unsigned long
> all the time (or even worse use function pointer casts, we have quite
> a few of those as well).

This looks really nice, but what is the long-term plan for the interface?
Do you expect that we will eventually change all 700+ users of timer_list
to the new type, or do we keep both variants around indefinitely to avoid
having to do mass-conversions?

If we are going to touch them all in the end, we might want to think
about other changes that could be useful here. The main one I have
in mind would be moving away from 'jiffies + timeout' as the interface,
and instead passing a relative number of milliseconds (or seconds)
into a mod_timer() variant. This is what most drivers want anyway,
and if we have both changes (callback argument and expiration
time) in place, we modernize the API one driver at a time with both
changes at once.

      Arnd

Re: RFC: better timer interface

[Christoph Hellwig]

On Tue, May 16, 2017 at 05:45:07PM +0200, Arnd Bergmann wrote:
> This looks really nice, but what is the long-term plan for the interface?
> Do you expect that we will eventually change all 700+ users of timer_list
> to the new type, or do we keep both variants around indefinitely to avoid
> having to do mass-conversions?

I think we should eventually move everyone over, but it might take
some time.

> If we are going to touch them all in the end, we might want to think
> about other changes that could be useful here. The main one I have
> in mind would be moving away from 'jiffies + timeout' as the interface,
> and instead passing a relative number of milliseconds (or seconds)
> into a mod_timer() variant. This is what most drivers want anyway,
> and if we have both changes (callback argument and expiration
> time) in place, we modernize the API one driver at a time with both
> changes at once.

Yes, that sounds useful to me as well.  As you said it's an independent
but somewhat related change.  I can add it to my series, but I'll
need a suggestions for a good and short name.  That already was the
hardest part for the setup side :)

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Randy Dunlap]

On 05/16/17 04:48, Christoph Hellwig wrote:

> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index e6789b8757d5..87afe52c8349 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
		\
> @@ -126,6 +146,32 @@ static inline void init_timer_on_stack_key(struct timer_list *timer,
>  	init_timer_on_stack_key((_timer), (_flags), NULL, NULL)
>  #endif
>  
> +/**
> + * prepare_timer - initialize a timer before first use
> + * @timer:	timer structure to prepare
> + * @func:	callback to be called when the timer expires
> + * @flags	%TIMER_* flags that control timer behavior

missing ':' on @flags:

> + *
> + * This function initializes a timer_list structure so that it can
> + * be used (by calling add_timer() or mod_timer()).
> + */
> +static inline void prepare_timer(struct timer_list *timer,
> +		void (*func)(struct timer_list *timer), u32 flags)
> +{



-- 
~Randy

Re: [PATCH 9/9] timers: remove old timer initialization macros

[Arnd Bergmann]

On Tue, May 16, 2017 at 1:48 PM, Christoph Hellwig <hch@lst.de> wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/timer.h | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index 87afe52c8349..9c6694d3f66a 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -80,35 +80,19 @@ struct timer_list {
>         struct timer_list _name = INIT_TIMER(_func, _expires, _flags)
>
>  /*
> - * Don't use the macros below, use DECLARE_TIMER and INIT_TIMER with their
> + * Don't use the macro below, use DECLARE_TIMER and INIT_TIMER with their
>   * improved callback signature above.
>   */
> -#define __TIMER_INITIALIZER(_function, _expires, _data, _flags) { \
> +#define DEFINE_TIMER(_name, _function, _expires, _data)                \
> +       struct timer_list _name = {                             \
>                 .entry = { .next = TIMER_ENTRY_STATIC },        \
>                 .function = (_function),                        \
>                 .expires = (_expires),                          \
>                 .data = (_data),                                \
> -               .flags = (_flags),                              \
>                 __TIMER_LOCKDEP_MAP_INITIALIZER(                \
>                         __FILE__ ":" __stringify(__LINE__))     \
>         }

Not sure what to do about it, but I notice that the '_expires'
argument is completely
bogus, I don't see any way it could be used in a meaningful way, and the only
user that passes anything other than zero is arch/mips/mti-malta/malta-display.c
and that seems to be unintentional.

      Arnd

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Arnd Bergmann]

On Tue, May 16, 2017 at 1:48 PM, Christoph Hellwig <hch@lst.de> wrote:

>         unsigned long           expires;
> -       void                    (*function)(unsigned long);
> +       union {
> +               void            (*func)(struct timer_list *timer);
> +               void            (*function)(unsigned long);
> +       };
...
> +#define INIT_TIMER(_func, _expires, _flags)            \
> +{                                                      \
> +       .entry = { .next = TIMER_ENTRY_STATIC },        \
> +       .func = (_func),                                \
> +       .expires = (_expires),                          \
> +       .flags = TIMER_MODERN | (_flags),               \
> +       __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \
> +}

If I remember correctly, this will fail with gcc-4.5 and earlier, which can't
use named initializers for anonymous unions. One of these two should
work, but they are both ugly:

a) don't use a named initializer for the union (a bit fragile)

 +#define INIT_TIMER(_func, _expires, _flags)            \
 +{                                                      \
 +       .entry = { .next = TIMER_ENTRY_STATIC },        \
 +       .expires = (_expires),                          \
 +       { .func = (_func) },                                \
 +       .flags = TIMER_MODERN | (_flags),               \
 +       __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \
 +}

b) give the union a name (breaks any reference to timer_list->func in C code):

 +       union {
 +               void            (*func)(struct timer_list *timer);
 +               void            (*function)(unsigned long);
 +       } u;
...
 +#define INIT_TIMER(_func, _expires, _flags)            \
 +{                                                      \
 +       .entry = { .next = TIMER_ENTRY_STATIC },        \
 +       .u.func = (_func),                                \
 +       .expires = (_expires),                          \
 +       .flags = TIMER_MODERN | (_flags),               \
 +       __TIMER_LOCKDEP_MAP_INITIALIZER(__FILE__ ":" __stringify(__LINE__)) \
 +}

> +/**
> + * prepare_timer - initialize a timer before first use
> + * @timer:     timer structure to prepare
> + * @func:      callback to be called when the timer expires
> + * @flags      %TIMER_* flags that control timer behavior
> + *
> + * This function initializes a timer_list structure so that it can
> + * be used (by calling add_timer() or mod_timer()).
> + */
> +static inline void prepare_timer(struct timer_list *timer,
> +               void (*func)(struct timer_list *timer), u32 flags)
> +{
> +       __init_timer(timer, TIMER_MODERN | flags);
> +       timer->func = func;
> +}
> +
> +static inline void prepare_timer_on_stack(struct timer_list *timer,
> +               void (*func)(struct timer_list *timer), u32 flags)
> +{
> +       __init_timer_on_stack(timer, TIMER_MODERN | flags);
> +       timer->func = func;
> +}

I fear this breaks lockdep output, which turns the name of
the timer into a string that gets printed later. It should work
when these are macros, or a macro wrapping an inline function
like __init_timer is.

      Arnd

Re: RFC: better timer interface

[Arnd Bergmann]

On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, May 16, 2017 at 05:45:07PM +0200, Arnd Bergmann wrote:
>> This looks really nice, but what is the long-term plan for the interface?
>> Do you expect that we will eventually change all 700+ users of timer_list
>> to the new type, or do we keep both variants around indefinitely to avoid
>> having to do mass-conversions?
>
> I think we should eventually move everyone over, but it might take
> some time.

Ok.

>> If we are going to touch them all in the end, we might want to think
>> about other changes that could be useful here. The main one I have
>> in mind would be moving away from 'jiffies + timeout' as the interface,
>> and instead passing a relative number of milliseconds (or seconds)
>> into a mod_timer() variant. This is what most drivers want anyway,
>> and if we have both changes (callback argument and expiration
>> time) in place, we modernize the API one driver at a time with both
>> changes at once.
>
> Yes, that sounds useful to me as well.  As you said it's an independent
> but somewhat related change.  I can add it to my series, but I'll
> need a suggestions for a good and short name.  That already was the
> hardest part for the setup side :)

If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
has), we could use one of

a) start_timer(struct timer_list *timer, unsigned long ms);
b) restart_timer(struct timer_list *timer, unsigned long ms);
c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
    mod_timer_sec(struct timer_list *timer, unsigned long sec);

The first is slightly shorter but conflicts with three files that use
the same name for a local function name. The third one fits
well with the existing interfaces and provides both millisecond
and second versions, I'd probably go with that.

We could consider even passing a default interval as another
argument to prepare_timer(), and using that in add_timer(),
but that would in those cases that have a constant interval
(maybe about half of the users from) and would be a bit surprising
to readers that are only familiar with the existing interfaces.

One final option would be a larger-scale replacement of
the API by mirroring the hrtimer style where possible while
staying compatible with the existing calls, e.g. timer_prepare(),
timer_add_expires(), timer_start(), ...

       Arnd

Re: [PATCH 3/9] kthread: remove unused macros

[Petr Mladek]

On Tue 2017-05-16 13:48:06, Christoph Hellwig wrote:
> KTHREAD_DELAYED_WORK_INIT and DEFINE_KTHREAD_DELAYED_WORK are unused
> and are using a timer helper that's about to go away.

A patch using this API is flying around, see
https://lkml.kernel.org/r/1476715742-14924-1-git-send-email-pmladek@suse.com
And I have one more, for hung_task.c, in the drawer.

I admit that I got sidetracked and did not push these conversions
last months. But the conversions are useful and I want to continue
or find a trainee that might continue.

I wanted to make your life easier, took inspiration from the
workqueues conversion and prepared the patch below. It is tested
with the above mentioned API user.

Please, let me known if you would prefer another approach.
I do not want to complicate development of the new timer API.


>From fcac2f124c0c4a5af0c803b4adef50cd2aef88e1 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 17 May 2017 14:00:19 +0200
Subject: [PATCH] kthread_worker: switch to modern timers

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/kthread.h | 11 ++++-------
 kernel/kthread.c        |  9 ++++-----
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 4fec8b775895..8c62c36eb32a 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -75,7 +75,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
  */
 struct kthread_work;
 typedef void (*kthread_work_func_t)(struct kthread_work *work);
-void kthread_delayed_work_timer_fn(unsigned long __data);
+void kthread_delayed_work_timer_fn(struct timer_list *timer);
 
 enum {
 	KTW_FREEZABLE		= 1 << 0,	/* freeze during suspend */
@@ -116,9 +116,8 @@ struct kthread_delayed_work {
 
 #define KTHREAD_DELAYED_WORK_INIT(dwork, fn) {				\
 	.work = KTHREAD_WORK_INIT((dwork).work, (fn)),			\
-	.timer = __TIMER_INITIALIZER(kthread_delayed_work_timer_fn,	\
-				     0, (unsigned long)&(dwork),	\
-				     TIMER_IRQSAFE),			\
+	.timer = INIT_TIMER(kthread_delayed_work_timer_fn,		\
+			    0, TIMER_IRQSAFE),				\
 	}
 
 #define DEFINE_KTHREAD_WORKER(worker)					\
@@ -163,9 +162,7 @@ extern void __kthread_init_worker(struct kthread_worker *worker,
 #define kthread_init_delayed_work(dwork, fn)				\
 	do {								\
 		kthread_init_work(&(dwork)->work, (fn));		\
-		__setup_timer(&(dwork)->timer,				\
-			      kthread_delayed_work_timer_fn,		\
-			      (unsigned long)(dwork),			\
+		prepare_timer(&(dwork)->timer, delayed_work_timer_fn,	\
 			      TIMER_IRQSAFE);				\
 	} while (0)
 
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 26db528c1d88..369e72ec7e48 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -797,15 +797,15 @@ bool kthread_queue_work(struct kthread_worker *worker,
 /**
  * kthread_delayed_work_timer_fn - callback that queues the associated kthread
  *	delayed work when the timer expires.
- * @__data: pointer to the data associated with the timer
+ * @timer: pointer to timer_list in the associated data structure
  *
  * The format of the function is defined by struct timer_list.
  * It should have been called from irqsafe timer with irq already off.
  */
-void kthread_delayed_work_timer_fn(unsigned long __data)
+void kthread_delayed_work_timer_fn(struct timer_list *timer)
 {
 	struct kthread_delayed_work *dwork =
-		(struct kthread_delayed_work *)__data;
+		container_of(timer, struct kthread_delayed_work, timer);
 	struct kthread_work *work = &dwork->work;
 	struct kthread_worker *worker = work->worker;
 
@@ -836,8 +836,7 @@ void __kthread_queue_delayed_work(struct kthread_worker *worker,
 	struct timer_list *timer = &dwork->timer;
 	struct kthread_work *work = &dwork->work;
 
-	WARN_ON_ONCE(timer->function != kthread_delayed_work_timer_fn ||
-		     timer->data != (unsigned long)dwork);
+	WARN_ON_ONCE(timer->func != kthread_delayed_work_timer_fn);
 
 	/*
 	 * If @delay is 0, queue @dwork->work immediately.  This is for
-- 
1.8.5.6

Re: [PATCH 3/9] kthread: remove unused macros

[Christoph Hellwig]

On Wed, May 17, 2017 at 02:09:52PM +0200, Petr Mladek wrote:
> On Tue 2017-05-16 13:48:06, Christoph Hellwig wrote:
> > KTHREAD_DELAYED_WORK_INIT and DEFINE_KTHREAD_DELAYED_WORK are unused
> > and are using a timer helper that's about to go away.
> 
> A patch using this API is flying around, see
> https://lkml.kernel.org/r/1476715742-14924-1-git-send-email-pmladek@suse.com
> And I have one more, for hung_task.c, in the drawer.
> 
> I admit that I got sidetracked and did not push these conversions
> last months. But the conversions are useful and I want to continue
> or find a trainee that might continue.
> 
> I wanted to make your life easier, took inspiration from the
> workqueues conversion and prepared the patch below. It is tested
> with the above mentioned API user.
> 
> Please, let me known if you would prefer another approach.
> I do not want to complicate development of the new timer API.

Thanks, I'll add your patch to the series.  I just have a tendency to
remove unused bits instead of trying to fix them up without being able
to test the result.

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Christoph Hellwig]

> b) give the union a name (breaks any reference to timer_list->func in C code):
> 
>  +       union {
>  +               void            (*func)(struct timer_list *timer);
>  +               void            (*function)(unsigned long);
>  +       } u;

I'll look into that, as it seems a lot safer, and places outside
the timer code shouldn't really touch it (although I bet they do,
so more fixes for this series..)

> I fear this breaks lockdep output, which turns the name of
> the timer into a string that gets printed later. It should work
> when these are macros, or a macro wrapping an inline function
> like __init_timer is.

Ok, I'll fix it up.  Although this macro mess isn't really readable
at all.

Re: [PATCH 9/9] timers: remove old timer initialization macros

[Christoph Hellwig]

On Tue, May 16, 2017 at 09:43:34PM +0200, Arnd Bergmann wrote:
> > - * Don't use the macros below, use DECLARE_TIMER and INIT_TIMER with their
> > + * Don't use the macro below, use DECLARE_TIMER and INIT_TIMER with their
> >   * improved callback signature above.
> >   */
> > -#define __TIMER_INITIALIZER(_function, _expires, _data, _flags) { \
> > +#define DEFINE_TIMER(_name, _function, _expires, _data)                \
> > +       struct timer_list _name = {                             \
> >                 .entry = { .next = TIMER_ENTRY_STATIC },        \
> >                 .function = (_function),                        \
> >                 .expires = (_expires),                          \
> >                 .data = (_data),                                \
> > -               .flags = (_flags),                              \
> >                 __TIMER_LOCKDEP_MAP_INITIALIZER(                \
> >                         __FILE__ ":" __stringify(__LINE__))     \
> >         }
> 
> Not sure what to do about it, but I notice that the '_expires'
> argument is completely
> bogus, I don't see any way it could be used in a meaningful way, and the only
> user that passes anything other than zero is arch/mips/mti-malta/malta-display.c
> and that seems to be unintentional.

DEFINE_TIMER is the old macro and instead of fixing it up I'll just
make sure the new DECLARE_TIMER gets rid of it.

Re: RFC: better timer interface

[Christoph Hellwig]

On Tue, May 16, 2017 at 10:26:39PM +0200, Arnd Bergmann wrote:
> If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
> has), we could use one of
> 
> a) start_timer(struct timer_list *timer, unsigned long ms);
> b) restart_timer(struct timer_list *timer, unsigned long ms);
> c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>     mod_timer_sec(struct timer_list *timer, unsigned long sec);
> 
> The first is slightly shorter but conflicts with three files that use
> the same name for a local function name. The third one fits
> well with the existing interfaces and provides both millisecond
> and second versions, I'd probably go with that.

Yeah, I'd take c) as well.  I'll give it a spin.

> We could consider even passing a default interval as another
> argument to prepare_timer(), and using that in add_timer(),
> but that would in those cases that have a constant interval
> (maybe about half of the users from) and would be a bit surprising
> to readers that are only familiar with the existing interfaces.

That seems rather ugly to me.

> One final option would be a larger-scale replacement of
> the API by mirroring the hrtimer style where possible while
> staying compatible with the existing calls, e.g. timer_prepare(),
> timer_add_expires(), timer_start(), ...

I'd chose timer_* for an entirely new API, but at this point this
seems a bit too much churn to me.

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Christoph Hellwig]

On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote:
> > b) give the union a name (breaks any reference to timer_list->func in C code):
> > 
> >  +       union {
> >  +               void            (*func)(struct timer_list *timer);
> >  +               void            (*function)(unsigned long);
> >  +       } u;
> 
> I'll look into that, as it seems a lot safer, and places outside
> the timer code shouldn't really touch it (although I bet they do,
> so more fixes for this series..)

Meh.  All the old init_timer users set function directly, so
I guess we need to use the other approach.

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Arnd Bergmann]

On Thu, May 18, 2017 at 10:41 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote:
>> > b) give the union a name (breaks any reference to timer_list->func in C code):
>> >
>> >  +       union {
>> >  +               void            (*func)(struct timer_list *timer);
>> >  +               void            (*function)(unsigned long);
>> >  +       } u;
>>
>> I'll look into that, as it seems a lot safer, and places outside
>> the timer code shouldn't really touch it (although I bet they do,
>> so more fixes for this series..)
>
> Meh.  All the old init_timer users set function directly, so
> I guess we need to use the other approach.

How expensive would it be to add another field to timer_list and
just have both pointers?

      Arnd

RE: [PATCH 2/9] timers: provide a "modern" variant of timers

[David Laight]

From: Christoph Hellwig
> Sent: 16 May 2017 12:48
>
> The new callback gets a pointer to the timer_list itself, which can
> then be used to get the containing structure using container_of
> instead of casting from and to unsigned long all the time.

What about sensible drivers that put some other value in the 'data'
field?

Perhaps it ought to have been 'void *data'.

Seems retrograde to be passing the address of the timer structure
(which, in principle, the callers no nothing about).

So I wouldn't call it 'modern', just different.

	David

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

['Christoph Hellwig']

On Fri, May 19, 2017 at 10:48:51AM +0000, David Laight wrote:
> From: Christoph Hellwig
> > Sent: 16 May 2017 12:48
> >
> > The new callback gets a pointer to the timer_list itself, which can
> > then be used to get the containing structure using container_of
> > instead of casting from and to unsigned long all the time.
> 
> What about sensible drivers that put some other value in the 'data'
> field?

They will add the equivalent of the data field into the containing
structure of the timer.  Just like we do for all other kernel interfaces
using the container_of patter, which includes just about every
primitive designed in the last 15 years.

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Christoph Hellwig]

On Thu, May 18, 2017 at 10:57:31AM +0200, Arnd Bergmann wrote:
> How expensive would it be to add another field to timer_list and
> just have both pointers?

That would add 4/8 bytes to every structure containing a timer,
so I'd rather avoid it if possible.  But one option might be to
inflict this onto users of outdated compilers and use the union
for modern ones.

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Arnd Bergmann]

On Sun, May 21, 2017 at 9:00 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, May 18, 2017 at 10:57:31AM +0200, Arnd Bergmann wrote:
>> How expensive would it be to add another field to timer_list and
>> just have both pointers?
>
> That would add 4/8 bytes to every structure containing a timer,
> so I'd rather avoid it if possible.

I didn't expect too many timers to be in allocated structures at the same
time on most systems, but I haven't researched this at all.

We should probably update the comment about the cacheline alignment
though: when most users embed the timer_list in some other structure,
it's not valid at all, and forcing timer_list to be cacheline aligned would
waste way more space than an extra field.

>  But one option might be to inflict this onto users of outdated compilers
> and use the union for modern ones.

Good idea, this sounds better than the alternatives at least. The
remaining users of those old compilers certainly don't care that much
about micro-optimizing the kernel anyway.

       Arnd

Re: RFC: better timer interface

[Thomas Gleixner]

On Tue, 16 May 2017, Arnd Bergmann wrote:
> On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
> > Yes, that sounds useful to me as well.  As you said it's an independent
> > but somewhat related change.  I can add it to my series, but I'll
> > need a suggestions for a good and short name.  That already was the
> > hardest part for the setup side :)
> 
> If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
> has), we could use one of
> 
> a) start_timer(struct timer_list *timer, unsigned long ms);
> b) restart_timer(struct timer_list *timer, unsigned long ms);
> c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>     mod_timer_sec(struct timer_list *timer, unsigned long sec);

Please make new functions prefixed with timer_ and get rid of that old
interface completely. It's horrible.
 
timer_init()
timer_start(timer, ms, abs)
timer_start_on(timer, ms, abs, cpu)
timer_cancel(timer, sync)

Is all what's required to make up a new milliseconds based interface.

We really do not need all that mod/restart/ whatever variants. Where is the
point of those?

Thanks,

	tglx

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Thomas Gleixner]

On Thu, 18 May 2017, Christoph Hellwig wrote:
> On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote:
> > > b) give the union a name (breaks any reference to timer_list->func in C code):
> > > 
> > >  +       union {
> > >  +               void            (*func)(struct timer_list *timer);
> > >  +               void            (*function)(unsigned long);
> > >  +       } u;
> > 
> > I'll look into that, as it seems a lot safer, and places outside
> > the timer code shouldn't really touch it (although I bet they do,
> > so more fixes for this series..)
> 
> Meh.  All the old init_timer users set function directly, so
> I guess we need to use the other approach.

There is another possibility. Create a coccinelle script which wraps all

      timer.function = f;
      timer->function = f;

assignements into a helper timer_set_function(timer, func) and ask Linus to
run it right before the next -rc. That handles everything in tree and the
few new instances in next can be addressed with patches sent to the
maintainers.

Thanks,

	tglx




      

Re: RFC: better timer interface

[Thomas Gleixner]

On Sun, 21 May 2017, Thomas Gleixner wrote:
> On Tue, 16 May 2017, Arnd Bergmann wrote:
> > On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
> > > Yes, that sounds useful to me as well.  As you said it's an independent
> > > but somewhat related change.  I can add it to my series, but I'll
> > > need a suggestions for a good and short name.  That already was the
> > > hardest part for the setup side :)
> > 
> > If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
> > has), we could use one of
> > 
> > a) start_timer(struct timer_list *timer, unsigned long ms);
> > b) restart_timer(struct timer_list *timer, unsigned long ms);
> > c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
> >     mod_timer_sec(struct timer_list *timer, unsigned long sec);
> 
> Please make new functions prefixed with timer_ and get rid of that old
> interface completely. It's horrible.
>  
> timer_init()
> timer_start(timer, ms, abs)

I'm not even sure, whether we need absolute timer wheel timers at
all, because most use cases are relative to now.

But it's easy enough to provide them. All we need for that is something
like

	unsigned long time_msec;	

which gets incremented every tick by the appropriate amount of
milliseconds.

Having that would also allow to replace all the

       end = jiffies + msec_to_jiffies(xxx);

       while (time_before(jiffies, end))
       	     ....

constructs with a milliseconds based machinery. So we can remove all
*_to_jiffies() interfaces over time.

Thanks,

	tglx

Re: [PATCH 2/9] timers: provide a "modern" variant of timers

[Al Viro]

On Sun, May 21, 2017 at 07:57:53PM +0200, Thomas Gleixner wrote:
> On Thu, 18 May 2017, Christoph Hellwig wrote:
> > On Thu, May 18, 2017 at 10:24:48AM +0200, Christoph Hellwig wrote:
> > > > b) give the union a name (breaks any reference to timer_list->func in C code):
> > > > 
> > > >  +       union {
> > > >  +               void            (*func)(struct timer_list *timer);
> > > >  +               void            (*function)(unsigned long);
> > > >  +       } u;
> > > 
> > > I'll look into that, as it seems a lot safer, and places outside
> > > the timer code shouldn't really touch it (although I bet they do,
> > > so more fixes for this series..)
> > 
> > Meh.  All the old init_timer users set function directly, so
> > I guess we need to use the other approach.
> 
> There is another possibility. Create a coccinelle script which wraps all
> 
>       timer.function = f;
>       timer->function = f;
> 
> assignements into a helper timer_set_function(timer, func) and ask Linus to
> run it right before the next -rc. That handles everything in tree and the
> few new instances in next can be addressed with patches sent to the
> maintainers.

FWIW, there was another possible approach - I toyed with that several years
ago, but it didn't go anywhere.  Namely, make timer.function take void *
*and* turn the setup part into setup(timer, callback, argument), verifying
that
	* callback(argument) will be acceptable expression for C typechecking
	* callback returns void
	* argument is a pointer type
then cast callback to void (*)(void *) and argument to void *.  That way
we get rid of any boilerplate in callbacks and get sane typechecking...

Re: RFC: better timer interface

[Arnd Bergmann]

On Sun, May 21, 2017 at 8:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sun, 21 May 2017, Thomas Gleixner wrote:
>> On Tue, 16 May 2017, Arnd Bergmann wrote:
>> > On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > > Yes, that sounds useful to me as well.  As you said it's an independent
>> > > but somewhat related change.  I can add it to my series, but I'll
>> > > need a suggestions for a good and short name.  That already was the
>> > > hardest part for the setup side :)
>> >
>> > If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
>> > has), we could use one of
>> >
>> > a) start_timer(struct timer_list *timer, unsigned long ms);
>> > b) restart_timer(struct timer_list *timer, unsigned long ms);
>> > c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>> >     mod_timer_sec(struct timer_list *timer, unsigned long sec);
>>
>> Please make new functions prefixed with timer_ and get rid of that old
>> interface completely. It's horrible.
>>
>> timer_init()
>> timer_start(timer, ms, abs)
>
> I'm not even sure, whether we need absolute timer wheel timers at
> all, because most use cases are relative to now.
>
> But it's easy enough to provide them. All we need for that is something
> like
>
>         unsigned long time_msec;
>
> which gets incremented every tick by the appropriate amount of
> milliseconds.
>
> Having that would also allow to replace all the
>
>        end = jiffies + msec_to_jiffies(xxx);
>
>        while (time_before(jiffies, end))
>              ....
>
> constructs with a milliseconds based machinery. So we can remove all
> *_to_jiffies() interfaces over time.

A lot of those users could probably just ktime_get()/ktime_before() here,
as they would by definition not be performance critical.

I don't see a way to just tk->tkr_mono.base but with a ktime_get_coarse()
we could just return the ktime_t of the last tick and not even need a seqlock
on 64-bit architectures, or have to introduce a new API.

        Arnd

Re: RFC: better timer interface

[Arnd Bergmann]

On Sun, May 21, 2017 at 7:13 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 16 May 2017, Arnd Bergmann wrote:
>> On Tue, May 16, 2017 at 5:51 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > Yes, that sounds useful to me as well.  As you said it's an independent
>> > but somewhat related change.  I can add it to my series, but I'll
>> > need a suggestions for a good and short name.  That already was the
>> > hardest part for the setup side :)
>>
>> If we keep the unusual *_timer() naming (rather than timer_*() as hrtimer
>> has), we could use one of
>>
>> a) start_timer(struct timer_list *timer, unsigned long ms);
>> b) restart_timer(struct timer_list *timer, unsigned long ms);
>> c) mod_timer_ms(struct timer_list *timer, unsigned long ms);
>>     mod_timer_sec(struct timer_list *timer, unsigned long sec);
>
> Please make new functions prefixed with timer_ and get rid of that old
> interface completely. It's horrible.
>
> timer_init()
> timer_start(timer, ms, abs)
> timer_start_on(timer, ms, abs, cpu)
> timer_cancel(timer, sync)
>
> Is all what's required to make up a new milliseconds based interface.
>
> We really do not need all that mod/restart/ whatever variants. Where is the
> point of those?

I agree, one of the above is good enough, if we do the large-scale API
replacement. Having both ms and sec variants would be for convenience
to avoid having lots of open-coded  '*MSEC_PER_SEC) multiplications.

We still need at least three variants of timer_init, for statically initialized
timers, dynamic allocation and on-stack allocation, as before.

For the 'abs' argument, I'd probably leave that out until we find code
that actually needs it and that can't use hrtimer as easily.

For timer_start_on(), that would be a replacement of add_timer_on(),
which only has seven callers today:

arch/x86/kernel/apic/x2apic_uv_x.c:             add_timer_on(timer, cpu);
drivers/tty/metag_da.c: add_timer_on(poll_timer, 0);
drivers/tty/mips_ejtag_fdc.c:
add_timer_on(&priv->poll_timer, dev->cpu);
drivers/tty/mips_ejtag_fdc.c:
add_timer_on(&priv->poll_timer, dev->cpu);
kernel/time/clocksource.c:      add_timer_on(&watchdog_timer, next_cpu);
kernel/time/clocksource.c:      add_timer_on(&watchdog_timer,
cpumask_first(cpu_online_mask));
kernel/workqueue.c:             add_timer_on(timer, cpu);

If hrtimer isn't already a better interface for those, we can probably
convert them all to the new API at once.

       Arnd

Re: RFC: better timer interface

[Thomas Gleixner]

On Mon, 22 May 2017, Arnd Bergmann wrote:
> On Sun, May 21, 2017 at 7:13 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> I agree, one of the above is good enough, if we do the large-scale API
> replacement. Having both ms and sec variants would be for convenience
> to avoid having lots of open-coded  '*MSEC_PER_SEC) multiplications.

Right, but that _sec variant is just a wrapper around the _ms functionality.

> We still need at least three variants of timer_init, for statically initialized
> timers, dynamic allocation and on-stack allocation, as before.

Again, that's variants. What I wanted to avoid is that mod_ restart_ start_
whatever distinction. It's pointless.

> For the 'abs' argument, I'd probably leave that out until we find code
> that actually needs it and that can't use hrtimer as easily.

> For timer_start_on(), that would be a replacement of add_timer_on(),
> which only has seven callers today:

> arch/x86/kernel/apic/x2apic_uv_x.c:             add_timer_on(timer, cpu);
> drivers/tty/metag_da.c: add_timer_on(poll_timer, 0);
> drivers/tty/mips_ejtag_fdc.c:
> add_timer_on(&priv->poll_timer, dev->cpu);
> drivers/tty/mips_ejtag_fdc.c:
> add_timer_on(&priv->poll_timer, dev->cpu);
> kernel/time/clocksource.c:      add_timer_on(&watchdog_timer, next_cpu);
> kernel/time/clocksource.c:      add_timer_on(&watchdog_timer,
> cpumask_first(cpu_online_mask));
> kernel/workqueue.c:             add_timer_on(timer, cpu);
> 
> If hrtimer isn't already a better interface for those, we can probably
> convert them all to the new API at once.

Some of them certainly can be converted. Though I'm starting to worry about
massive hrtimer usage, which can really become a burden because the expiry
functions run from hard interrupt context. We already have this horrible
construct of hrtimer -> tasklet -> real function, which is used to get the
hrtimer expiry out into softirq context.

We have similar issue in RT, where we need to move out everything which is
not hard irq safe to softirq context. I'm working on a solution there which
is less ugly than what we have now in RT. It's moving non irq safe timers
onto a different time base, e.g. MONOTONIC_SOFT, which merily wakes the
softirq when the first queued timer expires and let that one sort out the
expiry of those timers. It's WIP and should be available soon.

Thanks,

	tglx

Re: RFC: better timer interface

[Thomas Gleixner]

On Mon, 22 May 2017, Arnd Bergmann wrote:
> On Sun, May 21, 2017 at 8:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > But it's easy enough to provide them. All we need for that is something
> > like
> >
> >         unsigned long time_msec;
> >
> > which gets incremented every tick by the appropriate amount of
> > milliseconds.
> >
> > Having that would also allow to replace all the
> >
> >        end = jiffies + msec_to_jiffies(xxx);
> >
> >        while (time_before(jiffies, end))
> >              ....
> >
> > constructs with a milliseconds based machinery. So we can remove all
> > *_to_jiffies() interfaces over time.
> 
> A lot of those users could probably just ktime_get()/ktime_before() here,
> as they would by definition not be performance critical.

Right.

> I don't see a way to just tk->tkr_mono.base but with a ktime_get_coarse()
> we could just return the ktime_t of the last tick and not even need a seqlock
> on 64-bit architectures, or have to introduce a new API.

Yeah, that would be possible, but OTOH, for those loop thingies it probably
does not matter at all whether you have the overhead of ktime_get() or
not. We need to look at that stuff deeper.

Thanks,

	tglx

RE: RFC: better timer interface

[David Laight]

From: Thomas Gleixner
> Sent: 21 May 2017 19:15
...
> > timer_start(timer, ms, abs)
> 
> I'm not even sure, whether we need absolute timer wheel timers at
> all, because most use cases are relative to now.

Posix requires absolute timers for some userspace calls
(annoying because the code often wants relative).

OTOH how much conditional code is there for the 'abs' argument.
And is there any code that doesn't pass a constant?

Certainly worth a separate timer_start_abs(timer, wall_time)
function since you can't correctly map a wall_time timer
to a jiffies one.

	David

RE: RFC: better timer interface

[Thomas Gleixner]

On Tue, 23 May 2017, David Laight wrote:

> From: Thomas Gleixner
> > Sent: 21 May 2017 19:15
> ...
> > > timer_start(timer, ms, abs)
> > 
> > I'm not even sure, whether we need absolute timer wheel timers at
> > all, because most use cases are relative to now.
> 
> Posix requires absolute timers for some userspace calls
> (annoying because the code often wants relative).

Posix is completely irrelevant here. These timers are purely kernel
internal.

Thanks,

	tglx

RE: RFC: better timer interface

[David Laight]

From: Thomas Gleixner
> Sent: 23 May 2017 12:59
> On Tue, 23 May 2017, David Laight wrote:
> 
> > From: Thomas Gleixner
> > > Sent: 21 May 2017 19:15
> > ...
> > > > timer_start(timer, ms, abs)
> > >
> > > I'm not even sure, whether we need absolute timer wheel timers at
> > > all, because most use cases are relative to now.
> >
> > Posix requires absolute timers for some userspace calls
> > (annoying because the code often wants relative).
> 
> Posix is completely irrelevant here. These timers are purely kernel
> internal.

Somehow pthread_cond_timedwait() has to be implemented.
Doing so without kernel timers that use absolute 'wall clock' time is tricky.

	David

RE: RFC: better timer interface

[Thomas Gleixner]

On Tue, 23 May 2017, David Laight wrote:
> From: Thomas Gleixner
> > Sent: 23 May 2017 12:59
> > On Tue, 23 May 2017, David Laight wrote:
> > 
> > > From: Thomas Gleixner
> > > > Sent: 21 May 2017 19:15
> > > ...
> > > > > timer_start(timer, ms, abs)
> > > >
> > > > I'm not even sure, whether we need absolute timer wheel timers at
> > > > all, because most use cases are relative to now.
> > >
> > > Posix requires absolute timers for some userspace calls
> > > (annoying because the code often wants relative).
> > 
> > Posix is completely irrelevant here. These timers are purely kernel
> > internal.
> 
> Somehow pthread_cond_timedwait() has to be implemented.
> Doing so without kernel timers that use absolute 'wall clock' time is tricky.

Oh well. The timer wheel timers are NOT used to implement any posix
interface. That's all handled by hrtimers and they are not debated here.

So still nothing to see here.

Thanks,

	tglx