* Thomas Gleixner <[email protected]> wrote:
> On Tue, 7 Apr 2015, Viresh Kumar wrote:
>
> > At several instances we iterate over all possible clock-bases for a
> > particular cpu-base. Whereas, we only need to iterate over active bases.
> >
> > We already have per cpu-base 'active_bases' field, which is updated on
> > addition/removal of hrtimers.
> >
> > This patch creates for_each_active_base(), which uses 'active_bases' to
> > iterate only over active bases.
>
> I'm really not too excited about this incomprehensible macro mess and
> especially not about the code it generates.
>
> x86_64 i386 ARM power
>
> Mainline 7668 6942 8077 10253
>
> + Patch 8068 7294 8313 10861
>
> +400 +352 +236 +608
>
> That's insane.
>
> What's wrong with just adding
>
> if (!(cpu_base->active_bases & (1 << i)))
> continue;
>
> to the iterating sites?
>
> Index: linux/kernel/time/hrtimer.c
> ===================================================================
> --- linux.orig/kernel/time/hrtimer.c
> +++ linux/kernel/time/hrtimer.c
> @@ -451,6 +451,9 @@ static ktime_t __hrtimer_get_next_event(
> struct timerqueue_node *next;
> struct hrtimer *timer;
>
> + if (!(cpu_base->active_bases & (1 << i)))
> + continue;
> +
Btw., does cpu_base->active_bases even make sense? hrtimer bases are
fundamentally percpu, and to check whether there are any pending
timers is a very simple check:
base->active->next != NULL
So I'd rather suggest taking a direct look at the head, instead of
calculating bit positions, masks, etc.
Furthermore, we never actually use cpu_base->active_bases as a
'summary' value (which is the main point of bitmasks in general), so
I'd remove that complication altogether.
This would speed up various hrtimer primitives like
hrtimer_remove()/add and simplify the code. It would be a net code
shrink as well.
Totally untested patch below. It gives:
text data bss dec hex filename
7502 427 0 7929 1ef9 hrtimer.o.before
7422 427 0 7849 1ea9 hrtimer.o.after
and half of that code removal is from hot paths.
This would simplify the followup step of skipping over inactive bases
as well.
Thanks,
Ingo
include/linux/hrtimer.h | 2 --
kernel/time/hrtimer.c | 7 ++-----
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 05f6df1fdf5b..d65b858a94c1 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -166,7 +166,6 @@ enum hrtimer_base_type {
* @lock: lock protecting the base and associated clock bases
* and timers
* @cpu: cpu number
- * @active_bases: Bitfield to mark bases with active timers
* @clock_was_set: Indicates that clock was set from irq context.
* @expires_next: absolute time of the next event which was scheduled
* via clock_set_next_event()
@@ -182,7 +181,6 @@ enum hrtimer_base_type {
struct hrtimer_cpu_base {
raw_spinlock_t lock;
unsigned int cpu;
- unsigned int active_bases;
unsigned int clock_was_set;
#ifdef CONFIG_HIGH_RES_TIMERS
ktime_t expires_next;
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 76d4bd962b19..63e21df6c086 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -848,7 +848,6 @@ static int enqueue_hrtimer(struct hrtimer *timer,
debug_activate(timer);
timerqueue_add(&base->active, &timer->node);
- base->cpu_base->active_bases |= 1 << base->index;
/*
* HRTIMER_STATE_ENQUEUED is or'ed to the current state to preserve the
@@ -892,8 +891,6 @@ static void __remove_hrtimer(struct hrtimer *timer,
}
#endif
}
- if (!timerqueue_getnext(&base->active))
- base->cpu_base->active_bases &= ~(1 << base->index);
out:
timer->state = newstate;
}
@@ -1268,10 +1265,10 @@ void hrtimer_interrupt(struct clock_event_device *dev)
struct timerqueue_node *node;
ktime_t basenow;
- if (!(cpu_base->active_bases & (1 << i)))
+ base = cpu_base->clock_base + i;
+ if (!base->active.next)
continue;
- base = cpu_base->clock_base + i;
basenow = ktime_add(now, base->offset);
while ((node = timerqueue_getnext(&base->active))) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
