From nobody Thu Oct 28 18:15:50 2021 X-Original-To: freebsd-hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id CF25E1832CAF for ; Thu, 28 Oct 2021 18:16:00 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4HgDJb6k0dz4drg; Thu, 28 Oct 2021 18:15:59 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.16.1/8.16.1) with ESMTPS id 19SIFoLb013805 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Thu, 28 Oct 2021 21:15:53 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 19SIFoLb013805 Received: (from kostik@localhost) by tom.home (8.16.1/8.16.1/Submit) id 19SIFoDw013804; Thu, 28 Oct 2021 21:15:50 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 28 Oct 2021 21:15:50 +0300 From: Konstantin Belousov To: Mark Johnston Cc: Sebastian Huber , freebsd-hackers@freebsd.org Subject: Re: Dynamic timecounter changes Message-ID: References: List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.5 X-Spam-Checker-Version: SpamAssassin 3.4.5 (2021-03-20) on tom.home X-Rspamd-Queue-Id: 4HgDJb6k0dz4drg X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N 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.