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