[PATCH] hrtimer: Replace cpu_base->active_bases with a direct check of the active list

[Ingo Molnar]

* Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/