Re: [GIT PULL] omap changes for v2.6.39 merge window

[Marc Zyngier <Marc.Zyngier@arm.com>]

On Mon, 2011-04-04 at 12:21 +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 04, 2011 at 12:03:42PM +0100, Catalin Marinas wrote:
> > On Mon, 2011-04-04 at 01:59 +0100, Arnd Bergmann wrote:
> > > On Sunday 03 April 2011, Russell King - ARM Linux wrote:
> > > > Then there's those which change the cs->read function pointer at runtime,
> > ...
> > > > and those which share that pointer with their sched_clock() implementation.
> > > 
> > > Abstracting sched_clock() to be run-time selected is something that
> > > needs to be taken care of. Maybe we could have a generic sched_clock
> > > implementation that is written on top of clocksource instead of jiffies,
> > > and always select that on architectures that have a decent clocksource.
> > 
> > On Cortex-A15 with the virtualisation extensions and architected timers
> > the clocksource is implemented using a physical counter (as we want
> > wall-clock timing). But for sched_clock() we may want to use a virtual
> > counter (which is basically an offset from the physical one, set by the
> > hypervisor during guest OS switching). Marc already posted some patches
> > for this.
> 
> I had a quick look at the two patches, but I was far from impressed
> due to the apparant complexity I saw.
>
> There's no point in trying to consolidate stuff if it results in a net
> increase in the amount of code to be maintained as that just increases
> the burden, churn and maintainence headache.
> 
> Is there an easier way to consolidate it across all platforms?  I think
> so:
> 
> static DEFINE_CLOCK_DATA(cd);
> static u32 sched_clock_mask;
> static u32 (*read_sched_clock)(void);
> 
> 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);
> 	} else {
> 		/* jiffies based code */
> 	}
> }
> 
> static void notrace update_sched_clock(void)
> {
> 	u32 cyc = read_sched_clock();
> 	update_sched_clock(&cd, cyc, sched_clock_mask);
> }
> 
> void __init setup_sched_clock(u32 (*read)(void), int bits, unsigned long rate)
> {
> 	BUG_ON(bits > 32);
> 	read_sched_clock = read;
> 	sched_clock_mask = (1 << bits) - 1;
> 	init_sched_clock(&cd, update_sched_clock, bits, rate);
> }
> 
> and then get rid of the per-platform implementations entirely - all
> that platforms then have to provide is a read function and a call to
> setup_sched_clock().

The complexity mostly comes the fact that I tried to avoid having more
runtime complexity on platforms that didn't need to select their
sched_clock() implementation at runtime (no indirection while calling
sched_clock()).

If this can be relaxed, then your implementation is definitely better.

> Whether its worth it or not is questionable - the above is more lines
> of code than many of the existing implementations, and we're not going
> to shrink the existing implementations by much (maybe one to three
> lines.)  The only thing we gain is the ability to select an implementation
> at runtime.

I believe this last point to be rather important if we plan to have this
mythical single kernel covering several architectures. It's also nice
for the A15 to be able to use some default sched_clock() implementation
as a fallback if the generic timers are not available for some reason.

	M.
-- 
Reality is an implementation detail.