From nobody Sat Jan 11 20:12:24 2025 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 4YVqSj4PxRz4rjQS for ; Sat, 11 Jan 2025 20:12:37 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-pj1-x1030.google.com (mail-pj1-x1030.google.com [IPv6:2607:f8b0:4864:20::1030]) (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 "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YVqSj2RC8z4Dxs for ; Sat, 11 Jan 2025 20:12:37 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pj1-x1030.google.com with SMTP id 98e67ed59e1d1-2efe25558ddso3990226a91.2 for ; Sat, 11 Jan 2025 12:12:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1736626355; x=1737231155; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=L3zOoLd+tOJIW7qfW8jg0LgzWcMRnRhV5JGo/OJljP8=; b=OW1oFkdh+aVEWHJ1IaxyTeZNx6uEtiDJC8IP8ufjXuHOf3fnsj5oQizLoJzBD6naF+ 7LGLQ/iJB8/Hxm5qh18fugExbRd7NReYwjYFxEkk7Zd5160TsFsHvZHUOE3xsD/3BsDV z2edAm85RyWcEvFH8VviY8C2BPyHZ7P4Z0jY9hbfYu3Jicw59oOdOLE6O/CWr86HF7lc ssE4WNuIjgG68aqNhtN2vhghcdgQ4K3TMAshGgDfw0bvTH/ARh1oLHWrmSpBnr1eoA3T KJAB7rJL+nxrQnAcKkWAflJaCH7T25rlJ2CnZKnqLwEwhfr11WdEWZuFKHadkbGHOBur Lsvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736626355; x=1737231155; h=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=L3zOoLd+tOJIW7qfW8jg0LgzWcMRnRhV5JGo/OJljP8=; b=n58fcIyUobQx8wefAxLnGN5i8JiDPnhJSmSKKGeuFXDLTBWgPMpvru7Xw4sC8Viggf HHwP2ZB9Da5ENwYV5kki838zGWePsPF/BKwj+T89yzewj0EeczJDXQgi4OOzAHjrNK1q BgaIq8uYulxnmrfQ4HAE3OgMSvtc/023GkCAytiGzOO0WNprqrqG2cqwZv/9sgyxIO8d 1VwUbkEe7MprWshc++iOLt8ItTgHD0VkJ61OhO4I+xAruvNFx3KykEc/F68pnsjuJLmK m5rHVAOlufm0OwyThdBwuURPHXBsqm6IufyU8Ns+rYeytcp/7E3znUQmmjMVbfduuI09 ajTA== X-Forwarded-Encrypted: i=1; AJvYcCUwaeHirxc+n2BJIrCviV9x3xjjFEPtSGZ/CB5gBEAMi7yKVcpARUowKWoXaX1VPwqyVMmQ5zUboAmqxqaaScM=@freebsd.org X-Gm-Message-State: AOJu0YwEfof4rOh4WOFwhSXK+Az6UX2zXehdEjtmx4OHn2RQqPtGZ6aV RpMAxowEsK47/Z6gGoGvOJizmBZv2MlfeOPspe7tz+QvEoZCOfMWzwu+egK/iW+c62aLix1dWm4 25EJjpOoqd4TraSHJxZmmrgo8ayZesI1DtK3RlA== X-Gm-Gg: ASbGnct9/mMVL/DaIy8kdbnRg/7nzz22b8A40HSIcBlQjBE31JncicsKRW0KaKXbGjH moxuP3lMADgjrXjmtq9U9iO4GO+1QmvLGbQl0sOZSi0b94kxTz/Ex3sF2sxpDxRh4SoYoyuqh X-Google-Smtp-Source: AGHT+IEHdYq19ar89wdPNvtcwAh2EcgNew1JUW38Px83g/zVuzpvjI4Dz+/RcANXV6NlyNNe633GWCkJvis3sUJRHCE= X-Received: by 2002:a17:90b:4a44:b0:2ea:61de:38f7 with SMTP id 98e67ed59e1d1-2f548f1d420mr22715989a91.29.1736626355451; Sat, 11 Jan 2025 12:12:35 -0800 (PST) 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 References: <20250111131106.4d2657de20eeed7eef5c0b15@dec.sakura.ne.jp> <20250112043543.86b303419f954b2b287d39d1@dec.sakura.ne.jp> In-Reply-To: <20250112043543.86b303419f954b2b287d39d1@dec.sakura.ne.jp> From: Warner Losh Date: Sat, 11 Jan 2025 13:12:24 -0700 X-Gm-Features: AbW1kvZFWVZM85E1vk66P06qN_NSjvdoPi9uTZXqvDCMXYEzZtvkAXJ7jVyFdOg Message-ID: Subject: Re: widening ticks To: Tomoaki AOKI Cc: Mark Johnston , Konstantin Belousov , FreeBSD Hackers Content-Type: multipart/alternative; boundary="0000000000000658a3062b73d52d" X-Rspamd-Queue-Id: 4YVqSj2RC8z4Dxs X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] --0000000000000658a3062b73d52d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Why not have jiffiesjust be an alias for tickl at the assembler level, then just have extern unsigned long jiffies; so the types match and we don't have fragile macros? At the assembler level, long and unsigned long are the sane for object definition. Warner On Sat, Jan 11, 2025, 12:36=E2=80=AFPM Tomoaki AOKI wrote: > On Sat, 11 Jan 2025 11:34:06 -0500 > Mark Johnston wrote: > > > On Sat, Jan 11, 2025 at 01:11:06PM +0900, Tomoaki AOKI wrote: > > > On Wed, 8 Jan 2025 18:07:47 -0500 > > > Mark Johnston wrote: > > > > > > > On Thu, Jan 09, 2025 at 12:18:48AM +0200, Konstantin Belousov wrote= : > > > > > On Wed, Jan 08, 2025 at 04:31:16PM -0500, Mark Johnston wrote: > > > > > > The global "ticks" variable counts hardclock ticks, it's widely > used in > > > > > > the kernel for low-precision timekeeping. The linuxkpi provide= s > a very > > > > > > similar variable, "jiffies", but there's an incompatibility: th= e > former > > > > > > is a signed int and the latter is an unsigned long. It's not > > > > > > particularly easy to paper over this difference, which has been > > > > > > responsible for some nasty bugs, and modifying drivers to store > the > > > > > > jiffies value in a signed int is error-prone and a maintenance > burden > > > > > > that the linuxkpi is supposed to avoid. > > > > > > > > > > > > It would be nice to provide a compatible implementation of > jiffies. I > > > > > > can see a few approaches: > > > > > > - Define a 64-bit ticks variable, say ticks64, and make > hardclock() > > > > > > update both ticks and ticks64. Then #define jiffies ticks64 > on 64-bit > > > > > > platforms. This is the simplest to implement, but it adds > extra work > > > > > > to hardclock() and is somewhat ugly. > > > > > > - Make ticks an int64_t or a long and convert our native code > > > > > > accordingly. This is cleaner but requires a lot of auditing > to avoid > > > > > > introducing bugs, though perhaps some code could be left > unmodified, > > > > > > implicitly truncating the value to an int. For example I thi= nk > > > > > > sched_pctcpu_update() is fine. I've gotten an amd64 kernel t= o > compile > > > > > > and boot with this change, but it's hard to be confident in > it. This > > > > > > approach also has the potential downside of bloating > structures that > > > > > > store a ticks value, and it can't be MFCed. > > > > > > - Introduce a 64-bit ticks variable, ticks64, and > > > > > > #define ticks ((int)ticks64). This requires renaming any > struct > > > > > > fields and local vars named "ticks", of which there's a decen= t > number, > > > > > > but that can be done fairly mechanically. > > > > > > > > > > > > Is there another solution which avoids these pitfalls? If not, > should > > > > > > we go ahead with one of these approaches? If so, which one? > > > > > > > > > > You cannot do this in C, but can in asm: > > > > > .data > > > > > .globl ticksl, ticks > > > > > .type ticksl, @object > > > > > .type ticks, @object > > > > > ticksl: .quad > > > > > .size ticksl, 8 > > > > > ticks =3Dticksl /* for little-endian */ > > > > > /* ticks =3Dticksl + 4 for big-endian */ > > > > > .size ticks, 4 > > > > > > > > > > > > > > > Then update only ticksl in the hardclock(). > > > > > > > > I implemented your suggestion here: > https://reviews.freebsd.org/D48383 > > > > > > As this is already committed to main, commenting here instead of revi= ew > > > D48383. > > > > > > Maybe I'm too paranoid and overlooking something, but... > > > > > > *If "jiffies" in LinuxKPI is really unsigned, isn't there any > > > possibilities that relies on its value to be larger than > > > 0x7fffffffffffffff as a threshold? > > > (Yes, it should be silly and non-realistic, but theoretically > > > possible.) > > > > Ideally we would have > > > > #define jiffies ((unsigned long)ticksl) > > > > in the linuxkpi, but some Linux code uses "jiffies" as a struct field o= r > > local variable name, so this doesn't quite work. > > > > In practice, the value is usually assigned to an unsigned long or used > > as an operand where it would be implicitly promoted to an unsigned type= , > > so we don't see any incompatibilities. > > > > When jiffies is an int, code like the following can misbehave: > > > > unsigned long remain, timeout =3D jiffies + const; > > ... > > remain =3D timeout - jiffies; > > if ((long)remain < 0) > > /* timed out */ > > > > If (int)timeout and jiffies have different signs, as might happen close > > to a rollover, the comparison won't work as expected. > > > > Linux has some macros (time_after() etc.) which are supposed to be used > > instead of direct comparisons, but they're not always used. > > So ticksl should better be unsigned long if there's no reason to keep > it signed, isn't it? > > > > > *Is anywhere checking carry (sign) bit for int on LP32? > > > Maybe it would be the reason if "jiffies" in LinuxKPI is really > > > unsigned. > > > > Could you provide an example of what you mean? > > Not an example of code, but for example, when ticksl is at > 0x7fffffffffffffff (positive value), ticks shoule be 0xffffffff > (negative value), if I read the diff correctly. > The same thing starts happening ticksl is at 0x0000000080000000 throug > 0x00000000ffffffff and values alike. So signs (carry bits, usually the > leftmost bit of each) should be checked separately for ticksl and ticks. > > Am I (hopefully) overlooking something? > > -- > Tomoaki AOKI > > --0000000000000658a3062b73d52d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Why not have jiffiesjust be an alias for tickl at the ass= embler level, then just have extern unsigned long jiffies; so the types mat= ch and we don't have fragile macros? At the assembler level, long and u= nsigned long are the sane for object definition.=C2=A0
Warner

On Sat, Jan 11, = 2025, 12:36=E2=80=AFPM Tomoaki AOKI <junchoon@dec.sakura.ne.jp> wrote:
On Sat, 11 Jan 2025 11:34:06 -0500
Mark Johnston <markj@freebsd.org> wrote:

> On Sat, Jan 11, 2025 at 01:11:06PM +0900, Tomoaki AOKI wrote:
> > On Wed, 8 Jan 2025 18:07:47 -0500
> > Mark Johnston <markj@freebsd.org> wrote:
> >
> > > On Thu, Jan 09, 2025 at 12:18:48AM +0200, Konstantin Belouso= v wrote:
> > > > On Wed, Jan 08, 2025 at 04:31:16PM -0500, Mark Johnston= wrote:
> > > > > The global "ticks" variable counts hardc= lock ticks, it's widely used in
> > > > > the kernel for low-precision timekeeping.=C2=A0 Th= e linuxkpi provides a very
> > > > > similar variable, "jiffies", but there&#= 39;s an incompatibility: the former
> > > > > is a signed int and the latter is an unsigned long= .=C2=A0 It's not
> > > > > particularly easy to paper over this difference, w= hich has been
> > > > > responsible for some nasty bugs, and modifying dri= vers to store the
> > > > > jiffies value in a signed int is error-prone and a= maintenance burden
> > > > > that the linuxkpi is supposed to avoid.
> > > > >
> > > > > It would be nice to provide a compatible implement= ation of jiffies.=C2=A0 I
> > > > > can see a few approaches:
> > > > > - Define a 64-bit ticks variable, say ticks64, and= make hardclock()
> > > > >=C2=A0 =C2=A0update both ticks and ticks64.=C2=A0 T= hen #define jiffies ticks64 on 64-bit
> > > > >=C2=A0 =C2=A0platforms.=C2=A0 This is the simplest = to implement, but it adds extra work
> > > > >=C2=A0 =C2=A0to hardclock() and is somewhat ugly. > > > > > - Make ticks an int64_t or a long and convert our = native code
> > > > >=C2=A0 =C2=A0accordingly.=C2=A0 This is cleaner but= requires a lot of auditing to avoid
> > > > >=C2=A0 =C2=A0introducing bugs, though perhaps some = code could be left unmodified,
> > > > >=C2=A0 =C2=A0implicitly truncating the value to an = int.=C2=A0 For example I think
> > > > >=C2=A0 =C2=A0sched_pctcpu_update() is fine.=C2=A0 I= 've gotten an amd64 kernel to compile
> > > > >=C2=A0 =C2=A0and boot with this change, but it'= s hard to be confident in it.=C2=A0 This
> > > > >=C2=A0 =C2=A0approach also has the potential downsi= de of bloating structures that
> > > > >=C2=A0 =C2=A0store a ticks value, and it can't = be MFCed.
> > > > > - Introduce a 64-bit ticks variable, ticks64, and<= br> > > > > >=C2=A0 =C2=A0#define ticks ((int)ticks64).=C2=A0 Th= is requires renaming any struct
> > > > >=C2=A0 =C2=A0fields and local vars named "tick= s", of which there's a decent number,
> > > > >=C2=A0 =C2=A0but that can be done fairly mechanical= ly.
> > > > >
> > > > > Is there another solution which avoids these pitfa= lls?=C2=A0 If not, should
> > > > > we go ahead with one of these approaches?=C2=A0 If= so, which one?
> > > >
> > > > You cannot do this in C, but can in asm:
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.data
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.globl=C2=A0 ticksl, t= icks
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type=C2=A0 =C2=A0tick= sl, @object
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type=C2=A0 =C2=A0tick= s, @object
> > > > ticksl: .quad
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.size=C2=A0 =C2=A0tick= sl, 8
> > > > ticks=C2=A0 =C2=A0=3Dticksl=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* for little-endian */
> > > > /* ticks=C2=A0 =C2=A0 =C2=A0 =C2=A0 =3Dticksl + 4=C2=A0= for big-endian */
> > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.size=C2=A0 =C2=A0tick= s, 4
> > > >
> > > >
> > > > Then update only ticksl in the hardclock().
> > >
> > > I implemented your suggestion here: http= s://reviews.freebsd.org/D48383
> >
> > As this is already committed to main, commenting here instead of = review
> > D48383.
> >
> > Maybe I'm too paranoid and overlooking something, but...
> >
> > *If "jiffies" in LinuxKPI is really unsigned, isn't= there any
> >=C2=A0 possibilities that relies on its value to be larger than > >=C2=A0 0x7fffffffffffffff as a threshold?
> >=C2=A0 (Yes, it should be silly and non-realistic, but theoretical= ly
> >=C2=A0 =C2=A0possible.)
>
> Ideally we would have
>
>=C2=A0 =C2=A0 =C2=A0#define jiffies ((unsigned long)ticksl)
>
> in the linuxkpi, but some Linux code uses "jiffies" as a str= uct field or
> local variable name, so this doesn't quite work.
>
> In practice, the value is usually assigned to an unsigned long or used=
> as an operand where it would be implicitly promoted to an unsigned typ= e,
> so we don't see any incompatibilities.
>
> When jiffies is an int, code like the following can misbehave:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long remain, timeout =3D jiffies + = const;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0...
>=C2=A0 =C2=A0 =C2=A0 =C2=A0remain =3D timeout - jiffies;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((long)remain < 0)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* timed out */<= br> >
> If (int)timeout and jiffies have different signs, as might happen clos= e
> to a rollover, the comparison won't work as expected.
>
> Linux has some macros (time_after() etc.) which are supposed to be use= d
> instead of direct comparisons, but they're not always used.

So ticksl should better be unsigned long if there's no reason to keep it signed, isn't it?


> > *Is anywhere checking carry (sign) bit for int on LP32?
> >=C2=A0 Maybe it would be the reason if "jiffies" in Linu= xKPI is really
> >=C2=A0 unsigned.
>
> Could you provide an example of what you mean?

Not an example of code, but for example, when ticksl is at
0x7fffffffffffffff (positive value), ticks shoule be 0xffffffff
(negative value), if I read the diff correctly.
The same thing starts happening ticksl is at 0x0000000080000000 throug
0x00000000ffffffff and values alike. So signs (carry bits, usually the
leftmost bit of each) should be checked separately for ticksl and ticks.
Am I (hopefully) overlooking something?

--
Tomoaki AOKI=C2=A0 =C2=A0 <junchoon@dec.sakura.ne.jp>

--0000000000000658a3062b73d52d--