linux-kernel - Re: [PATCH V2 2/2] hrtimer: Iterate only over active clock-bases

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.11.1504082019220.3845@nanos>
Date:	Wed, 8 Apr 2015 22:11:05 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Viresh Kumar <viresh.kumar@...aro.org>
cc:	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: Re: [PATCH V2 2/2] hrtimer: Iterate only over active clock-bases

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;
+
 		next = timerqueue_getnext(&base->active);
 		if (!next)
 			continue;
@@ -1441,6 +1444,9 @@ void hrtimer_run_queues(void)
 		return;
 
 	for (index = 0; index < HRTIMER_MAX_CLOCK_BASES; index++) {
+		if (!(cpu_base->active_bases & (1 << index)))
+			continue;
+
 		base = &cpu_base->clock_base[index];
 		if (!timerqueue_getnext(&base->active))
 			continue;
@@ -1681,6 +1687,8 @@ static void migrate_hrtimers(int scpu)
 	raw_spin_lock_nested(&old_base->lock, SINGLE_DEPTH_NESTING);
 
 	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
+		if (!(old_base->active_bases & (1 << i)))
+			continue;
 		migrate_hrtimer_list(&old_base->clock_base[i],
 				     &new_base->clock_base[i]);
 	}

Now the code size increase is in a sane range:

		x86_64	i386	ARM	power

Mainline	7668	6942	8077	10253

+ patch		7748	6990	8113	10365

		 +80	 +48	 +36	 +112 

So your variant is at least 5 times larger than the simple and obvious
solution.

I told you before to look at the resulting binary code changes and
sanity check whether they are in the right ball park.

Thanks,

	tglx





--
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/