On Tue, Jul 14, 2020 at 11:59:14AM +0200, Mark Kettenis wrote:
> > Date: Mon, 13 Jul 2020 17:07:31 -0400
> > From: George Koehler <[email protected]>
> >
> > On Mon, 13 Jul 2020 01:18:14 -0500
> > Scott Cheloha <[email protected]> wrote:
> >
> > > To review, the update protocol is:
> > >
> > > 1. Set tk_generation to zero; the update has begun.
> > >
> > > 2. Memory barrier. The side effects of step (1) are "visible" to
> > > other CPUs before those of step (3).
> > >
> > > 3. Update the other tk_* members from the active timehands.
> > >
> > > 4. Memory barrier. The side effects of step (3) are "visible" before
> > > those of step (5).
> > >
> > > 5. Set tk_generation to th_generation; the update is over. The
> > > timekeep page is consistent once more.
> > >
> > > --
> > >
> > > As far as I can tell, tk_generation always changes after an update,
> > > during step (5).
> >
> > There are 2 values, th0.th_generation and th1.th_generation. When the
> > kernel updates tk_generation, it switches between these 2 values, but
> > the values might be equal.
> >
> > >From /sys/kern/kern_tc.c tc_windup():
> >
> > tho = timehands;
> > th = tho->th_next;
> > ogen = th->th_generation;
> > th->th_generation = 0;
> > membar_producer();
> > ... /* update th */
> > if (++ogen == 0)
> > ogen = 1;
> > membar_producer();
> > th->th_generation = ogen;
> >
> > timehands is a circular list, where th0->th_next == th1 and
> > th1->th_next == th0. Now suppose that we call tc_windup() with,
> >
> > timehands == th0
> > th0.th_generation == 2177
> > th1.th_generation == 2176
> >
> > We assign tho = th0, th = th1, and ogen = 2176. After we update the
> > clock, we increment ogen and set th1.th_generation = 2177. When we
> > update the user timekeep page, we change tk_generation from 2177, the
> > old th0.th_generation, to 2177, the new th1.th_generation; but libc
> > sees the same value 2177 and can't detect the update.
> >
> > We update the clock 100 times per second (in a 100 Hz kernel), but
> > were incrementing tk_generation only 50 times per second.
> >
> > I run the diff from my first mail (copied below). It moves apart the
> > initial values of th_generation, so they don't become equal:
> >
> > th0.th_generation = UINT_MAX / 2 /* changed from 1 */
> > th1.th_generation = 0 /* not changed */
> >
> > It might be better to change how tc_windup() sets ogen. --George
Thanks for the detailed explanation!
> 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.
I don't think it's worth it.
> And we should probably go for the "quick" fix initially regardless.
Agreed.
George: your solution is correct and quick but I think it's too
clever. I can't fully justify why. Just a gut feeling. Might be
wrong.
My alternate fix is to treat the timekeep structure as a separate
timehands and track its generation number separate from those of the
kernel timehands. See the attached diff. Basically we just reuse the
ogen code from tc_windup(). tk_generation is divorced from
th_generation.
Running with it now... I cannot trigger koehler@'s regress.
All: would appreciate a sanity check that this also fixes the issue.
Can you trigger koehler@'s regress with this?
ok?
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;
}
