Re: Synchronization protocol around TODR / time-related variables

[Andrew Doran <ad%netbsd.org@localhost>]

On Mon, Dec 23, 2019 at 08:06:04PM -0800, Jason Thorpe wrote:

> Now let's talk about settime1() in kern_time.c.  It does a few things that
> seem dicey vis a vis modifying global state without proper serialization,
> and just about the entire function is wrapped in an splclock()/splx()
> pair.
>
> First of all, it's not clear to me at all that splclock()/splx() is really
> needed here...  *maybe* across the call to tc_setclock()?  But the mutex
> it takes internally is an IPL_HIGH spin mutex, so splclock() seems
> superfluous, just a legacy hold-over. 

The splclock() doesn't buy us anything as far as I can tell.

IPL_HIGH for the timecounter lock irks me given that the clock interrupt is
IPL_SCHED but I have nothing to back that up.  In any case I suppose it's a
bit like kernel printf(), people are going to call from wherever they like
even if it's somewhere we'd prefer not and that's understandable.

> The only thing that requires some synchronization is modifying "boottime",
> but maybe it's not super critical (all of the consumers of this variable
> would need significant cleanup.

Looks like it wouldn't be too hard to wrap this in a function to return it
and at least keep the problem in one place.  Ah ha!  When searching for
boottime on OpenGrok I found a FreeBSDism in the NFS code that we pulled in:
getboottime().  Digging into it a bit further, it seems they have managed
this using the same concurrency strategy used for timecounters:

http://fxr.watson.org/fxr/source/kern/kern_tc.c#L506

Andrew