Date: Wed, 8 Jan 2025 17:03:23 -0700 From: Warner Losh <imp@bsdimp.com> To: Mark Johnston <markj@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: widening ticks Message-ID: <CANCZdfpn_GCT%2B_px%2BdzGPx9F3V86eSYJ-j%2BPax78%2B7yYpYsVMQ@mail.gmail.com> In-Reply-To: <Z376I6pc0c7m6FiQ@nuc> References: <Z37upJ3PineHvA4X@nuc> <CANCZdfqki1Z=jgRKAvs5kJMMTzMFXBa2B-dpRSE20S%2BpcaOEKg@mail.gmail.com> <Z376I6pc0c7m6FiQ@nuc>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
On Wed, Jan 8, 2025 at 3:20 PM Mark Johnston <markj@freebsd.org> wrote:
> On Wed, Jan 08, 2025 at 02:51:31PM -0700, Warner Losh wrote:
> > On Wed, Jan 8, 2025 at 2:31 PM Mark Johnston <markj@freebsd.org> wrote:
> >
> > > 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.
>
> Why is that a non-starter? This is just in the kernel, and as you note
> below, shadowing ticks isn't a great idea anyway. (I don't really want
> to go down this path in any case, but I'm wondering if I misunderstood
> something.)
>
I worry about it leaking to userland. And I worry about the third party
drivers
that can't tolerate it as a #define. That's all...
> > 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 = 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).
>
> Yeah, looking some more, I think having a flag day will make this too
> painful.
>
> So then I guess the question is, do we provide an int64_t ticks64 or a
> long ticksl? Do we have any 32-bit platforms where a 64-bit cmpset in
> hardclock() would be a problem?
>
I don't think so. I kinda like kib's notion too...
> > Warner
> >
> > Also I see both jiffies and jiffies_64 defined. Does that matter at all?
>
> They differ only on 32-bit systems I believe. On such systems there is
> a 64-bit tick counter, jiffies_64, but it might not be atomic.
>
Ah!
Warner
[-- Attachment #2 --]
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Wed, Jan 8, 2025 at 3:20 PM Mark Johnston <<a href="mailto:markj@freebsd.org">markj@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Jan 08, 2025 at 02:51:31PM -0700, Warner Losh wrote:<br>
> On Wed, Jan 8, 2025 at 2:31 PM Mark Johnston <<a href="mailto:markj@freebsd.org" target="_blank">markj@freebsd.org</a>> wrote:<br>
> <br>
> > The global "ticks" variable counts hardclock ticks, it's widely used in<br>
> > the kernel for low-precision timekeeping. The linuxkpi provides a very<br>
> > similar variable, "jiffies", but there's an incompatibility: the former<br>
> > is a signed int and the latter is an unsigned long. 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. I<br>
> > can see a few approaches:<br>
> > - Define a 64-bit ticks variable, say ticks64, and make hardclock()<br>
> > update both ticks and ticks64. Then #define jiffies ticks64 on 64-bit<br>
> > platforms. This is the simplest to implement, but it adds extra work<br>
> > to hardclock() and is somewhat ugly.<br>
> > - Make ticks an int64_t or a long and convert our native code<br>
> > accordingly. This is cleaner but requires a lot of auditing to avoid<br>
> > introducing bugs, though perhaps some code could be left unmodified,<br>
> > implicitly truncating the value to an int. For example I think<br>
> > sched_pctcpu_update() is fine. I've gotten an amd64 kernel to compile<br>
> > and boot with this change, but it's hard to be confident in it. This<br>
> > approach also has the potential downside of bloating structures that<br>
> > store a ticks value, and it can't be MFCed.<br>
> > - Introduce a 64-bit ticks variable, ticks64, and<br>
> > #define ticks ((int)ticks64). This requires renaming any struct<br>
> > fields and local vars named "ticks", of which there's a decent number,<br>
> > but that can be done fairly mechanically.<br>
> ><br>
> > Is there another solution which avoids these pitfalls? If not, should<br>
> > we go ahead with one of these approaches? If so, which one?<br>
> ><br>
> <br>
> So solution (1) is MFC-able, I think, so I like it.<br>
> (2) Isn't, but is likely a better long-term solution.<br>
> (3) is a non-starter since ticks is too common a name to #define.<br>
<br>
Why is that a non-starter? This is just in the kernel, and as you note<br>
below, shadowing ticks isn't a great idea anyway. (I don't really want<br>
to go down this path in any case, but I'm wondering if I misunderstood<br>
something.)<br></blockquote><div><br></div><div>I worry about it leaking to userland. And I worry about the third party drivers</div><div>that can't tolerate it as a #define. That's all...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> I could easily see a situation where we do (1) and then convert all current<br>
> users of ticks to be ticks64. This could proceed one at a time with as much<br>
> haste or caution as we need. Once we convert all of them over, we could<br>
> delete ticks and then there'd be no extra work in hardclock. This too would<br>
> be MFC-able.<br>
> <br>
> sys/net/iflib.c: uint64_t this_tick = ticks;<br>
> sys/netinet/tcp_subr.c: < (u_int)ticks))) {<br>
> <br>
> look fun! We also shadow it in a lot of places. The TCP stack uses it a lot<br>
> with a bunch of different variables, struct entries, etc, including RACK<br>
> and BBR.<br>
> The 802.11 stack uses it a bunch. As to a bunch of drivers, sometimes<br>
> shadowing<br>
> other times not.<br>
> <br>
> It would be a lot to audit all this, so I think having the new API in place<br>
> might be<br>
> better, and incrementally converting / removing the shadowing (even if it<br>
> isn't<br>
> completely in scoe, using ticks as a local variable is begging for trouble).<br>
<br>
Yeah, looking some more, I think having a flag day will make this too<br>
painful.<br>
<br>
So then I guess the question is, do we provide an int64_t ticks64 or a<br>
long ticksl? Do we have any 32-bit platforms where a 64-bit cmpset in<br>
hardclock() would be a problem?<br></blockquote><div><br></div><div>I don't think so. I kinda like kib's notion too...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Warner<br>
> <br>
> Also I see both jiffies and jiffies_64 defined. Does that matter at all?<br>
<br>
They differ only on 32-bit systems I believe. On such systems there is<br>
a 64-bit tick counter, jiffies_64, but it might not be atomic.<br></blockquote><div><br></div><div>Ah!</div><div><br></div><div>Warner </div></div></div>
home |
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpn_GCT%2B_px%2BdzGPx9F3V86eSYJ-j%2BPax78%2B7yYpYsVMQ>