Date: Thu, 28 Oct 2021 12:43:55 -0400 From: Mark Johnston <markj@freebsd.org> To: Sebastian Huber <sebastian.huber@embedded-brains.de> Cc: freebsd-hackers@freebsd.org Subject: Re: Dynamic timecounter changes Message-ID: <YXrTS4%2BlGP7z%2BoHu@nuc> In-Reply-To: <ca05516f-8162-0906-1631-43f208751cb7@embedded-brains.de> References: <ca05516f-8162-0906-1631-43f208751cb7@embedded-brains.de>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YXrTS4%2BlGP7z%2BoHu>