From nobody Thu Oct 28 16:43:55 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 C08441832748 for ; Thu, 28 Oct 2021 16:43:58 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HgBGQ4xGVz3MQh for ; Thu, 28 Oct 2021 16:43:58 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qk1-x72b.google.com with SMTP id bl14so6448330qkb.4 for ; Thu, 28 Oct 2021 09:43:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=2FYPhzwDop9uCNsFbdtA22WiAsRRy7udJ/itGX/OrR8=; b=MD4Nqv0CgC0YmOn8hjSwEK7FCS93qk/QEaWRMjyP94eVIxcpTT4PbyIzD0aKZoA++L PxITYZGYtUPTcQPi6yk5c8pB40mE7+ks5B6uz+HkrnDwa407uJzGNZCMwnAEoBW7BlYl IQ/EAt7Pz0JWA0G4DyzMXlbOwPib49PAaAvXDec/lMLBJWqYBZeLFb9u6xceV1qnZgds 13DCa3NnAZHjDgBVgTXWNueLJFw6eo4dBph0/xTYWskKSo8IOoKZX0UBmZx5As+iQ64Y YzWDxkf3eJH/ifZztC4HJ2V6y1+dq0dTJGmfaA4Ab0S0GOFg6+QdEDDioQzNJa4KkASm Vhjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to; bh=2FYPhzwDop9uCNsFbdtA22WiAsRRy7udJ/itGX/OrR8=; b=dTaUVmoIgCUGEpkfQEP1k2+k056pZkgmV1/vkcgcFdzHVFYbbgw1ne4EBrYxHse79z EgcPa/nlzdROMI6EJcNPZxchXJZl++mRp6A17D1ZR04VAiEhvkv6DAyQySlZG02VPr97 NzSwMkV9kIRfjdFh0pkDeEi+s6v3ABXlXnHN9scl6q8C+vR1L6ew+lt2y+H8HS0QPSiV 5LQWLAA+aqnpEGBKdHu2/8VqfD2rNn3lnIuM4nv6/oh5L20nJwTBObUKIzd2Qgfe1oji XVwU1mgjT2KiZ9MP628MCXNFiidzmouAMqHr74sTC0CcY9eou7bX1fo7dv13ZR3VDGyi h9BQ== X-Gm-Message-State: AOAM531dwC0fTOdv9Odf2RWR9355smCq/rEPWGksbDGCl+btxMiXFFL2 /M4gImuaBU1GHPfHNg9CATDDksjb0lQ= X-Google-Smtp-Source: ABdhPJx/2eFxB52uuGXaTJnXc4omQ0nZvxPdXRzLrvlUcHigbrCY716wFbgQHLBqUkZUARwE4Fp5HQ== X-Received: by 2002:a37:a285:: with SMTP id l127mr4611772qke.440.1635439438252; Thu, 28 Oct 2021 09:43:58 -0700 (PDT) Received: from nuc ([142.126.186.191]) by smtp.gmail.com with ESMTPSA id v15sm2499081qkl.91.2021.10.28.09.43.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Oct 2021 09:43:57 -0700 (PDT) Date: Thu, 28 Oct 2021 12:43:55 -0400 From: Mark Johnston To: Sebastian Huber Cc: 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-Rspamd-Queue-Id: 4HgBGQ4xGVz3MQh 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 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