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

Groups > linux.kernel > #1642448 > unrolled thread

RFC: better timer interface

Back to article view | Back to linux.kernel

Contents

  RFC: better timer interface Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 3/9] kthread: remove unused macros Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
      Re: [PATCH 3/9] kthread: remove unused macros Petr Mladek <pmladek@suse.com> - 2017-05-17 14:20 +0200
        Re: [PATCH 3/9] kthread: remove unused macros Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
    [PATCH 8/9] tlclk: switch switchover_timer to a modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 6/9] s390: switch topology_timer to a modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 4/9] workqueue: switch to modern timers Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 7/9] s390: switch lgr timer to a modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
    [PATCH 9/9] timers: remove old timer initialization macros Christoph Hellwig <hch@lst.de> - 2017-05-16 13:50 +0200
      Re: [PATCH 9/9] timers: remove old timer initialization macros Arnd Bergmann <arnd@arndb.de> - 2017-05-16 21:50 +0200
        Re: [PATCH 9/9] timers: remove old timer initialization macros Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
    [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-16 14:00 +0200
      Re: [PATCH 2/9] timers: provide a "modern" variant of timers Randy Dunlap <rdunlap@infradead.org> - 2017-05-16 21:30 +0200
      Re: [PATCH 2/9] timers: provide a "modern" variant of timers Arnd Bergmann <arnd@arndb.de> - 2017-05-16 22:10 +0200
        Re: [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
          Re: [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-18 10:50 +0200
            Re: [PATCH 2/9] timers: provide a "modern" variant of timers Arnd Bergmann <arnd@arndb.de> - 2017-05-18 11:00 +0200
              Re: [PATCH 2/9] timers: provide a "modern" variant of timers Christoph Hellwig <hch@lst.de> - 2017-05-21 09:10 +0200
                Re: [PATCH 2/9] timers: provide a "modern" variant of timers Arnd Bergmann <arnd@arndb.de> - 2017-05-21 14:30 +0200
            Re: [PATCH 2/9] timers: provide a "modern" variant of timers Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 20:00 +0200
              Re: [PATCH 2/9] timers: provide a "modern" variant of timers Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-21 20:30 +0200
      RE: [PATCH 2/9] timers: provide a "modern" variant of timers David Laight <David.Laight@ACULAB.COM> - 2017-05-19 12:50 +0200
        Re: [PATCH 2/9] timers: provide a "modern" variant of timers 'Christoph Hellwig' <hch@lst.de> - 2017-05-21 09:00 +0200
    [PATCH 5/9] powerpc/numa: switch topology_timer to modern timer Christoph Hellwig <hch@lst.de> - 2017-05-16 14:00 +0200
    Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-16 17:50 +0200
      Re: RFC: better timer interface Christoph Hellwig <hch@lst.de> - 2017-05-16 18:00 +0200
        Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-16 22:30 +0200
          Re: RFC: better timer interface Christoph Hellwig <hch@lst.de> - 2017-05-18 10:30 +0200
          Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 19:20 +0200
            Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 20:20 +0200
              Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-22 13:30 +0200
                Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-22 21:30 +0200
              RE: RFC: better timer interface David Laight <David.Laight@ACULAB.COM> - 2017-05-23 13:40 +0200
                RE: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-23 14:00 +0200
                  RE: RFC: better timer interface David Laight <David.Laight@ACULAB.COM> - 2017-05-23 15:00 +0200
                    RE: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-23 15:10 +0200
            Re: RFC: better timer interface Arnd Bergmann <arnd@arndb.de> - 2017-05-22 15:40 +0200
              Re: RFC: better timer interface Thomas Gleixner <tglx@linutronix.de> - 2017-05-22 21:20 +0200

Page 1 of 2  [1] 2  Next page →

#1642448 — RFC: better timer interface

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.

[toc] | [next] | [standalone]

#1642449 — [PATCH 3/9] kthread: remove unused macros

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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.

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

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

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

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

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

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




      

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web