From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Thu,  3 Oct 2024 14:29:56 +0200 (CEST)
Received: by mail-pl1-f169.google.com with SMTP id
 d9443c01a7336-207115e3056so7266605ad.2
 for <dev@dpdk.org>; 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>
 <Zv1-07BoFTr5DI8F@bricha3-mobl1.ger.corp.intel.com>
 <CAC-fF8SVfaTdwmh4+YVPpvsmw6wjVAGaKrWqbQAif0JxaEptOQ@mail.gmail.com>
 <Zv5kXjoMUaANrltI@bricha3-mobl1.ger.corp.intel.com>
In-Reply-To: <Zv5kXjoMUaANrltI@bricha3-mobl1.ger.corp.intel.com>
From: Isaac Boukris <iboukris@gmail.com>
Date: Thu, 3 Oct 2024 15:29:43 +0300
Message-ID: <CAC-fF8Tt78QAJTDC88OOVRNRBeCfDHgETsLx8wQPh4BSU3qD7Q@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] timer: allow platform to override cpu TSC frequency
To: Bruce Richardson <bruce.richardson@intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Thu, Oct 3, 2024 at 12:31=E2=80=AFPM Bruce Richardson
<bruce.richardson@intel.com> 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
> > <bruce.richardson@intel.com> 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 <iboukris@gmail.com>
> > > > ---
> > >
> > > 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.