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

[john stultz]

On Thu, 2004-09-02 at 18:39, George Anzinger wrote:
> john stultz wrote:
> > +static cycle_t jiffies_read(void)
> > +{
> > +	cycle_t ret = get_jiffies_64();
> > +	return ret;
> > +}
> > +
> > +static cycle_t jiffies_delta(cycle_t now, cycle_t then)
> > +{
> > +	/* simple subtraction, no need to mask */
> > +	return now - then;
> > +}
> 
> This should be inline...

Unfortunately they cannot be, as they are used as function pointers
through the struct timesource interface. 

> > +
> > +static nsec_t jiffies_cyc2ns(cycle_t cyc, cycle_t* remainder)
> > +{
> > +
> > +	cyc *= NSEC_PER_SEC/HZ;
> 
> Hm... This assumes that 1/HZ is what is needed here.  Today this value is 
> 999898.  Not exactly reachable by NSEC_PER_SEC/HZ.  Or did I miss something, 
> like the relationship of jiffie to 1/HZ and to real time.

You're right. You'd think with the recent discussions I would have
already fixed that, but it slipped by. I'll fix it to use ACTHZ in the
next release. Good catch. 


> > +int ntp_leapsecond(struct timespec now)
> > +{
> > +	/*
> > +	 * Leap second processing. If in leap-insert state at
> > +	 * the end of the day, the system clock is set back one
> > +	 * second; if in leap-delete state, the system clock is
> > +	 * set ahead one second. The microtime() routine or
> > +	 * external clock driver will insure that reported time
> > +	 * is always monotonic. The ugly divides should be
> > +	 * replaced.
> 
> ??  Is this really to be hidden?  I rather though this was just the same as a 
> user driven clock setting.  In any case, it is a slew of the wall clock and 
> should be presented to abs timer code so that the abs timers can be corrected. 
> This means a call to "clock_was_set()" AND an update of the mono_to_wall value.

Hmm. Looking at that comment, it seems a bit outdated. I'll see if I can
update it to be more clear. 

And yes, clock_was_set() is on my list. I realized I dropped it, but I
wanted to make sure I was using it properly before I just threw it in.


> > +void do_gettimeofday(struct timeval *tv)
> > +{
> > +	nsec_t wall, sys;
> > +	unsigned long seq;
> > +
> > +	/* atomically read wall and sys time */
> > +	do {
> > +		seq = read_seqbegin(&system_time_lock);
> > +
> > +		wall = wall_time_offset;
> > +		sys = __monotonic_clock();
> > +
> > +	} while (read_seqretry(&system_time_lock, seq));
> > +
> > +	/* add them and convert to timeval */
> > +	*tv = ns2timeval(wall+sys);
> > +}
> I am not sure you don't want to seperate the locking from the clock read.   This 
> so one lock can be put around larger bits of code.  For example, in 
> posix-times.c we need to get all three clocks under the same lock (that being 
> monotonic, wall_time_offset, and jiffies (and possibly as sub jiffie value)).

Well, all the values in timeofday.c are protected by the
system_time_lock, so I'm trying to minimize the locks. I do want to kill
off xtime and thus the xtime lock, so jiffies will be by itself and will
need a jiffies_lock of some sort. However part of this effort is to ween
folks off of using jiffies as a time value, thus in the future it should
only be used inside the timer subsystem as a interrupt counter. 

Thinking about it more, the NTP code could probably drop the ntp_lock
and just use the system_time_lock, but I think I'll push that
optimization off for a bit. 

> Something like:
> void get_tod_parts(nsec_t *wall, nsec_t *mon)
> {
> 	*wall = wall_time_offset;
> 	*mon  = __monotonic_clock();
> }
> 
> This could then be used in a larger function with out the double locking.

I understood your point above, but I'm not sure I see double locking.
__monotonic_clock() is lock free for exactly this reason.


> > +int do_settimeofday(struct timespec *tv)
> > +{
> > +	/* convert timespec to ns */
> > +	nsec_t newtime = timespec2ns(tv);
> > +
> > +	/* atomically adjust wall_time_offset to the desired value */
> > +	write_seqlock_irq(&system_time_lock);
> > +
> > +	wall_time_offset = newtime - __monotonic_clock();
> > +
> > +	/* clear NTP settings */
> > +	ntp_clear();
> > +
> > +	write_sequnlock_irq(&system_time_lock);
> 
> Also need a clock_was_set() call here.

Yep! 

Thanks for the feedback, George!
-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/