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
