From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2F43345AA0; Thu, 3 Oct 2024 14:29:58 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EE12440299; Thu, 3 Oct 2024 14:29:57 +0200 (CEST) Received: from mail-pl1-f169.google.com (mail-pl1-f169.google.com [209.85.214.169]) by mails.dpdk.org (Postfix) with ESMTP id 5AA8C4014F for ; Thu, 3 Oct 2024 14:29:56 +0200 (CEST) Received: by mail-pl1-f169.google.com with SMTP id d9443c01a7336-207115e3056so7266605ad.2 for ; Thu, 03 Oct 2024 05:29:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1727958595; x=1728563395; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Aw2fkzmHV/xxpG/EShZCbzJ/i206sI4RKVH41+epJ3I=; b=M0ctp6u01W03EgexSrJ0K/HC43ilPZOVHNh1dNnG+jFu7QarV16Q0nHAkrUwS5IoQu pnGF3lchaXN2y6BIFZziR+jz+CZAL9p5G0PvgGE9xbkQ41qbO9tVKHj1MilKwRlTSsNc 9+TIz2S/jEApzMlUrhUUmlf1IbKyBSY3JbnR9EvYjb3pGVe0AR5LGUeubCXgI+UKR+F9 MwhUp7B/IXTc8W2gkO8fDGsM63zd3ipgBZUQtsSELAKng5weE/nZYmU1rvSyINJRh2vy bsW/jyZ9n0XMuVvkiu/6S/5HEK5/vxgQAWhNHs39/N9bnjnyfXkmpLMGRLYj6oPwUCBf ZK5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727958595; x=1728563395; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Aw2fkzmHV/xxpG/EShZCbzJ/i206sI4RKVH41+epJ3I=; b=q7lHMdGQxcGy2cZhSa4wv9r6x41/cOqYEdGv15DDhP0cmcBB9LJPJo68Rwmuw/6a+E 5PJsdJXHwsbHz3tGuHbT5+fQHvsnOb1qsj3mUzzjBFFJBSjN3aKrTKs0rpqxXOZBFWsJ 6t/YSLdkDCdNr/T9aMolEFUEVCPpI1ZHjnHLNArYIEjpv7S+98mddVW+WVZskyfrbgl1 s1baMTCk1nOcFst1szFWW23gNjodMO6Ked8BUVpI7XI+Kxgh/Q0QCZNZoDumuAAAQ9D7 IjOvZ9BrCBE9nDUSx7ck/GZwGem1SxaTzGo36MjTSi3L3t9k3dyWQFCV7iu8VL+YS8R6 aFcA== X-Gm-Message-State: AOJu0YwadGiKCIOWS4DYZx4z+UTJo/L5YVEACjsPFc9fi9vYaevht5Um FVXM/UQIeY8zgfiRLSBv6YrImUCsYjp4IkSGAife5fhANGMKZHemqvEE3PJ5DeLqlLHMWUly+mz fPMwUcvj07s10hRKZg7l7DBfIol4= X-Google-Smtp-Source: AGHT+IGcea3u4SmCnWKH8CZIgdVyKCJlbNeKFO5l228BMgIshl27W1Yek3Ii6JzPG9hYjLXMbqEe5I8qGCF20YzFWEs= X-Received: by 2002:a17:903:41c6:b0:20b:5b16:b16b with SMTP id d9443c01a7336-20bc5a2db13mr93524415ad.36.1727958595190; Thu, 03 Oct 2024 05:29:55 -0700 (PDT) MIME-Version: 1.0 References: <20240921140022.107239-1-iboukris@gmail.com> <20241002165840.341116-1-iboukris@gmail.com> <20241002165840.341116-3-iboukris@gmail.com> In-Reply-To: From: Isaac Boukris Date: Thu, 3 Oct 2024 15:29:43 +0300 Message-ID: Subject: Re: [PATCH v4 2/2] timer: allow platform to override cpu TSC frequency To: Bruce Richardson Cc: dev@dpdk.org, stephen@networkplumber.org, roretzla@linux.microsoft.com, dmitry.kozliuk@gmail.com, david.marchand@redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Oct 3, 2024 at 12:31=E2=80=AFPM Bruce Richardson wrote: > > On Wed, Oct 02, 2024 at 10:14:57PM +0300, Isaac Boukris wrote: > > On Wed, Oct 2, 2024 at 8:11=E2=80=AFPM Bruce Richardson > > wrote: > > > > > > On Wed, Oct 02, 2024 at 07:56:36PM +0300, Isaac Boukris wrote: > > > > The CPU value is often not accurate, allow overriding it based > > > > on info from the host OS. > > > > > > > > On Linux X86, if the tsc_known_freq cpu flag is missing, it means > > > > the kernel doesn't trust it and calculates its own. We should do > > > > the same to avoid drift. > > > > > > > > On freebsd we have access to the kernel tsc_hz value, just use it. > > > > > > > > Signed-off-by: Isaac Boukris > > > > --- > > > > > > Looks good to me. Hope to test it out soon on my systems to see how i= t > > > goes. > > > > > > One comment inline below. > > > > > > /Bruce > > > > > > > lib/eal/common/eal_common_timer.c | 3 +- > > > > lib/eal/common/eal_private.h | 2 +- > > > > lib/eal/freebsd/eal_timer.c | 8 +++-- > > > > lib/eal/linux/eal_timer.c | 53 +++++++++++++++++++++++++++= ++-- > > > > lib/eal/windows/eal_timer.c | 5 ++- > > > > 5 files changed, 62 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/lib/eal/common/eal_common_timer.c b/lib/eal/common/eal= _common_timer.c > > > > index c5c4703f15..e00be0a5c8 100644 > > > > --- a/lib/eal/common/eal_common_timer.c > > > > +++ b/lib/eal/common/eal_common_timer.c > > > > @@ -66,8 +66,7 @@ set_tsc_freq(void) > > > > } > > > > > > > > freq =3D get_tsc_freq_arch(); > > > > - if (!freq) > > > > - freq =3D get_tsc_freq(); > > > > + freq =3D get_tsc_freq(freq); > > > > if (!freq) > > > > freq =3D estimate_tsc_freq(); > > > > > > > > diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_priv= ate.h > > > > index af09620426..bb315dab04 100644 > > > > --- a/lib/eal/common/eal_private.h > > > > +++ b/lib/eal/common/eal_private.h > > > > @@ -374,7 +374,7 @@ void set_tsc_freq(void); > > > > * > > > > * This function is private to the EAL. > > > > */ > > > > -uint64_t get_tsc_freq(void); > > > > +uint64_t get_tsc_freq(uint64_t arch_hz); > > > > > > > > /** > > > > * Get TSC frequency if the architecture supports. > > > > diff --git a/lib/eal/freebsd/eal_timer.c b/lib/eal/freebsd/eal_time= r.c > > > > index 3dd70e24ba..19fc9a7228 100644 > > > > --- a/lib/eal/freebsd/eal_timer.c > > > > +++ b/lib/eal/freebsd/eal_timer.c > > > > @@ -26,7 +26,7 @@ > > > > enum timer_source eal_timer_source =3D EAL_TIMER_TSC; > > > > > > > > uint64_t > > > > -get_tsc_freq(void) > > > > +get_tsc_freq(uint64_t arch_hz) > > > > { > > > > size_t sz; > > > > int tmp; > > > > @@ -50,9 +50,13 @@ get_tsc_freq(void) > > > > sz =3D sizeof(tsc_hz); > > > > if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) = { > > > > EAL_LOG(WARNING, "%s", strerror(errno)); > > > > - return 0; > > > > + return arch_hz; > > > > } > > > > > > > > + if (arch_hz && arch_hz - tsc_hz > arch_hz / 100) > > > > > > Do we not need an "abs()" call on the "arch_hz - tsc_hz" in case tsc_= hz is > > > the bigger value? > > > > As they are unsigned it will overflow and result the absolute value, I > > actually did use abs() at first, and the compiler yelled at me: > > warning: taking the absolute value of unsigned type =E2=80=98uint64_t= =E2=80=99 {aka > > =E2=80=98long unsigned int=E2=80=99} has no effect [-Wabsolute-value] > > Yes, the compiler is right about them being unsigned. However, without > using signed arithmetic and using abs, tsc_hz being even 1Hz greater than > arch_hz will lead to the condition failing, so it is actually checking fo= r > tsc_hz being within 1% only on the low side. Ah right, I guess the 'has no effect' warning confused me.. V5 patch on its= way.