Re: [RFC][PATCH] new timeofday core subsystem (v. A3)

[john stultz]

On Sat, 2005-03-12 at 16:49 -0800, Matt Mackall wrote:
> On Fri, Mar 11, 2005 at 05:24:15PM -0800, john stultz wrote:
> > +struct timesource_t timesource_jiffies = {
> > +	.name = "jiffies",
> > +	.priority = 0, /* lowest priority*/
> > +	.type = TIMESOURCE_FUNCTION,
> > +	.read_fnct = jiffies_read,
> > +	.mask = (cycle_t)~0,
> 
> Not sure this is right. The type of 0 is 'int' and the ~ will happen
> before the cast to a potentially longer type.

Good point. I'll change it to Andreas' suggestion of (type)-1.


> > +	.mult = NSEC_PER_SEC/HZ,
> 
> Does rounding matter here? Alpha has HZ of 1024, so this comes out to
> 976562.5.

Actually, there are probably a number of places where I need to be
better with rounding. Its a good idea there as well.

> > +static inline cycle_t read_timesource(struct timesource_t* ts)
> > +{
> > +	switch (ts->type) {
> > +	case TIMESOURCE_MMIO_32:
> > +		return (cycle_t)readl(ts->mmio_ptr);
> > +	case TIMESOURCE_MMIO_64:
> > +		return (cycle_t)readq(ts->mmio_ptr);
> > +	case TIMESOURCE_CYCLES:
> > +		return (cycle_t)get_cycles();
> > +	default:/* case: TIMESOURCE_FUNCTION */
> > +		return ts->read_fnct();
> > +	}
> > +}
> 
> Wouldn't it be better to change read_fnct to take a timesource * and
> then change all the other guys to generic_timesource_<foo> helper
> functions? This does away with the switch and makes it trivial to add
> new generic sources. Change mmio_ptr to void *private.

Not sure if I totally understand this, but originally I just had a read
function, but to allow this framework to function w/ ia64 fsyscalls (and
likely other arches vsyscalls) we need to pass the raw mmio pointers.
Thus the timesource type and switch idea was taken from the time
interpolator code.


> > @@ -467,6 +468,7 @@
> >  	pidhash_init();
> >  	init_timers();
> >  	softirq_init();
> > +	timeofday_init();
> >  	time_init();
> 
> Can we push time_init inside of timeofday_init?

Ideally, yea, but this way is cleaner until all the arches are converted
to the new timeofday code.

> > +/* Chapter 5: Kernel Variables [RFC 1589 pg. 28] */
> > +/* 5.1 Interface Variables */
> > +static int ntp_status		= STA_UNSYNC;		/* status */
> > +static long ntp_offset;					/* usec */
> > +static long ntp_constant	= 2;			/* ntp magic? */
> > +static long ntp_maxerror	= NTP_PHASE_LIMIT;	/* usec */
> > +static long ntp_esterror	= NTP_PHASE_LIMIT;	/* usec */
> > +static const long ntp_tolerance	= MAXFREQ;		/* shifted ppm */
> > +static const long ntp_precision	= 1;			/* constant */
> > +
> > +/* 5.2 Phase-Lock Loop Variables */
> > +static long ntp_freq;					/* shifted ppm */
> > +static long ntp_reftime;				/* sec */
> 
> You present a nice argument for not using tabs except at the beginning
> of the line.

Yea, I should have caught that earlier. I have the tab length set to 4
in my editor. Sorry. 


> > +#define MILLION 1000000
> 
> Still a magic number despite being a define. Very meta. Unused.

Should have been yanked along with ntp_scale(). Good catch.

> > +/* int ntp_advance(nsec_t interval):
> > + *	Periodic hook which increments NTP state machine by interval.
> > + *  Returns the signed PPM adjustment to be used for the next interval.
> > + *	This is ntp_hardclock in the RFC.
> 
> Why is it not ntp_hardclock here?

I'm not sure if ntp_hardclock is a very good name. Since we're advancing
the state machine, ntp_advance() seems to be more clear to me. However
if the NTP folks care enough then I'll be fine with changing it.

> > +	/* decrement singleshot offset interval */
> > +	ss_offset_len =- interval;
> 
> Eh?

Gah! Great catch! I would have never seen that terrible typo!



> > +		/* bound the adjustment to MAXPHASE/MINSEC */
> > +		if (tmp > (MAXPHASE / MINSEC) << SHIFT_UPDATE)
> > +		    tmp = (MAXPHASE / MINSEC) << SHIFT_UPDATE;
> > +		if (tmp < -(MAXPHASE / MINSEC) << SHIFT_UPDATE)
> > +		    tmp = -(MAXPHASE / MINSEC) << SHIFT_UPDATE;
> 
> max, min?
> 
> > +	/* Make sure offset is bounded by MAXPHASE */
> > +	if (tmp > MAXPHASE)
> > +		tmp = MAXPHASE;
> > +	if (tmp < -MAXPHASE)
> > +		tmp = -MAXPHASE;
> 
> max, min.

Good idea. 


> > +int do_adjtimex(struct timex *tx)
> > +{
> > +	do_gettimeofday(&tx->time);	/* set timex->time*/
> 
> Oh. Move the cap check back here..

Will do.

> > +	if (time_suspend_state != TIME_RUNNING) {
> > +		printk(KERN_INFO "timeofday_suspend_hook: ACK! called while we're suspended!");
> 
> Line length. Perhaps BUG_ON instead.

Eh, its not fatal to BUG_ON seems a bit harsh. I'll fix the line length
though. 


> > +	/* finally, update legacy time values */
> > +	write_seqlock_irqsave(&xtime_lock, x_flags);
> > +	xtime = ns2timespec(system_time + wall_time_offset);
> > +	wall_to_monotonic = ns2timespec(wall_time_offset);
> > +	wall_to_monotonic.tv_sec = -wall_to_monotonic.tv_sec;
> > +	wall_to_monotonic.tv_nsec = -wall_to_monotonic.tv_nsec;
> > +	/* XXX - should jiffies be updated here? */
> 
> Excellent question. 

Indeed.  Currently jiffies is used as both a interrupt counter and a
time unit, and I'm trying make it just the former. If I emulate it then
it stops functioning as a interrupt counter, and if I don't then I'll
probably break assumptions about jiffies being a time unit. So I'm not
sure which is the easiest path to go until all the users of jiffies are
audited for intent. 

As for the code style bits: Thanks, I'll try to clean those up.

I really appreciate the time you took to review my code! 

Thanks again for the feedback!
-john

-
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/