Page MenuHomeFreeBSD

sysclock: Bug fixes and cleanup
Needs ReviewPublic

Authored by Darryl.Veitch_uts.edu.au on Dec 7 2023, 12:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 11, 3:39 AM
Unknown Object (File)
Oct 1 2024, 8:16 PM
Unknown Object (File)
Sep 18 2024, 6:29 PM
Unknown Object (File)
Sep 4 2024, 6:11 PM
Unknown Object (File)
Aug 17 2024, 10:44 AM
Unknown Object (File)
Aug 16 2024, 6:40 AM
Unknown Object (File)
Jul 7 2024, 12:39 AM
Unknown Object (File)
Jul 6 2024, 2:11 AM
Subscribers

Details

Reviewers
phk
imp
brooks
Summary

Context: the sysclock system enables switching between a feedforward
(FF) or traditional feedback (FB) paradigm for system timekeeping.
Both FB and FF clock families are always running (if FFCLOCK is defined).
Which is chosen to fill timestamp requests is selectable via sysctl.

Adopting consistent naming using {FB,FF} for sysclock related variables,
in particular SYSCLOCK_{FBCK,FFWD} --> SYSCLOCK_{FB,FF} .
Also, "feed-forward" --> "feedforward" for symmetry with feedback usage.

kern_tc.c:tc_setclock :

  • bug: if sysclock is set to FFCLOCK, then the calls to nanotime and binuptime would be effected by FFclock. But as a core FBclock function, it must be completely coupled from the FFclock, especially at boot time when the FFclock is even initialised at this point. The change ensures only FB code is used.

kern_tc.c:tc_windup :

  • bug: use proper FFclock reading functions to fill time_{second,uptime} to ensure leap seconds are accounted for.

kern_ffclock.c:sysctl_kern_sysclock_active :

  • addition of verbosity to flag a change in the active sysclock: a rare and significant event.

Definition of {MS,MUS,NS,PS}_AS_BINFRAC constants to facilitate FFclock
error bound calculations and intelligibility.
Related bug fixes in kern_ffclock.c:ffclock_{abs,diff}time , where the
MUS_AS_BINFRAC value was used instead of PS_AS_BINFRAC.

kern_tc.c:sysclock_getsnapshot :

  • bug: ffi->tick_time was assigned the wrong member
  • bug: 1000000 factor in the wrong place in fbi->error calculation.
  • bug: the MUS_AS_BINFRAC value was used instead of PS_AS_BINFRAC .

kern_tc.c:sysclock_snap2bintime :

  • bug: wrong integer type passed to ffclock_convert_delta.

Addition of kernel configuration file FFCLOCK. This simply generates
the FFCLOCK symbol activating FFclock components within sysclock, and
in general.

Replacement of bcopy by equivalent memcpy throughout FFclock code for
consistency.

Whitespace adjustment to conform to style, and/or enhance readability.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 55064
Build 51953: arc lint + arc unit

Event Timeline

brooks added a subscriber: brooks.

I might have split this up a little more, but I think it's fine at this granularity.

sys/amd64/conf/FFCLOCK
2

The single blank comment line in current kernel configs is an artifact of $FreeBSD$ removal and I wouldn't include one.

I'm not sure how much appetite there is for more kernels there is in this context. the FFCLOCK entry in NOTES ensures those paths are compiled. If added, this would probably want a

#NO_UNIVERSE

entry.

This revision is now accepted and ready to land.Dec 20 2023, 4:45 PM
Darryl.Veitch_uts.edu.au marked an inline comment as done.

Config file tweak, NOTES update.

Tweaking content of FFclock kernel config file, and updating of
sys/conf/NOTES (was forgotten).

This revision now requires review to proceed.Dec 21 2023, 5:49 AM

Hi Brookes, I added a small tweak for your review a couple of months ago, but the system is still labelling it as "Unsubmitted" for some reason, so I think you haven't seen it, could you take a look?

sys/amd64/conf/FFCLOCK
2

Happy to remove the blank comment line.

I was unable to discover what #NO_UNIVERSE does, but happy to add it, can you explain?

I realised I forgot to update the comment block in sys/conf/NOTES for FFCLOCK.

Can I make the above 3 changes and arc-update this even though is accepted already? For expediency I will give this a go.

Calling Brooks?

Sorry for the radio silence. I'll keep this open a try to get back to is soon.

One problem I'm having with this review stack is that I'm qualified to review the syscall bits, but not really the clock related stuff. Perhaps @imp or @phk can find time?

This is too bog to easily review, so it has always dropped to the end of the list...

Makes sense. The commits are ordered to build things up progressively under distinct sub-topics, hopefully this offers a feasible way in. Happy to respond to any queries if that can save you time.