Date: Wed, 8 Jan 2025 14:51:31 -0700 From: Warner Losh <imp@bsdimp.com> To: Mark Johnston <markj@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: widening ticks Message-ID: <CANCZdfqki1Z=jgRKAvs5kJMMTzMFXBa2B-dpRSE20S%2BpcaOEKg@mail.gmail.com> In-Reply-To: <Z37upJ3PineHvA4X@nuc> References: <Z37upJ3PineHvA4X@nuc>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000faa563062b38ddd8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
On Wed, Jan 8, 2025 at 2:31=E2=80=AFPM Mark Johnston <markj@freebsd.org> wr=
ote:
> The global "ticks" variable counts hardclock ticks, it's widely used in
> the kernel for low-precision timekeeping. The linuxkpi provides a very
> similar variable, "jiffies", but there's an incompatibility: the 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 think
> sched_pctcpu_update() is fine. I've gotten an amd64 kernel to 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 decent 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?
>
So solution (1) is MFC-able, I think, so I like it.
(2) Isn't, but is likely a better long-term solution.
(3) is a non-starter since ticks is too common a name to #define.
I could easily see a situation where we do (1) and then convert all current
users of ticks to be ticks64. This could proceed one at a time with as much
haste or caution as we need. Once we convert all of them over, we could
delete ticks and then there'd be no extra work in hardclock. This too would
be MFC-able.
sys/net/iflib.c: uint64_t this_tick =3D ticks;
sys/netinet/tcp_subr.c: < (u_int)ticks))) {
look fun! We also shadow it in a lot of places. The TCP stack uses it a lot
with a bunch of different variables, struct entries, etc, including RACK
and BBR.
The 802.11 stack uses it a bunch. As to a bunch of drivers, sometimes
shadowing
other times not.
It would be a lot to audit all this, so I think having the new API in place
might be
better, and incrementally converting / removing the shadowing (even if it
isn't
completely in scoe, using ticks as a local variable is begging for trouble)=
.
Warner
Also I see both jiffies and jiffies_64 defined. Does that matter at all?
--000000000000faa563062b38ddd8
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote g=
mail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Wed, Jan 8, =
2025 at 2:31=E2=80=AFPM Mark Johnston <<a href=3D"mailto:markj@freebsd.o=
rg">markj@freebsd.org</a>> wrote:<br></div><blockquote class=3D"gmail_qu=
ote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,20=
4);padding-left:1ex">The global "ticks" variable counts hardclock=
ticks, it's widely used in<br>
the kernel for low-precision timekeeping.=C2=A0 The linuxkpi provides a ver=
y<br>
similar variable, "jiffies", but there'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, which has been<br>
responsible for some nasty bugs, and modifying drivers 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 implementation 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 update both ticks and ticks64.=C2=A0 Then #define jiffies ticks64 on=
64-bit<br>
=C2=A0 platforms.=C2=A0 This is the simplest to implement, but it adds extr=
a work<br>
=C2=A0 to hardclock() and is somewhat ugly.<br>
- Make ticks an int64_t or a long and convert our native code<br>
=C2=A0 accordingly.=C2=A0 This is cleaner but requires a lot of auditing to=
avoid<br>
=C2=A0 introducing bugs, though perhaps some code could be left unmodified,=
<br>
=C2=A0 implicitly truncating the value to an int.=C2=A0 For example I think=
<br>
=C2=A0 sched_pctcpu_update() is fine.=C2=A0 I've gotten an amd64 kernel=
to compile<br>
=C2=A0 and boot with this change, but it's hard to be confident in it.=
=C2=A0 This<br>
=C2=A0 approach also has the potential downside of bloating structures that=
<br>
=C2=A0 store a ticks value, and it can't be MFCed.<br>
- Introduce a 64-bit ticks variable, ticks64, and<br>
=C2=A0 #define ticks ((int)ticks64).=C2=A0 This requires renaming any struc=
t<br>
=C2=A0 fields and local vars named "ticks", of which there's =
a decent number,<br>
=C2=A0 but that can be done fairly mechanically.<br>
<br>
Is there another solution which avoids these pitfalls?=C2=A0 If not, should=
<br>
we go ahead with one of these approaches?=C2=A0 If so, which one?<br></bloc=
kquote><div><br></div><div>So solution (1) is MFC-able, I think, so I like =
it.</div><div>(2) Isn't, but is likely a better long-term solution.</di=
v><div>(3) is a non-starter since ticks is too common a name to #define.</d=
iv><div><br></div><div>I could easily see a situation where we do (1) and t=
hen convert all current</div><div>users of ticks to be ticks64. This could =
proceed one at a time with as much</div><div>haste or caution as we need. O=
nce we convert all of them over, we could</div><div>delete ticks and then t=
here'd be no extra work in hardclock. This too would</div><div>be MFC-a=
ble.</div><div><br></div><div>sys/net/iflib.c: uint64_t this_tick =3D ticks=
;</div><div>sys/netinet/tcp_subr.c: =C2=A0 =C2=A0 =C2=A0 =C2=A0 < (u_int=
)ticks))) {</div><div><br></div><div>look fun! We also shadow it in a lot o=
f places. The TCP stack uses it a lot</div><div>with a bunch of different v=
ariables, struct entries, etc, including RACK and BBR.</div><div>The 802.11=
stack uses it a bunch.=C2=A0 As to a bunch of drivers, sometimes shadowing=
</div><div>other times not.</div><div><br></div><div>It would be a lot to a=
udit all this, so I think having the new API in place might be</div><div>be=
tter, and incrementally converting / removing the shadowing (even if it isn=
't</div><div>completely in scoe, using ticks as a local variable is beg=
ging for trouble).</div><div><br></div><div>Warner</div><div><br></div><div=
>Also I see both jiffies and jiffies_64 defined. Does that matter at all?</=
div><div><br></div></div></div>
--000000000000faa563062b38ddd8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqki1Z=jgRKAvs5kJMMTzMFXBa2B-dpRSE20S%2BpcaOEKg>