From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751834AbbDIG2t (ORCPT ); Thu, 9 Apr 2015 02:28:49 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:36258 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751372AbbDIG2q (ORCPT ); Thu, 9 Apr 2015 02:28:46 -0400 Date: Thu, 9 Apr 2015 08:28:41 +0200 From: Ingo Molnar To: Thomas Gleixner Cc: Viresh Kumar , Ingo Molnar , Peter Zijlstra , linaro-kernel@lists.linaro.org, linux-kernel@vger.kernel.org, Preeti U Murthy Subject: [PATCH] hrtimer: Replace cpu_base->active_bases with a direct check of the active list Message-ID: <20150409062841.GB14259@gmail.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Thomas Gleixner 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))) {