lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 9 Apr 2015 08:28:41 +0200 From: Ingo Molnar <mingo@...nel.org> To: Thomas Gleixner <tglx@...utronix.de> Cc: Viresh Kumar <viresh.kumar@...aro.org>, Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, linaro-kernel@...ts.linaro.org, linux-kernel@...r.kernel.org, Preeti U Murthy <preeti@...ux.vnet.ibm.com> Subject: [PATCH] hrtimer: Replace cpu_base->active_bases with a direct check of the active list * Thomas Gleixner <tglx@...utronix.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))) { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists