Date: Fri, 11 Dec 2020 09:18:09 -0700 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Cy Schubert <Cy.Schubert@cschubert.com>, Poul-Henning Kamp <phk@phk.freebsd.dk>, Ian Lepore <ian@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: struct timex and Linux adjtimex() Message-ID: <CANCZdfrfQk32XfDbv8TSw6ikFk0h6w%2BrH6hv_ZJbR%2Bw4Cdt%2BDQ@mail.gmail.com> In-Reply-To: <X9ONC5C2BG9ll06T@kib.kiev.ua> References: <202012030523.0B35NsG7003810@slippy.cwsent.com> <4086.1606982335@critter.freebsd.dk> <5e0db735b29f1ece02521871b2cd392c3467101d.camel@freebsd.org> <25487.1607029223@critter.freebsd.dk> <202012032203.0B3M3VJx004269@slippy.cwsent.com> <25989.1607033614@critter.freebsd.dk> <202012032258.0B3MwqVQ004875@slippy.cwsent.com> <X8l7bjf2aEPFRdYj@kib.kiev.ua> <202012040114.0B41EuFc006408@slippy.cwsent.com> <X9ONC5C2BG9ll06T@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Dec 11, 2020, 8:15 AM Konstantin Belousov <kostikbel@gmail.com>
wrote:
> On Thu, Dec 03, 2020 at 05:14:56PM -0800, Cy Schubert wrote:
> > In message <X8l7bjf2aEPFRdYj@kib.kiev.ua>, Konstantin Belousov writes:
> > > On Thu, Dec 03, 2020 at 02:58:52PM -0800, Cy Schubert wrote:
> > > > In message <25989.1607033614@critter.freebsd.dk>, "Poul-Henning
> Kamp"
> > > > writes:
> > > > > --------
> > > > > Cy Schubert writes:
> > > > >
> > > > > > I will go back
> > > > > > with my initial proposal of a timespec add/subtract syscall
> takes a
> > > > > > timespec as input increments or decrements the clock by the
> timespec an
> > > d
> > > > > > returns a timespec with the time.
> > > > >
> > > > > I would be tempted by the clock_settime(2) "clock_id" argument.
> > > > >
> > > > > The functionality required has a LOT more commonality with
> > > > > clock_settime(2) than with ntp_adjtime(2), and absconding with a
> > > > > couple of the top bits of clock_id for "CLOCK_ADD_ADJUSTMENT" and
> > > > > "CLOCK_SUB_ADJUSTMENT" would be be a pretty clean solution.
> > > >
> > > > Correct. My initial proposal was:
> > > >
> > > > +.Fn clock_updtime "clockid_t clock_id" "const struct timespec *itp"
> > > > "struct timespec *otp"
> > > >
> > > > Briefly it does this:
> > > >
> > > > +int
> > > > +kern_clock_updtime(struct thread *td, clockid_t clock_id,
> > > > + const struct timespec *its, struct timespec *ots)
> > > Note that phk suggested using specific clock id with clock_settime(),
> > > and I believe that you only need one such clock id.
> >
> > Correct. This is from work I stashed in my git repo from Sunday. I
> haven't
> > updated it yet with phk's suggestions.
> >
> > >
> > > > +{
> > > > + struct timespec ats;
> > > > + int error;
> > > > +
> > > > + if ((error = kern_clock_gettime(td, clock_id, &ats)) != 0)
> > > > + return (error);
> > > > +
> > > > + timespecadd(its, &ats, &ats);
> > > > +
> > > > + if ((error = kern_clock_settime(td, clock_id, &ats)) != 0)
> > > > + return (error);
> > > > +
> > > > + return(kern_clock_gettime(td, clock_id, ots));
> > > > +}
> > > This is awful, it must not be done this way.
> > >
> > > Look how tc_setclock() is implemented. It is careful to adjust time
> > > with interrupts and preemption disabled, and does it by adjusting the
> > > source of truth, not by fetching through several layers and then hoping
> > > that we did not get delayed too much when pushing back.
> >
> > Thanks. I'll look there.
> >
> > >
> > > I think you need to refactor tc_setclock() somewhat to allow to specify
> > > offset instead of absolute value and use it as a helper.
> >
> > I'll do that. I'll add phk and you as reviewers.
> >
> > >
> > > >
> > > > I can prepare a review if you want. I haven't touched the man page
> nor any
> > > > tests yet.
> > > >
> > > > It's affected by kib@'s https://reviews.freebsd.org/D27471, as
> conflicts
> > > > will result. I'll wait until that's committed before continuing work
> on it,
> > >
> > > > assuming this is the direction we want to go.
> > > This change does not affect *setclock() work above.
> >
> > Thanks.
>
> Ok, I went ahead and wrote https://reviews.freebsd.org/D27571 .
> This should handle all notes from the conversation.
>
Apart from two arbitrary limits that aren't explained, I like this.
Warner
_______________________________________________
> freebsd-arch@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrfQk32XfDbv8TSw6ikFk0h6w%2BrH6hv_ZJbR%2Bw4Cdt%2BDQ>