Re: [PATCH 2/5] introduce CLOCK_MONOTONIC_RAW

[Roman Zippel <zippel@linux-m68k.org>]

Hi,

On Tuesday 18. March 2008, john stultz wrote:

> > A general comment: the raw clock doesn't need any adjustments, so updates
> > don't have to be done that frequently and you can move most of that logic
> > into second_overflow().
>
> You're suggesting adding MONTONIC_RAW code to ntp.c ?  That doesn't make
> much sense to me (the whole point is its not ntp adjusted).

I didn't looked at the code, what I meant was moving it close to it, so it's 
done at the same time.

> Even if you 
> mean just inside the "if (clock->xtime_nsec >= (u64)NSEC_PER_SEC ..."
> conditional, then that means we don't get to leverage the
> cycles_interval, and cycle_last, values, so all of that would have to be
> duplicated and managed. Which I'm not sure it would save us much more
> then the extra add and compare here.

You could use a shift (e.g. SHIFT_HZ for non NO_HZ), to scale these value up a 
little.

> > IMO that's really a clock property, so this belongs in the clock
> > structure.
> > (Some day we may want to have multiple active clocks for various purposes
> > and thus export multiple raw clocks.)
>
> I disagree. I think that crufts up the clocksource structure (which is
> ideally just a simple hw counter abstraction), with timekeeping state.

It's not cruft, littering the code with unnecessary static variables is IMO 
worse. In OO terms this would be an abstract base class, the actual 
implementation only needs to provide a few functions and otherwise doesn't 
really have to worry about all the details.
Splitting the structure is certainly an option, but hiding related variables 
as static is IMO not a good choice.

> I'm still not sold on the multiple clocks with multiple notions of time
> idea you keep on bringing up.

Well, here are some ideas where I think it might be useful, it would make it 
possible to chain clocks with different frequencies:
- SMP systems with slighty different clock frequencies.
- instead of deactivating an unstable tsc clock, delay activation until we 
know it's stable.
- deactivate a tsc (which stops during sleep) before sleep and resync when 
needed.
- use different clocks for timer purposes (e.g. jiffies and a slow clock).

(The last point would make IMO more acceptable to use hrtimer for more trivial 
purposes, if it were at least possible to select the used clock.)

> > > +		if (raw_snsec >= (u64)NSEC_PER_SEC << clock->shift) {
> > > +			raw_snsec -= (u64)NSEC_PER_SEC << clock->shift;
> > > +			monotonic_raw.tv_sec++;
> > > +		}
> > > +
> >
> > You don't really have to use clock->shift as a scale, thus simplifying
> > the shifting here (e.g. by using NTP_SCALE_SHIFT).
> > I'm thinking about doing this for e.g. xtime_nsec as well.
>
> Could you explain this more?
>
> Given the clocksources have different shift values, why should we not
> keep the extra resolution?

The extra resolution is still there, as it doesn't make any sense to use a 
shift larger than 32.

> How does adding the extra shifting to convert from the clocksource scale
> to the NTP_SCALE_SHIFT value simplify the shifting?

NTP_SCALE_SHIFT is a constant and a very simple shift, which produces better 
code.
There are two ways to use the nsec value here:
1. nsec_now = (((cycle_new - cycle_last) * mult) + nsec) >> shift
2. nsec_now = ((cycle_new - cycle_last) * mult) >> shift +
		nsec >> NTP_SCALE_SHIFT
Both do the same under the condition that the gettime operation is not 
significantly shorter than 1nsec, but the second version can generate 
slightly better code. I intended to use the first for xtime_nsec, but instead 
the value is still splitted at every timer interrupt.
For the raw time please at least get rid of the splitting of raw_nsec in 
update_wall_time(). From the monotonic_raw value you only need the tv_sec 
part and the tv_nsec part can be calculated as above.

bye, Roman