Date: Wed, 3 Jun 2015 17:56:06 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Sebastian Huber <sebastian.huber@embedded-brains.de> Cc: freebsd-hackers@freebsd.org, phk@phk.freebsd.dk Subject: Re: [PATCH v2] timecounters: Fix timehand generation read/write Message-ID: <20150603145606.GW2499@kib.kiev.ua> In-Reply-To: <1433332368-27645-1-git-send-email-sebastian.huber@embedded-brains.de> References: <1433331966-27548-1-git-send-email-sebastian.huber@embedded-brains.de> <1433332368-27645-1-git-send-email-sebastian.huber@embedded-brains.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jun 03, 2015 at 01:52:48PM +0200, Sebastian Huber 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.
> ---
>
> v2: Don't use tc_getgen() in tc_windup() since in the only writer there is no
> need for a read memory barrier.
>
> sys/kern/kern_tc.c | 100 +++++++++++++++++++++++++++++++----------------------
> 1 file changed, 59 insertions(+), 41 deletions(-)
>
> diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c
> index 9dca0e8..bb9614a 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);
Why not
return (atomic_load_acq_int(&th->th_generation);
?
> +}
> +
> +static void
> +tc_setgen(struct timehands *th, u_int newgen)
> +{
> +
> + wmb();
> + th->th_generation = newgen;
And there,
atomic_store_rel_int(&th->th_generation, newgen);
?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150603145606.GW2499>