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