[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Marc Zyngier]

sched_clock() is yet another blocker on the road to the single
image. This patch implements an idea by Russell King:

http://www.spinics.net/lists/linux-omap/msg49561.html

Instead of asking the platform to implement both sched_clock()
itself and the rollover callback, simply register a read()
function, and let the ARM code care about sched_clock() itself,
the conversion to ns and the rollover. sched_clock() uses
this read() function as an indirection to the platform code.

This allow some simplifications and possibly some footprint gain
when multiple platforms are compiled in. Among the drawbacks,
the removal of the *_fixed_sched_clock optimization which could
negatively impact some platforms (sa1100, tegra, versatile
and omap).

Tested on 11MPCore, OMAP4 and Tegra.

Cc: Imre Kaloz <kaloz@openwrt.org>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Colin Cross <ccross@android.com>
Cc: Erik Gilling <konkers@android.com>
Cc: Olof Johansson <olof@lixom.net>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Alessandro Rubini <rubini@unipv.it>
Cc: STEricsson <STEricsson_nomadik_linux@list.st.com>
Cc: Lennert Buytenhek <kernel@wantstofly.org>
Cc: Nicolas Pitre <nico@fluxnic.net>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Jamie Iles <jamie@jamieiles.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
Based on 3.1-rc1.

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Tony Lindgren]

* Marc Zyngier <marc.zyngier@arm.com> [110811 08:29]:
> sched_clock() is yet another blocker on the road to the single
> image. This patch implements an idea by Russell King:
> 
> http://www.spinics.net/lists/linux-omap/msg49561.html
> 
> Instead of asking the platform to implement both sched_clock()
> itself and the rollover callback, simply register a read()
> function, and let the ARM code care about sched_clock() itself,
> the conversion to ns and the rollover. sched_clock() uses
> this read() function as an indirection to the platform code.
> 
> This allow some simplifications and possibly some footprint gain
> when multiple platforms are compiled in. Among the drawbacks,
> the removal of the *_fixed_sched_clock optimization which could
> negatively impact some platforms (sa1100, tegra, versatile
> and omap).

Nice! We were already struggling supporting various timers
in a single binary for omap for sched_clock.
 
> Tested on 11MPCore, OMAP4 and Tegra.

Will test on various omaps soonish.

Tony

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Jamie Iles]

On Thu, Aug 11, 2011 at 05:02:11PM +0100, Marc Zyngier wrote:
> sched_clock() is yet another blocker on the road to the single
> image. This patch implements an idea by Russell King:
> 
> http://www.spinics.net/lists/linux-omap/msg49561.html
> 
> Instead of asking the platform to implement both sched_clock()
> itself and the rollover callback, simply register a read()
> function, and let the ARM code care about sched_clock() itself,
> the conversion to ns and the rollover. sched_clock() uses
> this read() function as an indirection to the platform code.
> 
> This allow some simplifications and possibly some footprint gain
> when multiple platforms are compiled in. Among the drawbacks,
> the removal of the *_fixed_sched_clock optimization which could
> negatively impact some platforms (sa1100, tegra, versatile
> and omap).
> 
> Tested on 11MPCore, OMAP4 and Tegra.
> 
> Cc: Imre Kaloz <kaloz@openwrt.org>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Eric Miao <eric.y.miao@gmail.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Erik Gilling <konkers@android.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Alessandro Rubini <rubini@unipv.it>
> Cc: STEricsson <STEricsson_nomadik_linux@list.st.com>
> Cc: Lennert Buytenhek <kernel@wantstofly.org>
> Cc: Nicolas Pitre <nico@fluxnic.net>
> Cc: Ben Dooks <ben-linux@fluff.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Jamie Iles <jamie@jamieiles.com>

Tested-by: Jamie Iles <jamie@jamieiles.com>

Jamie

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Marc Zyngier]

On 12/08/11 13:21, Jamie Iles wrote:
> On Thu, Aug 11, 2011 at 05:02:11PM +0100, Marc Zyngier wrote:
>> sched_clock() is yet another blocker on the road to the single
>> image. This patch implements an idea by Russell King:
>>
>> http://www.spinics.net/lists/linux-omap/msg49561.html
>>
>> Instead of asking the platform to implement both sched_clock()
>> itself and the rollover callback, simply register a read()
>> function, and let the ARM code care about sched_clock() itself,
>> the conversion to ns and the rollover. sched_clock() uses
>> this read() function as an indirection to the platform code.
>>
>> This allow some simplifications and possibly some footprint gain
>> when multiple platforms are compiled in. Among the drawbacks,
>> the removal of the *_fixed_sched_clock optimization which could
>> negatively impact some platforms (sa1100, tegra, versatile
>> and omap).
>>
>> Tested on 11MPCore, OMAP4 and Tegra.
>>
>> Cc: Imre Kaloz <kaloz@openwrt.org>
>> Cc: Krzysztof Halasa <khc@pm.waw.pl>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Eric Miao <eric.y.miao@gmail.com>
>> Cc: Colin Cross <ccross@android.com>
>> Cc: Erik Gilling <konkers@android.com>
>> Cc: Olof Johansson <olof@lixom.net>
>> Cc: Linus Walleij <linus.walleij@stericsson.com>
>> Cc: Sascha Hauer <kernel@pengutronix.de>
>> Cc: Alessandro Rubini <rubini@unipv.it>
>> Cc: STEricsson <STEricsson_nomadik_linux@list.st.com>
>> Cc: Lennert Buytenhek <kernel@wantstofly.org>
>> Cc: Nicolas Pitre <nico@fluxnic.net>
>> Cc: Ben Dooks <ben-linux@fluff.org>
>> Cc: Kukjin Kim <kgene.kim@samsung.com>
>> Cc: Jamie Iles <jamie@jamieiles.com>
> 
> Tested-by: Jamie Iles <jamie@jamieiles.com>

