From owner-freebsd-hackers@FreeBSD.ORG Wed Jun 3 11:46:20 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id ACD78BA5 for ; Wed, 3 Jun 2015 11:46:20 +0000 (UTC) (envelope-from sebastian.huber@embedded-brains.de) Received: from mail.embedded-brains.de (host-82-135-62-35.customer.m-online.net [82.135.62.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 37E6B1C55 for ; Wed, 3 Jun 2015 11:46:19 +0000 (UTC) (envelope-from sebastian.huber@embedded-brains.de) Received: from localhost (localhost.localhost [127.0.0.1]) by mail.embedded-brains.de (Postfix) with ESMTP id ED35E2A1989; Wed, 3 Jun 2015 13:46:13 +0200 (CEST) Received: from mail.embedded-brains.de ([127.0.0.1]) by localhost (zimbra.eb.localhost [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id V6BaU5sOap6a; Wed, 3 Jun 2015 13:46:13 +0200 (CEST) Received: from localhost (localhost.localhost [127.0.0.1]) by mail.embedded-brains.de (Postfix) with ESMTP id 690AE2A1962; Wed, 3 Jun 2015 13:46:13 +0200 (CEST) X-Virus-Scanned: amavisd-new at zimbra.eb.localhost Received: from mail.embedded-brains.de ([127.0.0.1]) by localhost (zimbra.eb.localhost [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id 7Kx9uFSD5CxU; Wed, 3 Jun 2015 13:46:13 +0200 (CEST) Received: from huber-linux.eb.localhost (unknown [192.168.96.129]) by mail.embedded-brains.de (Postfix) with ESMTP id 40C192A1939; Wed, 3 Jun 2015 13:46:13 +0200 (CEST) From: Sebastian Huber To: freebsd-hackers@freebsd.org Cc: phk@phk.freebsd.dk, Sebastian Huber Subject: [PATCH] timecounters: Fix timehand generation read/write Date: Wed, 3 Jun 2015 13:46:06 +0200 Message-Id: <1433331966-27548-1-git-send-email-sebastian.huber@embedded-brains.de> X-Mailer: git-send-email 1.8.4.5 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Jun 2015 11:46:20 -0000 The compiler is free to re-order load/store instructions to non-volatile variables around a load/store of a volatile variable. So the volatile generation counter is insufficent. In addition tests on a Freescale T4240 platform with 24 PowerPC processors showed that real memory barriers are required. Compiler memory barriers are not enough. For the test the timehand count was reduced to one with 10000 tc_windup() calls per second. The timehand memory location was adjusted so that the th_generation field was on its own cache line. --- sys/kern/kern_tc.c | 102 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 60 insertions(+), 42 deletions(-) diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c index 9dca0e8..00768cf 100644 --- a/sys/kern/kern_tc.c +++ b/sys/kern/kern_tc.c @@ -71,7 +71,7 @@ struct timehands { struct timeval th_microtime; struct timespec th_nanotime; /* Fields not to be copied in tc_windup start with th_generation. */ - volatile u_int th_generation; + u_int th_generation; struct timehands *th_next; }; @@ -189,6 +189,24 @@ tc_delta(struct timehands *th) tc->tc_counter_mask); } +static u_int +tc_getgen(struct timehands *th) +{ + u_int gen; + + gen = th->th_generation; + rmb(); + return (gen); +} + +static void +tc_setgen(struct timehands *th, u_int newgen) +{ + + wmb(); + th->th_generation = newgen; +} + /* * Functions for reading the time. We have to loop until we are sure that * the timehands that we operated on was not updated under our feet. See @@ -204,10 +222,10 @@ fbclock_binuptime(struct bintime *bt) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *bt = th->th_offset; bintime_addx(bt, th->th_scale * tc_delta(th)); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -262,9 +280,9 @@ fbclock_getbinuptime(struct bintime *bt) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *bt = th->th_offset; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -275,9 +293,9 @@ fbclock_getnanouptime(struct timespec *tsp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); bintime2timespec(&th->th_offset, tsp); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -288,9 +306,9 @@ fbclock_getmicrouptime(struct timeval *tvp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); bintime2timeval(&th->th_offset, tvp); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -301,9 +319,9 @@ fbclock_getbintime(struct bintime *bt) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *bt = th->th_offset; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); bintime_add(bt, &boottimebin); } @@ -315,9 +333,9 @@ fbclock_getnanotime(struct timespec *tsp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *tsp = th->th_nanotime; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -328,9 +346,9 @@ fbclock_getmicrotime(struct timeval *tvp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *tvp = th->th_microtime; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } #else /* !FFCLOCK */ void @@ -341,10 +359,10 @@ binuptime(struct bintime *bt) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *bt = th->th_offset; bintime_addx(bt, th->th_scale * tc_delta(th)); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -399,9 +417,9 @@ getbinuptime(struct bintime *bt) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *bt = th->th_offset; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -412,9 +430,9 @@ getnanouptime(struct timespec *tsp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); bintime2timespec(&th->th_offset, tsp); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -425,9 +443,9 @@ getmicrouptime(struct timeval *tvp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); bintime2timeval(&th->th_offset, tvp); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -438,9 +456,9 @@ getbintime(struct bintime *bt) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *bt = th->th_offset; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); bintime_add(bt, &boottimebin); } @@ -452,9 +470,9 @@ getnanotime(struct timespec *tsp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *tsp = th->th_nanotime; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } void @@ -465,9 +483,9 @@ getmicrotime(struct timeval *tvp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *tvp = th->th_microtime; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } #endif /* FFCLOCK */ @@ -880,11 +898,11 @@ ffclock_read_counter(ffcounter *ffcount) */ do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); ffth = fftimehands; delta = tc_delta(th); *ffcount = ffth->tick_ffcount; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); *ffcount += delta; } @@ -988,9 +1006,9 @@ dtrace_getnanotime(struct timespec *tsp) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); *tsp = th->th_nanotime; - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); } /* @@ -1028,7 +1046,7 @@ sysclock_getsnapshot(struct sysclock_snap *clock_snap, int fast) do { th = timehands; - gen = th->th_generation; + gen = tc_getgen(th); fbi->th_scale = th->th_scale; fbi->tick_time = th->th_offset; #ifdef FFCLOCK @@ -1042,7 +1060,7 @@ sysclock_getsnapshot(struct sysclock_snap *clock_snap, int fast) #endif if (!fast) delta = tc_delta(th); - } while (gen == 0 || gen != th->th_generation); + } while (gen == 0 || gen != tc_getgen(th)); clock_snap->delta = delta; clock_snap->sysclock_active = sysclock_active; @@ -1259,8 +1277,8 @@ tc_windup(void) */ tho = timehands; th = tho->th_next; - ogen = th->th_generation; - th->th_generation = 0; + ogen = tc_getgen(th); + tc_setgen(th, 0); bcopy(tho, th, offsetof(struct timehands, th_generation)); /* @@ -1377,7 +1395,7 @@ tc_windup(void) */ if (++ogen == 0) ogen = 1; - th->th_generation = ogen; + tc_setgen(th, ogen); /* Go live with the new struct timehands. */ #ifdef FFCLOCK @@ -1651,13 +1669,13 @@ pps_capture(struct pps_state *pps) KASSERT(pps != NULL, ("NULL pps pointer in pps_capture")); th = timehands; - pps->capgen = th->th_generation; + pps->capgen = tc_getgen(th); pps->capth = th; #ifdef FFCLOCK pps->capffth = fftimehands; #endif pps->capcount = th->th_counter->tc_get_timecount(th->th_counter); - if (pps->capgen != th->th_generation) + if (pps->capgen != tc_getgen(th)) pps->capgen = 0; } @@ -1677,7 +1695,7 @@ pps_event(struct pps_state *pps, int event) KASSERT(pps != NULL, ("NULL pps pointer in pps_event")); /* If the timecounter was wound up underneath us, bail out. */ - if (pps->capgen == 0 || pps->capgen != pps->capth->th_generation) + if (pps->capgen == 0 || pps->capgen != tc_getgen(pps->capth)) return; /* Things would be easier with arrays. */ @@ -1727,7 +1745,7 @@ pps_event(struct pps_state *pps, int event) bintime2timespec(&bt, &ts); /* If the timecounter was wound up underneath us, bail out. */ - if (pps->capgen != pps->capth->th_generation) + if (pps->capgen != tc_getgen(pps->capth)) return; *pcount = pps->capcount; -- 1.8.4.5