Re: Dynamic timecounter changes

On Thu, Oct 28, 2021 at 12:43:55PM -0400, Mark Johnston wrote:
> 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.
You can protect list update both with tc_lock and tc_setclock spinlock.
Then the iteration can use tc_lock as it does now.

> 
> 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

I think this would be useful regardless of the locking.