[PATCH 08/13] hrtimer: remove data field

[PATCH 08/13] hrtimer: remove data field

[Roman Zippel]

The nanosleep cleanup allows to remove the data field of hrtimer. The
callback function can use container_of() to get it's own data. Since the
hrtimer structure is usually embedded in other structures, the code also
becomes a bit simpler.

Signed-off-by: Roman Zippel <zippel@linux-m68k.org>

---

 fs/exec.c               |    2 +-
 include/linux/hrtimer.h |    3 +--
 include/linux/sched.h   |    1 +
 include/linux/timer.h   |    3 ++-
 kernel/fork.c           |    2 +-
 kernel/hrtimer.c        |   11 ++++-------
 kernel/itimer.c         |   11 +++++------
 kernel/posix-timers.c   |    7 +++----
 8 files changed, 18 insertions(+), 22 deletions(-)

Index: linux-2.6-git/fs/exec.c
===================================================================
--- linux-2.6-git.orig/fs/exec.c	2006-02-12 19:52:50.000000000 +0100
+++ linux-2.6-git/fs/exec.c	2006-02-12 20:08:49.000000000 +0100
@@ -632,7 +632,7 @@ static int de_thread(struct task_struct 
 		 * synchronize with any firing (by calling del_timer_sync)
 		 * before we can safely let the old group leader die.
 		 */
-		sig->real_timer.data = current;
+		sig->tsk = current;
 		spin_unlock_irq(lock);
 		if (hrtimer_cancel(&sig->real_timer))
 			hrtimer_restart(&sig->real_timer);
Index: linux-2.6-git/include/linux/hrtimer.h
===================================================================
--- linux-2.6-git.orig/include/linux/hrtimer.h	2006-02-12 20:05:36.000000000 +0100
+++ linux-2.6-git/include/linux/hrtimer.h	2006-02-12 20:08:49.000000000 +0100
@@ -55,8 +55,7 @@ struct hrtimer_base;
 struct hrtimer {
 	struct rb_node		node;
 	ktime_t			expires;
-	int			(*function)(void *);
-	void			*data;
+	int			(*function)(struct hrtimer *);
 	struct hrtimer_base	*base;
 };
 
Index: linux-2.6-git/include/linux/sched.h
===================================================================
--- linux-2.6-git.orig/include/linux/sched.h	2006-02-12 19:52:50.000000000 +0100
+++ linux-2.6-git/include/linux/sched.h	2006-02-12 20:08:49.000000000 +0100
@@ -401,6 +401,7 @@ struct signal_struct {
 
 	/* ITIMER_REAL timer for the process */
 	struct hrtimer real_timer;
+	struct task_struct *tsk;
 	ktime_t it_real_incr;
 
 	/* ITIMER_PROF and ITIMER_VIRTUAL timers for the process */
Index: linux-2.6-git/include/linux/timer.h
===================================================================
--- linux-2.6-git.orig/include/linux/timer.h	2006-02-12 19:52:50.000000000 +0100
+++ linux-2.6-git/include/linux/timer.h	2006-02-12 20:08:49.000000000 +0100
@@ -96,6 +96,7 @@ static inline void add_timer(struct time
 
 extern void init_timers(void);
 extern void run_local_timers(void);
-extern int it_real_fn(void *);
+struct hrtimer;
+extern int it_real_fn(struct hrtimer *);
 
 #endif
Index: linux-2.6-git/kernel/fork.c
===================================================================
--- linux-2.6-git.orig/kernel/fork.c	2006-02-12 19:52:50.000000000 +0100
+++ linux-2.6-git/kernel/fork.c	2006-02-12 20:08:49.000000000 +0100
@@ -845,7 +845,7 @@ static inline int copy_signal(unsigned l
 	hrtimer_init(&sig->real_timer, CLOCK_MONOTONIC, HRTIMER_REL);
 	sig->it_real_incr.tv64 = 0;
 	sig->real_timer.function = it_real_fn;
-	sig->real_timer.data = tsk;
+	sig->tsk = tsk;
 
 	sig->it_virt_expires = cputime_zero;
 	sig->it_virt_incr = cputime_zero;
Index: linux-2.6-git/kernel/hrtimer.c
===================================================================
--- linux-2.6-git.orig/kernel/hrtimer.c	2006-02-12 20:05:36.000000000 +0100
+++ linux-2.6-git/kernel/hrtimer.c	2006-02-12 20:08:49.000000000 +0100
@@ -547,21 +547,19 @@ static inline void run_hrtimer_queue(str
 
 	while ((node = base->first)) {
 		struct hrtimer *timer;
-		int (*fn)(void *);
+		int (*fn)(struct hrtimer *);
 		int restart;
-		void *data;
 
 		timer = rb_entry(node, struct hrtimer, node);
 		if (now.tv64 <= timer->expires.tv64)
 			break;
 
 		fn = timer->function;
-		data = timer->data;
 		set_curr_timer(base, timer);
 		__remove_hrtimer(timer, base);
 		spin_unlock_irq(&base->lock);
 
-		restart = fn(data);
+		restart = fn(timer);
 
 		spin_lock_irq(&base->lock);
 
@@ -604,9 +602,9 @@ struct sleep_hrtimer {
 	int done;
 };
 
-static int nanosleep_wakeup(void *data)
+static int nanosleep_wakeup(struct hrtimer *timer)
 {
-	struct sleep_hrtimer *t = data;
+	struct sleep_hrtimer *t = container_of(timer, struct sleep_hrtimer, timer);
 
 	t->done = 1;
 	wake_up_process(t->task);
@@ -617,7 +615,6 @@ static int nanosleep_wakeup(void *data)
 static int __sched do_nanosleep(struct sleep_hrtimer *t, enum hrtimer_mode mode)
 {
 	t->timer.function = nanosleep_wakeup;
-	t->timer.data = t;
 	t->task = current;
 	t->done = 0;
 
Index: linux-2.6-git/kernel/itimer.c
===================================================================
--- linux-2.6-git.orig/kernel/itimer.c	2006-02-12 19:52:50.000000000 +0100
+++ linux-2.6-git/kernel/itimer.c	2006-02-12 20:08:49.000000000 +0100
@@ -128,15 +128,14 @@ asmlinkage long sys_getitimer(int which,
 /*
  * The timer is automagically restarted, when interval != 0
  */
-int it_real_fn(void *data)
+int it_real_fn(struct hrtimer *timer)
 {
-	struct task_struct *tsk = (struct task_struct *) data;
+	struct signal_struct *sig = container_of(timer, struct signal_struct, real_timer);
 
-	send_group_sig_info(SIGALRM, SEND_SIG_PRIV, tsk);
+	send_group_sig_info(SIGALRM, SEND_SIG_PRIV, sig->tsk);
 
-	if (tsk->signal->it_real_incr.tv64 != 0) {
-		hrtimer_forward(&tsk->signal->real_timer,
-			       tsk->signal->it_real_incr);
+	if (sig->it_real_incr.tv64 != 0) {
+		hrtimer_forward(&sig->real_timer, sig->it_real_incr);
 
 		return HRTIMER_RESTART;
 	}
Index: linux-2.6-git/kernel/posix-timers.c
===================================================================
--- linux-2.6-git.orig/kernel/posix-timers.c	2006-02-12 19:52:50.000000000 +0100
+++ linux-2.6-git/kernel/posix-timers.c	2006-02-12 20:08:49.000000000 +0100
@@ -144,7 +144,7 @@ static int common_timer_set(struct k_iti
 			    struct itimerspec *, struct itimerspec *);
 static int common_timer_del(struct k_itimer *timer);
 
-static int posix_timer_fn(void *data);
+static int posix_timer_fn(struct hrtimer *data);
 
 static struct k_itimer *lock_timer(timer_t timer_id, unsigned long *flags);
 
@@ -330,9 +330,9 @@ EXPORT_SYMBOL_GPL(posix_timer_event);
 
  * This code is for CLOCK_REALTIME* and CLOCK_MONOTONIC* timers.
  */
-static int posix_timer_fn(void *data)
+static int posix_timer_fn(struct hrtimer *data)
 {
-	struct k_itimer *timr = data;
+	struct k_itimer *timr = container_of(data, struct k_itimer, it.real.timer);
 	unsigned long flags;
 	int si_private = 0;
 	int ret = HRTIMER_NORESTART;
@@ -715,7 +715,6 @@ common_timer_set(struct k_itimer *timr, 
 
 	mode = flags & TIMER_ABSTIME ? HRTIMER_ABS : HRTIMER_REL;
 	hrtimer_init(&timr->it.real.timer, timr->it_clock, mode);
-	timr->it.real.timer.data = timr;
 	timr->it.real.timer.function = posix_timer_fn;
 
 	timer->expires = timespec_to_ktime(new_setting->it_value);

Re: [PATCH 08/13] hrtimer: remove data field

[Ingo Molnar]

* Roman Zippel <zippel@linux-m68k.org> wrote:

> The nanosleep cleanup allows to remove the data field of hrtimer. The 
> callback function can use container_of() to get it's own data. Since 
> the hrtimer structure is usually embedded in other structures, the 
> code also becomes a bit simpler.

i addressed this when you first raised this issue (back in the ktimers 
flamewars), and generally the feeling of people i asked was that doing 
the container_of() approach is less readable than an explicit 'data' 
field. It also deviates from struct timer_list, which we wanted to stay 
close to. Furthermore, for standalone hrtimers this creates the need to 
generate a wrapper structure. So i dont really like this change - but no 
strong feelings either way.

	Ingo

Re: [PATCH 08/13] hrtimer: remove data field

[Roman Zippel]

Hi,

On Mon, 13 Feb 2006, Ingo Molnar wrote:

> > The nanosleep cleanup allows to remove the data field of hrtimer. The 
> > callback function can use container_of() to get it's own data. Since 
> > the hrtimer structure is usually embedded in other structures, the 
> > code also becomes a bit simpler.
> 
> i addressed this when you first raised this issue (back in the ktimers 
> flamewars), and generally the feeling of people i asked was that doing 
> the container_of() approach is less readable than an explicit 'data' 
> field. It also deviates from struct timer_list, which we wanted to stay 
> close to. Furthermore, for standalone hrtimers this creates the need to 
> generate a wrapper structure. So i dont really like this change - but no 
> strong feelings either way.

With the complete size reduction struct hrtimer becomes 32 bytes on 32 
bits archs and so we can fit the basic hrtimer into one or two cache 
lines.
container_of() is becoming more and more common in the kernel, so I don't 
know who asked, it's not that difficult to use. I agree it makes simple 
test modules a bit more difficult, but so far the more common case is that 
this structure is embedded in other structures and container_of() creates 
simpler code. Additionally you get type checking for free, which you don't 
get with a void pointer.

bye, Roman

Re: [PATCH 08/13] hrtimer: remove data field

[Ingo Molnar]

* Roman Zippel <zippel@linux-m68k.org> wrote:

> On Mon, 13 Feb 2006, Ingo Molnar wrote:
> 
> > > The nanosleep cleanup allows to remove the data field of hrtimer. The 
> > > callback function can use container_of() to get it's own data. Since 
> > > the hrtimer structure is usually embedded in other structures, the 
> > > code also becomes a bit simpler.
> > 
> > i addressed this when you first raised this issue (back in the ktimers 
> > flamewars), and generally the feeling of people i asked was that doing 
> > the container_of() approach is less readable than an explicit 'data' 
> > field. It also deviates from struct timer_list, which we wanted to stay 
> > close to. Furthermore, for standalone hrtimers this creates the need to 
> > generate a wrapper structure. So i dont really like this change - but no 
> > strong feelings either way.
> 
> With the complete size reduction struct hrtimer becomes 32 bytes on 32 
> bits archs and so we can fit the basic hrtimer into one or two cache 
> lines. container_of() is becoming more and more common in the kernel, 
> so I don't know who asked, it's not that difficult to use. I agree it 
> makes simple test modules a bit more difficult, but so far the more 
> common case is that this structure is embedded in other structures and 
> container_of() creates simpler code. Additionally you get type 
> checking for free, which you don't get with a void pointer.

yeah, i agreed with you back then too. (in fact i raised doing the same 
for timer_list, which is embedded in other structs quite frequently too, 
but this thought didnt acquire much traction either.)

But clearly this is not a must-have item for 2.6.16.

	Ingo