> Date: Tue, 14 Jul 2020 20:17:30 -0500
> From: Scott Cheloha <[email protected]>
> Cc: Mark Kettenis <[email protected]>, [email protected], [email protected]
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> On Tue, Jul 14, 2020 at 08:21:24PM -0400, George Koehler wrote:
> > On Tue, 14 Jul 2020 11:59:14 +0200 (CEST)
> > Mark Kettenis <[email protected]> wrote:
> >
> > > Yeah, one possible approach would be to increment ogen by two. A
> > > little bit easier to check that they can never be the same since one
> > > is always odd and the other is always even.
> > >
> > > Another possible approach would be to export both timehands. This
> > > could help avoiding some of the looping int the bin*time() functions
> > > in libc. Not sure that's worth it. And we should probably go for the
> > > "quick" fix initially regardless.
> >
> > I suspect that the bin*time() functions almost never loop back.
>
> I agree.
>
> I'm actually thinking about reducing the kernel timehands count to 1.
> We dropped from 10 to 2 about a year ago because having 10 seemed
> superfluous. There were no issues associated with that change as far
> as we know.
>
> Now I'm unsure if we even need 2 timehands. I'll need to measure it.
>
> > Here's a quick fix: read ogen from the other struct timehands, so
> > there is only one sequence of generation values, giving odds to th0
> > and evens to th1 (until the generation overflows and skips zero, then
> > it would give evens to th0 and odds to th1).
> >
> > I like this better than my first diff. OK?
>
> This also works, but like I said for your earlier diff it feels
> clever. The solution requires a non-trivial amount of thought to
> understand that it works correctly and why.
>
> The existing update protocol and th_generation code has worked for 15
> years. Excepting the recent addition of memory barriers it has worked
> fine.
>
> I'd like to keep the working code and adapt the new timekeep code to
> work with the existing code instead of changing the working code to
> support the new timekeep code. To me, it seems backwards to
> complicate simple, extant code if we don't need to.
>
> To do this I propose the attached diff. It decouples tk_generation
> from th_generation. tk_generation need not depend upon th_generation.
> That the other tk_* values come from the timehands is incidental to
> the correct operation of the lockless read protocol.
>
> I'll defer to kettenis@ though. Maybe I'm making a fuss over nothing.
>
> Thanks again for figuring this out.
I think George's change is fine and simpler.
> Index: kern_tc.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_tc.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 kern_tc.c
> --- kern_tc.c 6 Jul 2020 13:33:09 -0000 1.62
> +++ kern_tc.c 14 Jul 2020 23:42:06 -0000
> @@ -24,6 +24,7 @@
> #include <sys/param.h>
> #include <sys/atomic.h>
> #include <sys/kernel.h>
> +#include <sys/limits.h>
> #include <sys/mutex.h>
> #include <sys/rwlock.h>
> #include <sys/stdint.h>
> @@ -532,11 +533,13 @@ tc_update_timekeep(void)
> {
> static struct timecounter *last_tc = NULL;
> struct timehands *th;
> + u_int ogen;
>
> if (timekeep == NULL)
> return;
>
> th = timehands;
> + ogen = timekeep->tk_generation;
> timekeep->tk_generation = 0;
> membar_producer();
> timekeep->tk_scale = th->th_scale;
> @@ -550,7 +553,7 @@ tc_update_timekeep(void)
> last_tc = th->th_counter;
> }
> membar_producer();
> - timekeep->tk_generation = th->th_generation;
> + timekeep->tk_generation = (ogen == UINT_MAX) ? 1 : ogen + 1;
>
> return;
> }
>
