Date: Fri, 4 Dec 2020 01:57:34 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Cy Schubert <Cy.Schubert@cschubert.com> Cc: 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: <X8l7bjf2aEPFRdYj@kib.kiev.ua> In-Reply-To: <202012032258.0B3MwqVQ004875@slippy.cwsent.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 and
> > > 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.
> +{
> + 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.
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 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?X8l7bjf2aEPFRdYj>