Re: widening ticks

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 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?

> 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.