[PATCH] introduce setup_timer() helper

[PATCH] introduce setup_timer() helper

[Oleg Nesterov]

Every user of init_timer() also needs to initialize ->function and
->data fields. This patch adds a simple setup_timer() helper for that.

The schedule_timeout() is patched as an example of usage.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.14-rc1/include/linux/timer.h~4_SETUP	2005-09-17 18:57:30.000000000 +0400
+++ 2.6.14-rc1/include/linux/timer.h	2005-09-18 20:55:15.000000000 +0400
@@ -38,6 +38,15 @@ extern struct timer_base_s __init_timer_
 
 void fastcall init_timer(struct timer_list * timer);
 
+static inline void setup_timer(struct timer_list * timer,
+				void (*function)(unsigned long),
+				unsigned long data)
+{
+	timer->function = function;
+	timer->data = data;
+	init_timer(timer);
+}
+
 /***
  * timer_pending - is a timer pending?
  * @timer: the timer in question
--- 2.6.14-rc1/kernel/timer.c~4_SETUP	2005-09-17 18:57:30.000000000 +0400
+++ 2.6.14-rc1/kernel/timer.c	2005-09-18 20:59:43.000000000 +0400
@@ -1137,12 +1137,8 @@ fastcall signed long __sched schedule_ti
 
 	expire = timeout + jiffies;
 
-	init_timer(&timer);
-	timer.expires = expire;
-	timer.data = (unsigned long) current;
-	timer.function = process_timeout;
-
-	add_timer(&timer);
+	setup_timer(&timer, process_timeout, (unsigned long)current);
+	__mod_timer(&timer, expire);
 	schedule();
 	del_singleshot_timer_sync(&timer);

Re: [PATCH] introduce setup_timer() helper

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

> 				unsigned long data)
> +{
> +	timer->function = function;
> +	timer->data = data;
> +	init_timer(timer);
> +}

are you sure you want to do this in this order???
I'd expect the init_timer to be first...


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] introduce setup_timer() helper

[Arjan van de Ven]

On Sun, Sep 18, 2005 at 07:51:20PM +0400, Oleg Nesterov wrote:
> Arjan van de Ven wrote:
> > 
> > >                               unsigned long data)
> > > +{
> > > +     timer->function = function;
> > > +     timer->data = data;
> > > +     init_timer(timer);
> > > +}
> > 
> > are you sure you want to do this in this order???
> > I'd expect the init_timer to be first...
> 
> I think it does not matter from correctness point of view.

right now.. it probably doesn't.
However I think conceptually, touching a timer before init_timer() is just
wrong. For one... it would prevent init_timer() from being able to use
memset() on the timer. Which it doesn't today but it's the kind of thing
that you don't want to prevent happening in the future.

> 	setup_timer(timer, expr1(), expr2())
> 
> it is better to initialize ->func and ->data first, otherwise
> the compiler should save the results from expr{1,2}, then call
> init_timer(), then copy these results to *timer.

I don't see how that is different.... 

Re: [PATCH] introduce setup_timer() helper

[Oleg Nesterov]

Arjan van de Ven wrote:
> 
> >                               unsigned long data)
> > +{
> > +     timer->function = function;
> > +     timer->data = data;
> > +     init_timer(timer);
> > +}
> 
> are you sure you want to do this in this order???
> I'd expect the init_timer to be first...

I think it does not matter from correctness point of view.

But if we have:

	setup_timer(timer, expr1(), expr2())

it is better to initialize ->func and ->data first, otherwise
the compiler should save the results from expr{1,2}, then call
init_timer(), then copy these results to *timer.

Oleg.

Re: [PATCH] introduce setup_timer() helper

[Oleg Nesterov]

Arjan van de Ven wrote:
> 
> On Sun, Sep 18, 2005 at 07:51:20PM +0400, Oleg Nesterov wrote:
> >
> > I think it does not matter from correctness point of view.
> 
> right now.. it probably doesn't.
> However I think conceptually, touching a timer before init_timer() is just
> wrong.

It is indeed wrong outside timer.{h,c}, but setup_timer() is a
part of timers subsystem, so I hope it's ok.

> For one... it would prevent init_timer() from being able to use
> memset() on the timer. Which it doesn't today but it's the kind of thing
> that you don't want to prevent happening in the future.

Yes, in that case we will have to change setup_timer(). But I
doubt this will happen. init_timer() only needs to set timer's
->base, and to ensure the timer is not pending.

> 
> >       setup_timer(timer, expr1(), expr2())
> >
> > it is better to initialize ->func and ->data first, otherwise
> > the compiler should save the results from expr{1,2}, then call
> > init_timer(), then copy these results to *timer.
> 
> I don't see how that is different....

I think this can save a couple of cpu cycles. The init_timer()
is not inline, gcc can't reorder exprx() and init_timer() calls.

Ok, I do not want to persist very much, I can resend this patch.

Andrew, should I?

Oleg.

Re: [PATCH] introduce setup_timer() helper

[Andrew Morton]

Oleg Nesterov <oleg@tv-sign.ru> wrote:
>
> I think this can save a couple of cpu cycles. The init_timer()
>  is not inline, gcc can't reorder exprx() and init_timer() calls.
> 
>  Ok, I do not want to persist very much, I can resend this patch.
> 
>  Andrew, should I?

Try both, see which one generates the shorter code?

Re: [PATCH] introduce setup_timer() helper

[Oleg Nesterov]

Andrew Morton wrote:
> 
> Oleg Nesterov <oleg@tv-sign.ru> wrote:
> >
> > I think this can save a couple of cpu cycles. The init_timer()
> >  is not inline, gcc can't reorder exprx() and init_timer() calls.
> >
> >  Ok, I do not want to persist very much, I can resend this patch.
> >
> >  Andrew, should I?
> 
> Try both, see which one generates the shorter code?

The code:

	void *expr(void);

	void tst(struct timer_list *timer)
	{
		setup_timer(timer, expr(), 0);
	}

Asm output:

     1  tst:
     2          pushl   %ebp
     3          movl    %esp, %ebp
     4          pushl   %ebx
     5          movl    8(%ebp), %ebx
     6          call    expr
     7          movl    %eax, 16(%ebx)
     8          movl    %ebx, %eax
     9          movl    $0, 20(%ebx)
    10          call    init_timer
    11          popl    %ebx
    12          popl    %ebp
    13          ret

After the Arjan proposed change:

     1  tst:
     2          pushl   %ebp
     3          movl    %esp, %ebp
     4          subl    $8, %esp
     5          movl    %ebx, (%esp)
     6          movl    8(%ebp), %ebx
     7          movl    %esi, 4(%esp)
     8          call    expr
     9          movl    %eax, %esi
    10          movl    %ebx, %eax
    11          call    init_timer
    12          movl    %esi, 16(%ebx)
    13          movl    $0, 20(%ebx)
    14          movl    (%esp), %ebx
    15          movl    4(%esp), %esi
    16          movl    %ebp, %esp
    17          popl    %ebp
    18          ret

I don't think we'll see any difference in practice, but still...

Oleg.