Skip to contentSkip to conversation

ARM: sched_clock: allow sched_clock to be selected at runtime

PATCH Superseded Marc Zyngier 5 participants 7 messages Aug 11, 2011 – Aug 30, 2011

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

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 <[email protected]>
Cc Krzysztof Halasa <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Eric Miao <[email protected]>
Cc Colin Cross <[email protected]>
Cc Erik Gilling <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Alessandro Rubini <[email protected]>
Cc STEricsson <[email protected]>
Cc Lennert Buytenhek <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Ben Dooks <[email protected]>
Cc Kukjin Kim <[email protected]>
Cc Jamie Iles <[email protected]>

---
Based on 3.1-rc1.

#1 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

Quoted 11 lines · 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.

Quoted 5 lines

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.

Quoted 1 line

Tested on 11MPCore, OMAP4 and Tegra.

Will test on various omaps soonish.

Tony

#2 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

In reply to Marc Zyngier
Quoted 19 lines · 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.

Quoted 16 lines
Cc Imre Kaloz <[email protected]>
Cc Krzysztof Halasa <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Eric Miao <[email protected]>
Cc Colin Cross <[email protected]>
Cc Erik Gilling <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Alessandro Rubini <[email protected]>
Cc STEricsson <[email protected]>
Cc Lennert Buytenhek <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Ben Dooks <[email protected]>
Cc Kukjin Kim <[email protected]>
Cc Jamie Iles <[email protected]>

Jamie

#3 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

Quoted 37 lines · Jamie Iles
Quoted 19 lines · 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.

Quoted 16 lines
Cc Imre Kaloz <[email protected]>
Cc Krzysztof Halasa <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Eric Miao <[email protected]>
Cc Colin Cross <[email protected]>
Cc Erik Gilling <[email protected]>
Cc Olof Johansson <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Alessandro Rubini <[email protected]>
Cc STEricsson <[email protected]>
Cc Lennert Buytenhek <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc Ben Dooks <[email protected]>
Cc Kukjin Kim <[email protected]>
Cc Jamie Iles <[email protected]>
Quoted 1 line

Thanks Jamie.

	M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#4 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

In reply to Marc Zyngier
Quoted 18 lines · 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.

[...]

Review

Quoted 10 lines
(quoted diff) +10 -0
+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

#5 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

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

Hi Nicolas,

Quoted 23 lines · Marc Zyngier
Quoted 19 lines

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.

[...]

Quoted 16 lines

Review

Quoted 10 lines
(quoted diff) +10 -0
+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.

Quoted 7 lines

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.
Signature · 1 line
Jazz is not dead. It just smells funny...
#6 lore raw

PATCH v2 1/1 ARM: sched_clock: allow sched_clock to be selected at runtime

In reply to Nicolas Pitre
Quoted 17 lines · Nicolas Pitre

Review

Quoted 10 lines · Marc Zyngier
(quoted diff) +10 -0
+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.