Messages in this thread | | | Date | Thu, 9 Apr 2015 08:12:55 +0530 | Subject | Re: [PATCH V2 2/2] hrtimer: Iterate only over active clock-bases | From | Viresh Kumar <> |
| |
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
| |