netdev - Re: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 10 Jul 2014 03:34:58 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Viresh Kumar <viresh.kumar@...aro.org>,
	linaro-kernel@...ts.linaro.org, linux-kernel@...r.kernel.org,
	arvind.chauhan@....com, preeti@...ux.vnet.ibm.com,
	khilman@...aro.org, Darren Hart <dvhart@...ux.intel.com>,
	"David S. Miller" <davem@...emloft.net>,
	Ingo Molnar <mingo@...hat.com>, netdev@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it

On Wed, Jul 09, 2014 at 11:30:41PM +0200, Thomas Gleixner wrote:
> On Wed, 9 Jul 2014, Viresh Kumar wrote:
> 
> So your patch series drops active hrtimer checks after adding it,
> according to your subject line.
> 
> Quite useeul to drop something after adding it, right?
> 
> > hrtimer_start*() family never fails to enqueue a hrtimer to a clock-base. The
> > only special case is when the hrtimer was in past. If it is getting enqueued to
> > local CPUs's clock-base, we raise a softirq and exit, else we handle that on
> > next interrupt on remote CPU.
> > 
> > At several places in the kernel, we try to make sure if hrtimer was added
> > properly or not by calling hrtimer_active(), like:
> > 
> > 	hrtimer_start(timer, expires, mode);
> > 	if (hrtimer_active(timer)) {
> > 		/* Added successfully */
> > 	} else {
> > 		/* Was added in the past */
> > 	}
> > 
> > As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
> > So, there is no point calling hrtimer_active().
> 
> Wrong as usual.
> 
> It's a common pattern that short timeouts are given which lead to
> immediate expiry so the extra round through schedule is even more
> pointless than the extra check.

It may be a common pattern but it's not obvious at all as is in the code
except for timers gurus.

It looks like error handling while it's actually an optimization.

Also what about this pattern when it's used in interrupt or interrupt-disabled code?
In this case the handler is not going to fire right away, unless it's enqueued
on another CPU for unpinned timers.

For example this code in tick_nohz_stop_sched_tick():

          hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED);
          /* Check, if the timer was already in the past */
          if (hrtimer_active(&ts->sched_timer))
                 goto out;

It's not clear what this is handling. Concurrent immediate callback expiration from another CPU?
But the timer is pinned local so it can't execute right away between hrtimer_start() and hrtimer_active()
check...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html