On 9 April 2015 at 01:41, Thomas Gleixner tglx@linutronix.de wrote:

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.

After Peter's mail yesterday, I did check it on x86_64 and it surely looked a lot bigger.

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;
  

•           next = timerqueue_getnext(&base->active);
            if (!next)
                    continue;
  

Isn't the check we already have here lightweight enough for this ? timerqueue_getnext() returns head->next..

What benefit are we getting with this extra check ?

Maybe we can drop 'active_bases' from struct hrtimer_cpu_base ?

'active_bases' can be used effectively, if we can quit early from this loop, i.e. by checking for !active_bases on every iteration.

But that generates a lot more code and is probably not that helpful for small loop size that we have here.

-- viresh