2012-05-25 22:09:11

by Thomas Gleixner

Subject: [patch 2/4] timers: Consolidate base->next_timer update

Another bunch of mindlessly copied code. All callers of
internal_add_timer() except the recascading code updates
base->next_timer.

Move this into internal_add_timer() and let the cascading code call
__internal_add_timer().

Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/timer.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)

Index: tip/kernel/timer.c
===================================================================
--- tip.orig/kernel/timer.c
+++ tip/kernel/timer.c
@@ -330,7 +330,8 @@ void set_timer_slack(struct timer_list *
}
EXPORT_SYMBOL_GPL(set_timer_slack);

-static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
+static void
+__internal_add_timer(struct tvec_base *base, struct timer_list *timer)
{
unsigned long expires = timer->expires;
unsigned long idx = expires - base->timer_jiffies;
@@ -372,6 +373,17 @@ static void internal_add_timer(struct tv
list_add_tail(&timer->entry, vec);
}

+static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
+{
+ __internal_add_timer(base, timer);
+ /*
+ * Update base->next_timer if this is the earliest one.
+ */
+ if (time_before(timer->expires, base->next_timer) &&
+ !tbase_get_deferrable(timer->base))
+ base->next_timer = timer->expires;
+}
+
#ifdef CONFIG_TIMER_STATS
void __timer_stats_timer_set_start_info(struct timer_list *timer, void *addr)
{
@@ -757,9 +769,6 @@ __mod_timer(struct timer_list *timer, un
}

timer->expires = expires;
- if (time_before(timer->expires, base->next_timer) &&
- !tbase_get_deferrable(timer->base))
- base->next_timer = timer->expires;
internal_add_timer(base, timer);

out_unlock:
@@ -925,9 +934,6 @@ void add_timer_on(struct timer_list *tim
spin_lock_irqsave(&base->lock, flags);
timer_set_base(timer, base);
debug_activate(timer, timer->expires);
- if (time_before(timer->expires, base->next_timer) &&
- !tbase_get_deferrable(timer->base))
- base->next_timer = timer->expires;
internal_add_timer(base, timer);
/*
* Check whether the other CPU is idle and needs to be
@@ -1079,7 +1085,8 @@ static int cascade(struct tvec_base *bas
*/
list_for_each_entry_safe(timer, tmp, &tv_list, entry) {
BUG_ON(tbase_get_base(timer->base) != base);
- internal_add_timer(base, timer);
+ /* No accounting, while moving them */
+ __internal_add_timer(base, timer);
}

return index;
@@ -1706,9 +1713,6 @@ static void migrate_timer_list(struct tv
timer = list_first_entry(head, struct timer_list, entry);
detach_timer(timer, false);
timer_set_base(timer, new_base);
- if (time_before(timer->expires, new_base->next_timer) &&
- !tbase_get_deferrable(timer->base))
- new_base->next_timer = timer->expires;
internal_add_timer(new_base, timer);
}
}

2012-05-29 06:42:35

by Nikunj A Dadhania

Subject: Re: [patch 2/4] timers: Consolidate base->next_timer update

>
> -static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> +static void
> +__internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> {
> unsigned long expires = timer->expires;
> unsigned long idx = expires - base->timer_jiffies;
> @@ -372,6 +373,17 @@ static void internal_add_timer(struct tv
> list_add_tail(&timer->entry, vec);
> }
>
> +static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> +{
> + __internal_add_timer(base, timer);
> + /*
> + * Update base->next_timer if this is the earliest one.
> + */
> + if (time_before(timer->expires, base->next_timer) &&
> + !tbase_get_deferrable(timer->base))
> + base->next_timer = timer->expires;
> +}
> +
>
Shouldn't this be like this?

+ /*
+ * Update base->next_timer if this is the earliest one.
+ */
+ if (time_before(timer->expires, base->next_timer) &&
+ !tbase_get_deferrable(timer->base))
+ base->next_timer = timer->expires;
+ __internal_add_timer(base, timer);

As per the below code?

> #ifdef CONFIG_TIMER_STATS
> void __timer_stats_timer_set_start_info(struct timer_list *timer, void *addr)
> {
> @@ -757,9 +769,6 @@ __mod_timer(struct timer_list *timer, un
> }
>
> timer->expires = expires;
> - if (time_before(timer->expires, base->next_timer) &&
> - !tbase_get_deferrable(timer->base))
> - base->next_timer = timer->expires;
> internal_add_timer(base, timer);
>

2012-05-29 09:38:34

by Thomas Gleixner

Subject: Re: [patch 2/4] timers: Consolidate base->next_timer update

On Tue, 29 May 2012, Nikunj A Dadhania wrote:

> >
> > -static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> > +static void
> > +__internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> > {
> > unsigned long expires = timer->expires;
> > unsigned long idx = expires - base->timer_jiffies;
> > @@ -372,6 +373,17 @@ static void internal_add_timer(struct tv
> > list_add_tail(&timer->entry, vec);
> > }
> >
> > +static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> > +{
> > + __internal_add_timer(base, timer);
> > + /*
> > + * Update base->next_timer if this is the earliest one.
> > + */
> > + if (time_before(timer->expires, base->next_timer) &&
> > + !tbase_get_deferrable(timer->base))
> > + base->next_timer = timer->expires;
> > +}
> > +
> >
> Shouldn't this be like this?
>
> + /*
> + * Update base->next_timer if this is the earliest one.
> + */
> + if (time_before(timer->expires, base->next_timer) &&
> + !tbase_get_deferrable(timer->base))
> + base->next_timer = timer->expires;
> + __internal_add_timer(base, timer);
>
> As per the below code?

And why should this matter?

2012-05-29 10:36:22

by Nikunj A Dadhania

Subject: Re: [patch 2/4] timers: Consolidate base->next_timer update

On Tue, 29 May 2012 11:38:27 +0200 (CEST), Thomas Gleixner <[email protected]> wrote:
> On Tue, 29 May 2012, Nikunj A Dadhania wrote:
>
> > >
> > > -static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> > > +static void
> > > +__internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> > > {
> > > unsigned long expires = timer->expires;
> > > unsigned long idx = expires - base->timer_jiffies;
> > > @@ -372,6 +373,17 @@ static void internal_add_timer(struct tv
> > > list_add_tail(&timer->entry, vec);
> > > }
> > >
> > > +static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
> > > +{
> > > + __internal_add_timer(base, timer);
> > > + /*
> > > + * Update base->next_timer if this is the earliest one.
> > > + */
> > > + if (time_before(timer->expires, base->next_timer) &&
> > > + !tbase_get_deferrable(timer->base))
> > > + base->next_timer = timer->expires;
> > > +}
> > > +
> > >
> > Shouldn't this be like this?
> >
> > + /*
> > + * Update base->next_timer if this is the earliest one.
> > + */
> > + if (time_before(timer->expires, base->next_timer) &&
> > + !tbase_get_deferrable(timer->base))
> > + base->next_timer = timer->expires;
> > + __internal_add_timer(base, timer);
> >
> > As per the below code?
>
> And why should this matter?
>
Yes it does not matter, sorry for the noise.

Looking at the internal_add_timer(), there is no such dependency. I was
thinking that the base->next_timer is changed and would be used in
__internal_add_timer.

Nikunj

2012-06-06 11:59:51

by Thomas Gleixner

Subject: [tip:timers/core] timers: Consolidate base->next_timer update

Commit-ID: facbb4a7efbd658046bf615f03cd97a1504785d8
Gitweb: http://git.kernel.org/tip/facbb4a7efbd658046bf615f03cd97a1504785d8
Author: Thomas Gleixner <[email protected]>
AuthorDate: Fri, 25 May 2012 22:08:57 +0000
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 6 Jun 2012 13:49:01 +0200

timers: Consolidate base->next_timer update

Another bunch of mindlessly copied code. All callers of
internal_add_timer() except the recascading code updates
base->next_timer.

Move this into internal_add_timer() and let the cascading code call
__internal_add_timer().

Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Gilad Ben-Yossef <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
---
kernel/timer.c | 26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index 0f70deb..7207690 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -330,7 +330,8 @@ void set_timer_slack(struct timer_list *timer, int slack_hz)
}
EXPORT_SYMBOL_GPL(set_timer_slack);

-static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
+static void
+__internal_add_timer(struct tvec_base *base, struct timer_list *timer)
{
unsigned long expires = timer->expires;
unsigned long idx = expires - base->timer_jiffies;
@@ -372,6 +373,17 @@ static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
list_add_tail(&timer->entry, vec);
}

+static void internal_add_timer(struct tvec_base *base, struct timer_list *timer)
+{
+ __internal_add_timer(base, timer);
+ /*
+ * Update base->next_timer if this is the earliest one.
+ */
+ if (time_before(timer->expires, base->next_timer) &&
+ !tbase_get_deferrable(timer->base))
+ base->next_timer = timer->expires;
+}
+
#ifdef CONFIG_TIMER_STATS
void __timer_stats_timer_set_start_info(struct timer_list *timer, void *addr)
{
@@ -757,9 +769,6 @@ __mod_timer(struct timer_list *timer, unsigned long expires,
}

timer->expires = expires;
- if (time_before(timer->expires, base->next_timer) &&
- !tbase_get_deferrable(timer->base))
- base->next_timer = timer->expires;
internal_add_timer(base, timer);

out_unlock:
@@ -925,9 +934,6 @@ void add_timer_on(struct timer_list *timer, int cpu)
spin_lock_irqsave(&base->lock, flags);
timer_set_base(timer, base);
debug_activate(timer, timer->expires);
- if (time_before(timer->expires, base->next_timer) &&
- !tbase_get_deferrable(timer->base))
- base->next_timer = timer->expires;
internal_add_timer(base, timer);
/*
* Check whether the other CPU is idle and needs to be
@@ -1079,7 +1085,8 @@ static int cascade(struct tvec_base *base, struct tvec *tv, int index)
*/
list_for_each_entry_safe(timer, tmp, &tv_list, entry) {
BUG_ON(tbase_get_base(timer->base) != base);
- internal_add_timer(base, timer);
+ /* No accounting, while moving them */
+ __internal_add_timer(base, timer);
}

return index;
@@ -1706,9 +1713,6 @@ static void migrate_timer_list(struct tvec_base *new_base, struct list_head *hea
timer = list_first_entry(head, struct timer_list, entry);
detach_timer(timer, false);
timer_set_base(timer, new_base);
- if (time_before(timer->expires, new_base->next_timer) &&
- !tbase_get_deferrable(timer->base))
- new_base->next_timer = timer->expires;
internal_add_timer(new_base, timer);
}
}