Re: Dynamic timecounter changes

On Thu, Oct 28, 2021 at 10:52:59AM +0200, Sebastian Huber wrote:
> Hello,
> 
> there was a recent change which protected timecounter changes with a mutex:
> 
> https://github.com/freebsd/freebsd-src/commit/621fd9dcb2d83daab477c130bc99b905f6fc27dc
> 
> If the timecounter can change dynamically, could tc_windup() see 
> different timercounter here:
> 
> 	/*
> 	 * Capture a timecounter delta on the current timecounter and if
> 	 * changing timecounters, a counter value from the new timecounter.
> 	 * Update the offset fields accordingly.
> 	 */
> 	delta = tc_delta(th);
> 	if (th->th_counter != timecounter)
> 		ncount = timecounter->tc_get_timecount(timecounter);
> 	else
> 		ncount = 0;
> 
> and here:
> 
> 	/* Now is a good time to change timecounters. */
> 	if (th->th_counter != timecounter) {
> #ifndef __arm__
> 		if ((timecounter->tc_flags & TC_FLAGS_C2STOP) != 0)
> 			cpu_disable_c2_sleep++;
> 		if ((th->th_counter->tc_flags & TC_FLAGS_C2STOP) != 0)
> 			cpu_disable_c2_sleep--;
> #endif
> 		th->th_counter = timecounter;
> 		th->th_offset_count = ncount;
> 		tc_min_ticktock_freq = max(1, timecounter->tc_frequency /
> 		    (((uint64_t)timecounter->tc_counter_mask + 1) / 3));
> 		recalculate_scaling_factor_and_large_delta(th);
> #ifdef FFCLOCK
> 		ffclock_change_tc(th);
> #endif
> 	}
> 
> An ncount value from two different timecounter would be used in this 
> case.  Maybe the "timecounter" global variable should be just read once 
> into a local variable.

I think you are right.  An alternate solution would be to synchronize
updates of the global "timecounter" variable with tc_setclock_mtx.  That
lock can't trivially be used to protect the list since it's a spin mutex
and sysctl_kern_timecounter_choice() can't hold it across sbuf_printf()
calls.

Hmm, but the ACPI timer driver also updates the selected timecounter.
Perhaps loading "timecounter" once is the better solution for now.

diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c
index 9a928ca37aff..611d81b3280b 100644
--- a/sys/kern/kern_tc.c
+++ b/sys/kern/kern_tc.c
@@ -1319,6 +1319,7 @@ static void
 tc_windup(struct bintime *new_boottimebin)
 {
 	struct bintime bt;
+	struct timecounter *tc;
 	struct timehands *th, *tho;
 	uint64_t scale;
 	u_int delta, ncount, ogen;
@@ -1348,9 +1349,10 @@ tc_windup(struct bintime *new_boottimebin)
 	 * changing timecounters, a counter value from the new timecounter.
 	 * Update the offset fields accordingly.
 	 */
+	tc = atomic_load_ptr(&timecounter);
 	delta = tc_delta(th);
-	if (th->th_counter != timecounter)
-		ncount = timecounter->tc_get_timecount(timecounter);
+	if (th->th_counter != tc)
+		ncount = tc->tc_get_timecount(tc);
 	else
 		ncount = 0;
 #ifdef FFCLOCK
@@ -1407,17 +1409,17 @@ tc_windup(struct bintime *new_boottimebin)
 	bintime2timespec(&bt, &th->th_nanotime);
 
 	/* Now is a good time to change timecounters. */
-	if (th->th_counter != timecounter) {
+	if (th->th_counter != tc) {
 #ifndef __arm__
-		if ((timecounter->tc_flags & TC_FLAGS_C2STOP) != 0)
+		if ((tc->tc_flags & TC_FLAGS_C2STOP) != 0)
 			cpu_disable_c2_sleep++;
 		if ((th->th_counter->tc_flags & TC_FLAGS_C2STOP) != 0)
 			cpu_disable_c2_sleep--;
 #endif
-		th->th_counter = timecounter;
+		th->th_counter = tc;
 		th->th_offset_count = ncount;
-		tc_min_ticktock_freq = max(1, timecounter->tc_frequency /
-		    (((uint64_t)timecounter->tc_counter_mask + 1) / 3));
+		tc_min_ticktock_freq = max(1, tc->tc_frequency /
+		    (((uint64_t)tc->tc_counter_mask + 1) / 3));
 #ifdef FFCLOCK
 		ffclock_change_tc(th);
 #endif