Page MenuHomeFreeBSD

Further locking changes to timekeeping code.
ClosedPublic

Authored by kib on Jul 24 2016, 3:23 PM.

Details

Summary

This is a continuation of r302252/D6825 after further bde comments.
The patch fixes one bug in r302252 and completes the locking of
time-update code. It contains several changes, which are inverweaved
and hard to either code or test independently.

Change ntpadj_lock to spinlock always (and rename stuff removing
ADJ/adj from the names). ntp_update_second() requires ntp_lock and is
called from the tc_windup(), so ntp_lock must be spinlock.

Prevent parallel tc_windup() calls, both parallel top-level calls from
setclock() and from simultaneous top-level and interrupt . For this,
tc_windup() is protected with a tc_setclock_mtx spinlock, in the try
mode when called from hardclock interrupt. If spinlock cannot be
obtained without spinning from the interrupt context, this means that
top-level executes tc_windup() on other core and our try may be
avoided.

Decrease number of timehands to just two. This is useful because
consumers can now be only one tc_windup() call late.

The boottimebin and boottime variables should be adjusted from
tc_windup(). To be correct, they must be part of the timehands and
read using lockless protocol. Remove the globals and provide the
getboottime(9)/getboottimebin(9) KPI. Change consumers of boottime to
use the KPI; this is the textually biggest part of the patch.

Some uses of boottime were found doubtful, e.g. NLM uses boottime to
identify the system boot instance. Arguably the identity should not
change on the leap second adjustment, but the patch is about
timekeeping code and the consumers were kept bug-to-bug compatible.

Reduce scope if resettodr_lock in resettodr() to just CLOCK_SETTIME().

Remove Giant from settime(), tc_setclock_mtx guards tc_windup() calls,
and there is no other issues with parallel settime(). Remove spl()
vestiges there as well.

Test Plan

Patch was tested by pho. Ntpd state was inspected during the runs.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib updated this revision to Diff 18722.Jul 24 2016, 3:23 PM
kib retitled this revision from to Further locking changes to timekeeping code..
kib updated this object.
kib edited the test plan for this revision. (Show Details)
kib added reviewers: jhb, imp, ian.
kib set the repository for this revision to rS FreeBSD src repository.
kib added a subscriber: pho.Jul 24 2016, 3:25 PM
jhb accepted this revision.Jul 26 2016, 5:00 PM
jhb edited edge metadata.

Perhaps split out an initial change to add getboottime() and getboottimebin() wrappers around the existing globals first? That is something that could be easily MFC'd to older branches so that future backports can always use the new API if needed. It would also narrow the patch to the "real" changes. I would be tempted to make the timehands change a separate commit (so it could be MFC'd easily to older branches as well).

I think the changes look good though.

This revision is now accepted and ready to land.Jul 26 2016, 5:00 PM
This revision was automatically updated to reflect the committed changes.