* Thomas Gleixner tglx@linutronix.de 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))) {