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 <[email protected]>
--- 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);
> 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...
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....
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.
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.
Oleg Nesterov <[email protected]> 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?
Andrew Morton wrote:
>
> Oleg Nesterov <[email protected]> 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.