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

john stultz
2004-09-03T02:08:02Z

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