Re: [RFC][patch 1/5] move clock source related code toclocksource.c

[Martin Schwidefsky]

On Tue, 21 Jul 2009 15:00:07 -0700
john stultz <johnstul@xxxxxxxxxx> wrote:

> On Tue, 2009-07-21 at 12:50 -0700, Daniel Walker wrote:
> > On Tue, 2009-07-21 at 21:17 +0200, Martin Schwidefsky wrote:
> > > plain text document attachment (clocksource-move.diff)
> > > From: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> > > 
> > > Move clock source related code from timekeeping.c to clocksource.c
> > > where they belong. The selected clocks source "clock" is now defined
> > > in clocksource.c and clocksource_init is added to set up the initial
> > > clock.
> > 
> > The problem is most (if not all) the code your moving is actually time
> > keeping code .. The reason it seems like clocksource code is cause John
> > wasn't very choosy about which structure he added variables too .. So
> > really this clean up needs to be in reverse, remove all the timekeeping
> > code from the clocksource code.
> 
> Not so much that I wasn't very choosy, but that I had to pick my battles
> there. At the time, Roman claimed keeping the timekeeping values in the
> clocksource (instead of global to timekeeping.c) actually produced
> better code.

There are only a couple of functions where the performance is
important, these are the ktime_get functions (and variants) and probably
the update_wall_time function.

> I do agree with Daniel's main point, that the patch mixes the layers I
> tried to establish in the design.
> 
> Clocksource: Abstracts out a hardware counter.
> NTP: Provides the reference time.
> Timekeeping: Manages accumulating the clocksource, and combining input
> from ntp's reference time to steer the hardware frequency.

Imho what makes the code hard to understand is that the internals of
the clocksource have leaked into the timekeeping code. I'm getting at
the cycle, mult and shift values here. The code would be much easier to
understand if the clocksource would just return nanoseconds. The bad
thing here is that we would loose some bits of precision.

> Unfortunately, many timekeeping values got stuffed into the struct
> clocksource. I've had plans to try to clean this up and utilize Patrick
> Ohly's simpler clockcounter struct as a basis for a clocksource, nesting
> the structures somewhat to look something like:
> 
> 
> /* minimal structure only giving hardware info and access methods */
> struct cyclecounter {
> 	char *name;
> 	cycle_t (*read)(const struct cyclecounter *cc);
> 	cycle_t (*vread)(const struct cyclecounter *cc);
> 	cycle_t mask;
> 	u32 mult;
> 	u32 shift;
> };
> 
> /* more complicated structure holding timekeeping values */
> struct timesource {
> 	struct cyclecounter counter;
> 	u32	corrected_mult;
> 	cycle_t cycle_interval;
> 	u64	xtime_interval;
> 	u32	raw_interval;
> 	cycle_t cycle_last;
> 	u64	xtime_nsec;
> 	s64	error; /* probably should be ntp_error */
> 	...
> }
> 
> However such a change would be quite a bit of churn to much of the
> timekeeping code, and to only marginal benefit. So I've put it off.

That would be an improvement, but there are still these pesky cycles in
the timesource.
 
> Martin, I've not been able to review your changes in extreme detail, but
> I'm curious what the motivation for the drastic code rearrangement was?

It started of with a minor performance optimization, I wanted to get
rid of the change_clocksource call every tick. When I looked at the
code to understand it I started to move things around.

> I see you pushing a fair amount of code down a level, for instance,
> except for the locking, getmonotonicraw() basically gets pushed down to
> clocksource_read_raw().  The ktime_get/ktime_get_ts/getnstimeofday do
> reduce some duplicate code, but that could still be minimized without
> pushing stuff down to the clocksource level.

The background here is that I want to isolate the use ofthe cycles, mult
and shift values to clocksource.[ch]

> Is there any streamlining performance benefit from the way you moved
> things around?

The biggest gain is the ktime_get optimization that is already in the
tip tree. The call to change_clocksource showed up in the instructions
trace for a cpu wake up and I started to wonder if this is necessary.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/