From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC PATCH] sched_clock: also call register_current_timer_delay() if possible
Date: Fri, 2 May 2014 17:50:12 +0100 [thread overview]
Message-ID: <20140502165011.GC20642@arm.com> (raw)
In-Reply-To: <20140430165653.GA26716@linutronix.de>
On Wed, Apr 30, 2014 at 05:56:53PM +0100, Sebastian Andrzej Siewior wrote:
> * Will Deacon | 2014-04-30 14:26:28 [+0100]:
> >I don't think that's the problem I was referring to. What I mean is that a
> >clocksource might overflow at any number of bits, so the delay calculation
> >needs to take this into account when it does:
> >
> > while ((get_cycles() - start) < cycles)
> >
> >because a premature overflow from get_cycles() will cause us to return
> >early. The solution is to mask the result of the subtraction before the
> >comparison to match the width of the clock.
>
> So I got this:
[...]
> Is this what you had in mind? If so, there is one user of
> register_current_timer_delay() which I didn't convert. That is
> arch_timer_delay_timer_register(). It returns arch_counter_get_cntvct()
> which seems to return an u64 (which is truncated to 32bit). However
> arch_counter_register() registers the clocksource with 56bits. So this
> does not look too good, right?
That should be fine, I think there's only an issue if you can overflow
twice during a single delay operation, so the thing would need to be
ticking at quite a frequency for that to happen!
> The other thing I noticed is
> |arch/arm/include/asm/timex.h:typedef unsigned long cycles_t;
>
> This works for clocksource because timekeeping is using
> |include/linux/clocksource.h:typedef u64 cycle_t;
>
> instead.
> Do I assume correct, that the arch_timer is really providing a number
> wider than 32bit? Shouldn't I then promote cycles_t to 64bit if that
> timer is active? Unless you have better suggestions of course :)
The architected timer is guaranteed to be at least 56 bits wide, but I
think we can safely truncate delay sources to 32-bit.
So actually, we only have a problem if people want to register delay clocks
smaller than 32-bit. Maybe it's simpler to enforce at least 32-bit precision
and don't bother with the registration if the clock is smaller than that?
You could use sizeof(cycles_t) to parameterise that.
Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Russell King <linux@arm.linux.org.uk>,
John Stultz <john.stultz@linaro.org>,
Theodore Ts o <tytso@mit.edu>,
Stephen Boyd <sboyd@codeaurora.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC PATCH] sched_clock: also call register_current_timer_delay() if possible
Date: Fri, 2 May 2014 17:50:12 +0100 [thread overview]
Message-ID: <20140502165011.GC20642@arm.com> (raw)
In-Reply-To: <20140430165653.GA26716@linutronix.de>
On Wed, Apr 30, 2014 at 05:56:53PM +0100, Sebastian Andrzej Siewior wrote:
> * Will Deacon | 2014-04-30 14:26:28 [+0100]:
> >I don't think that's the problem I was referring to. What I mean is that a
> >clocksource might overflow at any number of bits, so the delay calculation
> >needs to take this into account when it does:
> >
> > while ((get_cycles() - start) < cycles)
> >
> >because a premature overflow from get_cycles() will cause us to return
> >early. The solution is to mask the result of the subtraction before the
> >comparison to match the width of the clock.
>
> So I got this:
[...]
> Is this what you had in mind? If so, there is one user of
> register_current_timer_delay() which I didn't convert. That is
> arch_timer_delay_timer_register(). It returns arch_counter_get_cntvct()
> which seems to return an u64 (which is truncated to 32bit). However
> arch_counter_register() registers the clocksource with 56bits. So this
> does not look too good, right?
That should be fine, I think there's only an issue if you can overflow
twice during a single delay operation, so the thing would need to be
ticking at quite a frequency for that to happen!
> The other thing I noticed is
> |arch/arm/include/asm/timex.h:typedef unsigned long cycles_t;
>
> This works for clocksource because timekeeping is using
> |include/linux/clocksource.h:typedef u64 cycle_t;
>
> instead.
> Do I assume correct, that the arch_timer is really providing a number
> wider than 32bit? Shouldn't I then promote cycles_t to 64bit if that
> timer is active? Unless you have better suggestions of course :)
The architected timer is guaranteed to be at least 56 bits wide, but I
think we can safely truncate delay sources to 32-bit.
So actually, we only have a problem if people want to register delay clocks
smaller than 32-bit. Maybe it's simpler to enforce at least 32-bit precision
and don't bother with the registration if the clock is smaller than that?
You could use sizeof(cycles_t) to parameterise that.
Will
next prev parent reply other threads:[~2014-05-02 16:50 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-04-30 12:23 [RFC PATCH] sched_clock: also call register_current_timer_delay() if possible Sebastian Andrzej Siewior
2014-04-30 12:23 ` Sebastian Andrzej Siewior
2014-04-30 12:48 ` Will Deacon
2014-04-30 12:48 ` Will Deacon
2014-04-30 13:01 ` Sebastian Andrzej Siewior
2014-04-30 13:01 ` Sebastian Andrzej Siewior
2014-04-30 13:26 ` Will Deacon
2014-04-30 13:26 ` Will Deacon
2014-04-30 16:56 ` Sebastian Andrzej Siewior
2014-04-30 16:56 ` Sebastian Andrzej Siewior
2014-05-02 16:50 ` Will Deacon [this message]
2014-05-02 16:50 ` Will Deacon
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=20140502165011.GC20642@arm.com \
--to=will.deacon@arm.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.