[PATCH] timecounters: Fix timehand generation read/write
Gedare Bloom
gedare at gwu.edu
Wed Jun 3 14:17:47 UTC 2015
On Wed, Jun 3, 2015 at 7:57 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> 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.
>
> Close #2356.
> ---
> cpukit/score/src/kern_tc.c | 115 +++++++++++++--------
> testsuites/sptests/Makefile.am | 1 +
> testsuites/sptests/configure.ac | 1 +
> testsuites/sptests/sptimecounter03/Makefile.am | 19 ++++
> testsuites/sptests/sptimecounter03/init.c | 110 ++++++++++++++++++++
> .../sptests/sptimecounter03/sptimecounter03.doc | 11 ++
> .../sptests/sptimecounter03/sptimecounter03.scn | 2 +
> 7 files changed, 216 insertions(+), 43 deletions(-)
> create mode 100644 testsuites/sptests/sptimecounter03/Makefile.am
> create mode 100644 testsuites/sptests/sptimecounter03/init.c
> create mode 100644 testsuites/sptests/sptimecounter03/sptimecounter03.doc
> create mode 100644 testsuites/sptests/sptimecounter03/sptimecounter03.scn
>
> diff --git a/cpukit/score/src/kern_tc.c b/cpukit/score/src/kern_tc.c
> index 5479927..6693fe8 100644
> --- a/cpukit/score/src/kern_tc.c
> +++ b/cpukit/score/src/kern_tc.c
> @@ -66,7 +66,8 @@ __FBSDID("$FreeBSD r277406 2015-01-20T03:54:30Z$");
> #include <sys/vdso.h>
> #endif /* __rtems__ */
> #ifdef __rtems__
> -#include <rtems.h>
> +#include <rtems/rtems/clock.h>
> +#include <rtems/score/atomic.h>
> ISR_LOCK_DEFINE(static, _Timecounter_Lock, "Timecounter");
> #define hz rtems_clock_get_ticks_per_second()
> #define printf(...)
> @@ -76,6 +77,16 @@ fls(int x)
> {
> return x ? sizeof(x) * 8 - __builtin_clz(x) : 0;
> }
> +static void
Any good reason none of the functions suggest inline?
> +rmb(void)
> +{
> + _Atomic_Fence(ATOMIC_ORDER_ACQUIRE);
> +}
> +static void
> +wmb(void)
> +{
> + _Atomic_Fence(ATOMIC_ORDER_RELEASE);
> +}
> /* FIXME: https://devel.rtems.org/ticket/2348 */
> #define ntp_update_second(a, b) do { (void) a; (void) b; } while (0)
> #endif /* __rtems__ */
> @@ -120,7 +131,7 @@ struct timehands {
> struct timeval th_microtime;
> struct timespec th_nanotime;
> /* Fields not to be copied in tc_windup start with th_generation. */
> - volatile uint32_t th_generation;
> + uint32_t th_generation;
> struct timehands *th_next;
> };
>
> @@ -252,6 +263,24 @@ tc_delta(struct timehands *th)
> tc->tc_counter_mask);
> }
>
> +static uint32_t
> +tc_getgen(struct timehands *th)
> +{
> + uint32_t gen;
> +
> + gen = th->th_generation;
> + rmb();
> + return (gen);
> +}
> +
> +static void
> +tc_setgen(struct timehands *th, uint32_t 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
> @@ -267,10 +296,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
> @@ -325,9 +354,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
> @@ -338,9 +367,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
> @@ -351,9 +380,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
> @@ -364,9 +393,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);
> }
>
> @@ -378,9 +407,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
> @@ -391,9 +420,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
> @@ -404,10 +433,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
> @@ -462,9 +491,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
> @@ -475,9 +504,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
> @@ -488,9 +517,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
> @@ -501,9 +530,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);
> }
>
> @@ -515,9 +544,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
> @@ -528,9 +557,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 */
>
> @@ -943,11 +972,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;
> }
> @@ -1052,9 +1081,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));
> }
> #endif /* __rtems__ */
>
> @@ -1096,7 +1125,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
> @@ -1110,7 +1139,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;
> #ifdef FFCLOCK
> @@ -1358,7 +1387,7 @@ tc_windup(void)
> tho = timehands;
> th = tho->th_next;
> ogen = th->th_generation;
> - th->th_generation = 0;
> + tc_setgen(th, 0);
> bcopy(tho, th, offsetof(struct timehands, th_generation));
>
> /*
> @@ -1479,7 +1508,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
> @@ -1727,13 +1756,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;
> }
>
> @@ -1753,7 +1782,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. */
> @@ -1803,7 +1832,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;
> @@ -1921,7 +1950,7 @@ _Timecounter_Tick_simple(uint32_t delta, uint32_t offset)
> */
> if (++ogen == 0)
> ogen = 1;
> - th->th_generation = ogen;
> + tc_setgen(th, ogen);
>
> /* Go live with the new struct timehands. */
> time_second = th->th_microtime.tv_sec;
> diff --git a/testsuites/sptests/Makefile.am b/testsuites/sptests/Makefile.am
> index c3fc443..948a8f0 100644
> --- a/testsuites/sptests/Makefile.am
> +++ b/testsuites/sptests/Makefile.am
> @@ -40,6 +40,7 @@ endif
> _SUBDIRS += spintrcritical23
> _SUBDIRS += sptimecounter01
> _SUBDIRS += sptimecounter02
> +_SUBDIRS += sptimecounter03
> _SUBDIRS += spatomic01
> _SUBDIRS += spintrcritical22
> _SUBDIRS += spsem03
> diff --git a/testsuites/sptests/configure.ac b/testsuites/sptests/configure.ac
> index b8287a4..a16ab3e 100644
> --- a/testsuites/sptests/configure.ac
> +++ b/testsuites/sptests/configure.ac
> @@ -43,6 +43,7 @@ AC_CONFIG_FILES([Makefile
> spintrcritical23/Makefile
> sptimecounter01/Makefile
> sptimecounter02/Makefile
> +sptimecounter03/Makefile
> spatomic01/Makefile
> spglobalcon01/Makefile
> spintrcritical22/Makefile
> diff --git a/testsuites/sptests/sptimecounter03/Makefile.am b/testsuites/sptests/sptimecounter03/Makefile.am
> new file mode 100644
> index 0000000..28209fa
> --- /dev/null
> +++ b/testsuites/sptests/sptimecounter03/Makefile.am
> @@ -0,0 +1,19 @@
> +rtems_tests_PROGRAMS = sptimecounter03
> +sptimecounter03_SOURCES = init.c
> +
> +dist_rtems_tests_DATA = sptimecounter03.scn sptimecounter03.doc
> +
> +include $(RTEMS_ROOT)/make/custom/@RTEMS_BSP at .cfg
> +include $(top_srcdir)/../automake/compile.am
> +include $(top_srcdir)/../automake/leaf.am
> +
> +AM_CPPFLAGS += -I$(top_srcdir)/../support/include
> +
> +LINK_OBJS = $(sptimecounter03_OBJECTS)
> +LINK_LIBS = $(sptimecounter03_LDLIBS)
> +
> +sptimecounter03$(EXEEXT): $(sptimecounter03_OBJECTS) $(sptimecounter03_DEPENDENCIES)
> + @rm -f sptimecounter03$(EXEEXT)
> + $(make-exe)
> +
> +include $(top_srcdir)/../automake/local.am
> diff --git a/testsuites/sptests/sptimecounter03/init.c b/testsuites/sptests/sptimecounter03/init.c
> new file mode 100644
> index 0000000..2595c7d
> --- /dev/null
> +++ b/testsuites/sptests/sptimecounter03/init.c
> @@ -0,0 +1,110 @@
> +/*
> + * Copyright (c) 2015 embedded brains GmbH. All rights reserved.
> + *
> + * embedded brains GmbH
> + * Dornierstr. 4
> + * 82178 Puchheim
> + * Germany
> + * <rtems at embedded-brains.de>
> + *
> + * The license and distribution terms for this file may be
> + * found in the file LICENSE in this distribution or at
> + * http://www.rtems.org/license/LICENSE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> + #include "config.h"
> +#endif
> +
> +#include <rtems/test.h>
> +
> +#include <rtems/bsd.h>
> +
> +#include <test_support.h>
> +
> +#include "tmacros.h"
> +
> +const char rtems_test_name[] = "SPTIMECOUNTER 3";
> +
> +#define CPU_COUNT 32
> +
> +static rtems_test_parallel_context ctx;;
> +
> +static rtems_interval test_binuptime_init(
> + rtems_test_parallel_context *ctx,
> + void *arg,
> + size_t active_workers
> +)
> +{
> + return 10 * rtems_clock_get_ticks_per_second();
> +}
> +
> +static void test_binuptime_body(
> + rtems_test_parallel_context *ctx,
> + void *arg,
> + size_t active_workers,
> + size_t worker_index
> +)
> +{
> + struct bintime start;
> + struct bintime end;
> +
> + rtems_bsd_binuptime(&start);
> +
> + do {
> + rtems_bsd_binuptime(&end);
> + rtems_test_assert(
> + end.sec > start.sec
> + || (end.sec == start.sec && end.frac >= start.frac)
> + );
> + start = end;
> + } while (!rtems_test_parallel_stop_job(ctx));
> +}
> +
> +static void test_binuptime_fini(
> + rtems_test_parallel_context *ctx,
> + void *arg,
> + size_t active_workers
> +)
> +{
> + /* Nothing to do */
> +}
> +
> +static const rtems_test_parallel_job jobs[] = {
> + {
> + .init = test_binuptime_init,
> + .body = test_binuptime_body,
> + .fini = test_binuptime_fini,
> + .cascade = false
> + }
> +};
> +
> +static void Init(rtems_task_argument arg)
> +{
> + TEST_BEGIN();
> +
> + rtems_test_parallel(&ctx, NULL, &jobs[0], RTEMS_ARRAY_SIZE(jobs));
> +
> + TEST_END();
> + rtems_test_exit(0);
> +}
> +
> +#define CONFIGURE_MICROSECONDS_PER_TICK 1000
> +
> +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
> +#define CONFIGURE_APPLICATION_NEEDS_CONSOLE_DRIVER
> +
> +#define CONFIGURE_MAXIMUM_TASKS CPU_COUNT
> +#define CONFIGURE_MAXIMUM_TIMERS 1
> +
> +#define CONFIGURE_SMP_APPLICATION
> +
> +#define CONFIGURE_SMP_MAXIMUM_PROCESSORS CPU_COUNT
> +
> +#define CONFIGURE_RTEMS_INIT_TASKS_TABLE
> +
> +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
> +
> +#define CONFIGURE_INIT
> +
> +#include <rtems/confdefs.h>
> diff --git a/testsuites/sptests/sptimecounter03/sptimecounter03.doc b/testsuites/sptests/sptimecounter03/sptimecounter03.doc
> new file mode 100644
> index 0000000..f978e56
> --- /dev/null
> +++ b/testsuites/sptests/sptimecounter03/sptimecounter03.doc
> @@ -0,0 +1,11 @@
> +This file describes the directives and concepts tested by this test set.
> +
> +test set name: sptimecounter03
> +
> +directives:
> +
> + - rtems_bsd_binuptime
> +
> +concepts:
> +
> + - Ensure that the binuptime is monotonic within a certain time frame.
> diff --git a/testsuites/sptests/sptimecounter03/sptimecounter03.scn b/testsuites/sptests/sptimecounter03/sptimecounter03.scn
> new file mode 100644
> index 0000000..fb249e5
> --- /dev/null
> +++ b/testsuites/sptests/sptimecounter03/sptimecounter03.scn
> @@ -0,0 +1,2 @@
> +*** BEGIN OF TEST SPTIMECOUNTER 3 ***
> +*** END OF TEST SPTIMECOUNTER 3 ***
> --
> 1.8.4.5
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list