linux-kernel - Re: [PATCH v2] hrtimers: calculate expires

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]

Message-ID: <20140623110302.GA20225@stfomichev-desktop.yandex.net>
Date:	Mon, 23 Jun 2014 15:03:02 +0400
From:	Stanislav Fomichev <stfomichev@...dex-team.ru>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	john.stultz@...aro.org, prarit@...hat.com,
	paul.gortmaker@...driver.com, juri.lelli@...il.com,
	ddaney.cavm@...il.com, mbohan@...eaurora.org,
	david.vrabel@...rix.com, david.engraf@...go.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hrtimers: calculate expires_next after all timers are
 executed

Hi Thomas,

> 
> + * @next:              time of the next event on this clock base
> 
> What initializes that? It's 0 to begin with.
I thought I can skip initialization because I update base->next
in the interrupt or in __remove_hrtimer, like:
- enqueue_timer, base->next is 0
- reprogram device
- device fires -> hrtimer_interrupt
- __run_hrtimer
- __remove_hrtimer
- if last base->next = KTIME_MAX
- otherwise base->next = ktime_sub(hrtimer_get_expires(timer), base->offset)
  in hrtimer_interrupt

> > @@ -893,6 +895,10 @@ static int enqueue_hrtimer(struct hrtimer *timer,
> >  	 */
> >  	timer->state |= HRTIMER_STATE_ENQUEUED;
> >  
> > +	expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
> 
> This does not work when time gets set and the offset changes. We need
> to store the absolute expiry time and subtract the offset at
> evaluation time.
Hm, looking at this code after a while it seems I don't need to update
base->next in enqueue_hrtimer. It's enough to set it to KTIME_MAX in
__remove_hrtimer or to actual value upon breaking from __run_hrtimer
loop in hrtimer_interrupt.

> > @@ -929,8 +935,10 @@ static void __remove_hrtimer(struct hrtimer *timer,
> >  		}
> >  #endif
> >  	}
> > -	if (!timerqueue_getnext(&base->active))
> > +	if (!timerqueue_getnext(&base->active)) {
> >  		base->cpu_base->active_bases &= ~(1 << base->index);
> > +		base->next = ktime_set(KTIME_SEC_MAX, 0);
> > +	}
> 
> And what updates base->next if there are timers pending?
See above, hrtimer_interrupt updates it before breaking or sets to
KTIME_MAX in __remove_hrtimer if it's the last one.

> > +	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
> > +		ktime_t expires;
> 
> So this adds the third incarnation of finding the next expiring timer
> to the code. Not really helpful.
Didn't really think about all the other places, refactoring may come in
another patch.

> Untested patch which addresses the issues below.
Aside from a small nitpick below, looks reasonable, I'll try to run it on a
couple of machines.
Should I send you a v3 afterwards with the changelog or
tested-by would be enough?

> +	while (active) {
> +		idx = __ffs(active);
> +		active &= ~(1UL << idx);
Is there any reason you did that instead of conventional:
	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
		if (!(cpu_base->active_bases & (1 << i)))
			continue;

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