Date: Sat, 11 Jan 2025 13:12:24 -0700 From: Warner Losh <imp@bsdimp.com> To: Tomoaki AOKI <junchoon@dec.sakura.ne.jp> Cc: Mark Johnston <markj@freebsd.org>, Konstantin Belousov <kostikbel@gmail.com>, FreeBSD Hackers <freebsd-hackers@freebsd.org> Subject: Re: widening ticks Message-ID: <CANCZdfqJ4GO2=-0ACfUj_%2BkKSU5VtKbrV9oeaDyNy9W6TP%2Bmng@mail.gmail.com> In-Reply-To: <20250112043543.86b303419f954b2b287d39d1@dec.sakura.ne.jp> References: <Z37upJ3PineHvA4X@nuc> <Z375yLv59Y1erje9@kib.kiev.ua> <Z38FQ4xphOJTohER@nuc> <20250111131106.4d2657de20eeed7eef5c0b15@dec.sakura.ne.jp> <Z4KdfhiHOx39Kk2T@nuc> <20250112043543.86b303419f954b2b287d39d1@dec.sakura.ne.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <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 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 <junchoon@dec.sakura.ne.jp> > > --0000000000000658a3062b73d52d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"auto">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<div dir=3D"auto"><br= ></div><div dir=3D"auto">Warner</div></div><br><div class=3D"gmail_quote gm= ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Sat, Jan 11, = 2025, 12:36=E2=80=AFPM Tomoaki AOKI <<a href=3D"mailto:junchoon@dec.saku= ra.ne.jp">junchoon@dec.sakura.ne.jp</a>> wrote:<br></div><blockquote cla= ss=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;pa= dding-left:1ex">On Sat, 11 Jan 2025 11:34:06 -0500<br> Mark Johnston <<a href=3D"mailto:markj@freebsd.org" target=3D"_blank" re= l=3D"noreferrer">markj@freebsd.org</a>> wrote:<br> <br> > On Sat, Jan 11, 2025 at 01:11:06PM +0900, Tomoaki AOKI wrote:<br> > > On Wed, 8 Jan 2025 18:07:47 -0500<br> > > Mark Johnston <<a href=3D"mailto:markj@freebsd.org" target=3D"= _blank" rel=3D"noreferrer">markj@freebsd.org</a>> wrote:<br> > > <br> > > > On Thu, Jan 09, 2025 at 12:18:48AM +0200, Konstantin Belouso= v wrote:<br> > > > > On Wed, Jan 08, 2025 at 04:31:16PM -0500, Mark Johnston= wrote:<br> > > > > > The global "ticks" variable counts hardc= lock ticks, it's widely used in<br> > > > > > the kernel for low-precision timekeeping.=C2=A0 Th= e linuxkpi provides a very<br> > > > > > similar variable, "jiffies", but there&#= 39;s an incompatibility: the former<br> > > > > > is a signed int and the latter is an unsigned long= .=C2=A0 It's not<br> > > > > > particularly easy to paper over this difference, w= hich has been<br> > > > > > responsible for some nasty bugs, and modifying dri= vers to store the<br> > > > > > jiffies value in a signed int is error-prone and a= maintenance burden<br> > > > > > that the linuxkpi is supposed to avoid.<br> > > > > > <br> > > > > > It would be nice to provide a compatible implement= ation of jiffies.=C2=A0 I<br> > > > > > can see a few approaches:<br> > > > > > - Define a 64-bit ticks variable, say ticks64, and= make hardclock()<br> > > > > >=C2=A0 =C2=A0update both ticks and ticks64.=C2=A0 T= hen #define jiffies ticks64 on 64-bit<br> > > > > >=C2=A0 =C2=A0platforms.=C2=A0 This is the simplest = to implement, but it adds extra work<br> > > > > >=C2=A0 =C2=A0to hardclock() and is somewhat ugly.<b= r> > > > > > - Make ticks an int64_t or a long and convert our = native code<br> > > > > >=C2=A0 =C2=A0accordingly.=C2=A0 This is cleaner but= requires a lot of auditing to avoid<br> > > > > >=C2=A0 =C2=A0introducing bugs, though perhaps some = code could be left unmodified,<br> > > > > >=C2=A0 =C2=A0implicitly truncating the value to an = int.=C2=A0 For example I think<br> > > > > >=C2=A0 =C2=A0sched_pctcpu_update() is fine.=C2=A0 I= 've gotten an amd64 kernel to compile<br> > > > > >=C2=A0 =C2=A0and boot with this change, but it'= s hard to be confident in it.=C2=A0 This<br> > > > > >=C2=A0 =C2=A0approach also has the potential downsi= de of bloating structures that<br> > > > > >=C2=A0 =C2=A0store a ticks value, and it can't = be MFCed.<br> > > > > > - 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<br> > > > > >=C2=A0 =C2=A0fields and local vars named "tick= s", of which there's a decent number,<br> > > > > >=C2=A0 =C2=A0but that can be done fairly mechanical= ly.<br> > > > > > <br> > > > > > Is there another solution which avoids these pitfa= lls?=C2=A0 If not, should<br> > > > > > we go ahead with one of these approaches?=C2=A0 If= so, which one?<br> > > > > <br> > > > > You cannot do this in C, but can in asm:<br> > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.data<br> > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.globl=C2=A0 ticksl, t= icks<br> > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type=C2=A0 =C2=A0tick= sl, @object<br> > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.type=C2=A0 =C2=A0tick= s, @object<br> > > > > ticksl: .quad<br> > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.size=C2=A0 =C2=A0tick= sl, 8<br> > > > > ticks=C2=A0 =C2=A0=3Dticksl=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0/* for little-endian */<br> > > > > /* ticks=C2=A0 =C2=A0 =C2=A0 =C2=A0 =3Dticksl + 4=C2=A0= for big-endian */<br> > > > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.size=C2=A0 =C2=A0tick= s, 4<br> > > > > <br> > > > > <br> > > > > Then update only ticksl in the hardclock().<br> > > > <br> > > > I implemented your suggestion here: <a href=3D"https://revie= ws.freebsd.org/D48383" rel=3D"noreferrer noreferrer" target=3D"_blank">http= s://reviews.freebsd.org/D48383</a><br> > > <br> > > As this is already committed to main, commenting here instead of = review<br> > > D48383.<br> > > <br> > > Maybe I'm too paranoid and overlooking something, but...<br> > > <br> > > *If "jiffies" in LinuxKPI is really unsigned, isn't= there any<br> > >=C2=A0 possibilities that relies on its value to be larger than<br= > > >=C2=A0 0x7fffffffffffffff as a threshold?<br> > >=C2=A0 (Yes, it should be silly and non-realistic, but theoretical= ly<br> > >=C2=A0 =C2=A0possible.)<br> > <br> > Ideally we would have<br> > <br> >=C2=A0 =C2=A0 =C2=A0#define jiffies ((unsigned long)ticksl)<br> > <br> > in the linuxkpi, but some Linux code uses "jiffies" as a str= uct field or<br> > local variable name, so this doesn't quite work.<br> > <br> > In practice, the value is usually assigned to an unsigned long or used= <br> > as an operand where it would be implicitly promoted to an unsigned typ= e,<br> > so we don't see any incompatibilities.<br> > <br> > When jiffies is an int, code like the following can misbehave:<br> > <br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long remain, timeout =3D jiffies + = const;<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0...<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0remain =3D timeout - jiffies;<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0if ((long)remain < 0)<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* timed out */<= br> > <br> > If (int)timeout and jiffies have different signs, as might happen clos= e<br> > to a rollover, the comparison won't work as expected.<br> > <br> > Linux has some macros (time_after() etc.) which are supposed to be use= d<br> > instead of direct comparisons, but they're not always used.<br> <br> So ticksl should better be unsigned long if there's no reason to keep<b= r> it signed, isn't it?<br> <br> <br> > > *Is anywhere checking carry (sign) bit for int on LP32?<br> > >=C2=A0 Maybe it would be the reason if "jiffies" in Linu= xKPI is really<br> > >=C2=A0 unsigned.<br> > <br> > Could you provide an example of what you mean?<br> <br> Not an example of code, but for example, when ticksl is at<br> 0x7fffffffffffffff (positive value), ticks shoule be 0xffffffff<br> (negative value), if I read the diff correctly.<br> The same thing starts happening ticksl is at 0x0000000080000000 throug<br> 0x00000000ffffffff and values alike. So signs (carry bits, usually the<br> leftmost bit of each) should be checked separately for ticksl and ticks.<br= > <br> Am I (hopefully) overlooking something?<br> <br> -- <br> Tomoaki AOKI=C2=A0 =C2=A0 <<a href=3D"mailto:junchoon@dec.sakura.ne.jp" = target=3D"_blank" rel=3D"noreferrer">junchoon@dec.sakura.ne.jp</a>><br> <br> </blockquote></div> --0000000000000658a3062b73d52d--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqJ4GO2=-0ACfUj_%2BkKSU5VtKbrV9oeaDyNy9W6TP%2Bmng>