Thanks Jamie.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Nicolas Pitre]

On Thu, 11 Aug 2011, Marc Zyngier wrote:

> sched_clock() is yet another blocker on the road to the single
> image. This patch implements an idea by Russell King:
> 
> http://www.spinics.net/lists/linux-omap/msg49561.html
> 
> Instead of asking the platform to implement both sched_clock()
> itself and the rollover callback, simply register a read()
> function, and let the ARM code care about sched_clock() itself,
> the conversion to ns and the rollover. sched_clock() uses
> this read() function as an indirection to the platform code.
> 
> This allow some simplifications and possibly some footprint gain
> when multiple platforms are compiled in. Among the drawbacks,
> the removal of the *_fixed_sched_clock optimization which could
> negatively impact some platforms (sa1100, tegra, versatile
> and omap).
> 
> Tested on 11MPCore, OMAP4 and Tegra.

[...]

> +unsigned long long notrace sched_clock(void)
> +{
> +	if (read_sched_clock) {
> +		u32 cyc = read_sched_clock();
> +		return cyc_to_sched_clock(&cd, cyc, sched_clock_mask);
> +	}
> +
> +	return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> +					* (NSEC_PER_SEC / HZ);
> +}

This bothers me that this function has to include a conditional which 
will _always_ be evaluated as true except during early boot.  Why not 
simply initializing read_sched_clock with a jiffy_shed_clock function 
instead, and let it be overriden during boot?  This way the actual 
sched_clock() code will be more straight forward.

Of course there is the race where read_sched_clock might be updated 
before or after the corresponding clock data/mask are updated leading to 
funky results if you are lucky enough to call sched_clock() during that 
exact wrong moment.  But either we don't care (switching from a jiffy to 
a hardware clock base is going to cause quite a glitch already anyway), 
or simply disabling IRQs during initialization should prevent the race 
(presuming no other CPUs are running at that point).


Nicolas

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Marc Zyngier]

On 16/08/11 01:00, Nicolas Pitre wrote:

Hi Nicolas,

> On Thu, 11 Aug 2011, Marc Zyngier wrote:
> 
>> sched_clock() is yet another blocker on the road to the single
>> image. This patch implements an idea by Russell King:
>>
>> http://www.spinics.net/lists/linux-omap/msg49561.html
>>
>> Instead of asking the platform to implement both sched_clock()
>> itself and the rollover callback, simply register a read()
>> function, and let the ARM code care about sched_clock() itself,
>> the conversion to ns and the rollover. sched_clock() uses
>> this read() function as an indirection to the platform code.
>>
>> This allow some simplifications and possibly some footprint gain
>> when multiple platforms are compiled in. Among the drawbacks,
>> the removal of the *_fixed_sched_clock optimization which could
>> negatively impact some platforms (sa1100, tegra, versatile
>> and omap).
>>
>> Tested on 11MPCore, OMAP4 and Tegra.
> 
> [...]
> 
>> +unsigned long long notrace sched_clock(void)
>> +{
>> +	if (read_sched_clock) {
>> +		u32 cyc = read_sched_clock();
>> +		return cyc_to_sched_clock(&cd, cyc, sched_clock_mask);
>> +	}
>> +
>> +	return (unsigned long long)(jiffies - INITIAL_JIFFIES)
>> +					* (NSEC_PER_SEC / HZ);
>> +}
> 
> This bothers me that this function has to include a conditional which 
> will _always_ be evaluated as true except during early boot.  Why not 
> simply initializing read_sched_clock with a jiffy_shed_clock function 
> instead, and let it be overriden during boot?  This way the actual 
> sched_clock() code will be more straight forward.

Good point. I'll send an updated patch in a minute.

> Of course there is the race where read_sched_clock might be updated 
> before or after the corresponding clock data/mask are updated leading to 
> funky results if you are lucky enough to call sched_clock() during that 
> exact wrong moment.  But either we don't care (switching from a jiffy to 
> a hardware clock base is going to cause quite a glitch already anyway), 
> or simply disabling IRQs during initialization should prevent the race 
> (presuming no other CPUs are running at that point).

I've added a check that interrupts are disabled at that point (which
should always be true anyway).

Thanks for reviewing,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2] ARM: sched_clock: allow sched_clock to be selected at runtime

[Russell King - ARM Linux]

On Mon, Aug 15, 2011 at 08:00:45PM -0400, Nicolas Pitre wrote:
> On Thu, 11 Aug 2011, Marc Zyngier wrote:
> > +unsigned long long notrace sched_clock(void)
> > +{
> > +	if (read_sched_clock) {
> > +		u32 cyc = read_sched_clock();
> > +		return cyc_to_sched_clock(&cd, cyc, sched_clock_mask);
> > +	}
> > +
> > +	return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> > +					* (NSEC_PER_SEC / HZ);
> > +}
> 
> This bothers me that this function has to include a conditional which 
> will _always_ be evaluated as true except during early boot.  Why not 
> simply initializing read_sched_clock with a jiffy_shed_clock function 
> instead, and let it be overriden during boot?  This way the actual 
> sched_clock() code will be more straight forward.

It's also utterly pointless.

The point being is that `jiffies' is a constant during early boot,
because it needs timers and interrupts to be setup and running
(specifically the clock event subsystem) before it starts incrementing.
Chances are that's the case up until _after_ the point where
sched_clock should have already been initialized.

Plus, doing this switching in this way may result in a step in the
value of sched_clock() - possibly after sched_clock() has been used
to initialize the scheduler.

So, I don't see this as a proper solution to the problem.  It's an
attempt to "solve" one problem while creating other problems.  Let's
not do that - lets always aim to _reduce_ the number of problems
overall.