Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: johnstul@us.ibm.com (john stultz)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] nomadik: prevent sched_clock() wraparound
Date: Tue, 16 Nov 2010 12:53:49 -0800	[thread overview]
Message-ID: <1289940829.3860.22.camel@localhost.localdomain> (raw)
In-Reply-To: <1289898690-29910-1-git-send-email-linus.walleij@stericsson.com>

On Tue, 2010-11-16 at 10:11 +0100, Linus Walleij wrote:
> +unsigned long long sched_clock(void)
> +{
> +	unsigned long long v;
> +
> +	if (unlikely(!mtu_base))
> +		return 0;
> +
> +	v = cnt32_to_63(-readl(mtu_base + MTU_VAL(0)));
> +	/* The highest bit is not valid */
> +	v &= 0x7FFFFFFFFFFFFFFFLLU;
> +	return clocksource_cyc2ns(v, nmdk_clksrc.mult, nmdk_clksrc.shift);


I suspect this will still break. 

The cycle value being passed as v is likely to be large, and the
clocksource mult and shift are calculated to be as large as possible
without overflowing when given only a few seconds worth of cycles.

So its very likely that after a few seconds of running (or even less,
with the 32_to_63 conversion), the cyc2ns function will overflow, as
v*mult will be greater then 64bits.

So I'd advise *not* using the mult/shift pair calculated for
timekeeping, and possibly not using the clocksource_cyc2ns function (as
I'm likely to make that private to timekeeping at somepoint in the
future). Instead calculate a mult/shift pair using a smaller shift
value, so the overflow point will be much more rare.

For instance, on x86, the TSC based sched_clock uses a shift value of
10, but the TSC clocksource uses a shift value of ~30.

> +}
> +
> +/* Just kick sched_clock every so often */
> +static void cnt32_to_63_keepwarm(unsigned long data)
>  {
> -	return clocksource_cyc2ns(nmdk_clksrc.read(
> -				  &nmdk_clksrc),
> -				  nmdk_clksrc.mult,
> -				  nmdk_clksrc.shift);
> +	mod_timer(&cnt32_to_63_keepwarm_timer, round_jiffies(jiffies + data));
> +	(void) sched_clock();
> +}

Also, just FYI: This can be problematic on PREEMPT_RT systems, where
timers can be delayed by high priority processes hogging the cpu.

thanks
-john

  reply	other threads:[~2010-11-16 20:53 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-11-16  9:11 [PATCH] nomadik: prevent sched_clock() wraparound Linus Walleij
2010-11-16 20:53 ` john stultz [this message]
2010-11-16 22:15   ` Linus Walleij
2010-11-16 22:31     ` Russell King - ARM Linux
2010-11-17  8:34       ` Linus Walleij
2010-11-17  8:38         ` Russell King - ARM Linux
2010-11-17 15:15           ` Nicolas Pitre
2010-11-17  9:26         ` Linus Walleij
2010-11-17 15:04           ` Nicolas Pitre

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1289940829.3860.22.camel@localhost.localdomain \
    --to=johnstul@us.ibm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox