Date: Tue, 09 Jun 2015 13:07:54 +0200 From: Sebastian Huber <sebastian.huber@embedded-brains.de> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-hackers@freebsd.org, phk@phk.freebsd.dk Subject: Re: [PATCH v3] timecounters: Fix timehand generation read/write Message-ID: <5576C90A.9010807@embedded-brains.de> In-Reply-To: <20150609105526.GK2499@kib.kiev.ua> References: <1433331966-27548-1-git-send-email-sebastian.huber@embedded-brains.de> <1433838715-22850-1-git-send-email-sebastian.huber@embedded-brains.de> <20150609105526.GK2499@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On 09/06/15 12:55, Konstantin Belousov wrote:
> On Tue, Jun 09, 2015 at 10:31:55AM +0200, Sebastian Huber wrote:
>> The compiler is free to re-order load/store instructions to non-volati=
le
>> 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.
> This looks fine. The only detail I changed is that I do not see a reas=
on
> to use atomics or barriers on UP machines. See the fragment at the end
> of the message for the exact diff.
You still need the compiler memory barriers in the UP case (at least=20
with GCC 4.9 on PowerPC), e.g. something like this:
static u_int
tc_getgen(struct timehands *th)
{
u_int gen;
gen =3D th->th_generation;
__compiler_membar();
return (gen);
}
static void
tc_setgen(struct timehands *th, u_int newgen)
{
__compiler_membar();
th->th_generation =3D newgen;
}
>
>> For the test the timehand count was reduced to one with 10000
>> tc_windup() calls per second. The timehand memory location was adjust=
ed
>> so that the th_generation field was on its own cache line.
> You mean, that the 'th_generation for its own cache line' was done for
> testing only ?
Yes, this was done for testing only. In case the timehand data is in one=20
cache line I don't think its possible to see the error case if no=20
barriers are present.
>
>> ---
>>
>> v2: Don't use tc_getgen() in tc_windup() since in the only writer ther=
e is no
>> need for a read memory barrier.
>>
>> v3: Use atomic load/store instead of explicit memory barriers.
>>
> If you do not have any comments, I will commit this version.
>
> diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c
> index 9dca0e8..fabfe03 100644
> --- a/sys/kern/kern_tc.c
> +++ b/sys/kern/kern_tc.c
> @@ -189,6 +190,28 @@ tc_delta(struct timehands *th)
> tc->tc_counter_mask);
> }
> =20
> +static u_int
> +tc_getgen(struct timehands *th)
> +{
> +
> +#ifdef SMP
> + return (atomic_load_acq_int(&th->th_generation));
> +#else
> + return (th->th_generation);
> +#endif
> +}
> +
> +static void
> +tc_setgen(struct timehands *th, u_int newgen)
> +{
> +
> +#ifdef SMP
> + atomic_store_rel_int(&th->th_generation, newgen);
> +#else
> + th->th_generation =3D newgen;
> +#endif
> +}
> +
> /*
> * 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
--=20
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.huber@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine gesch=E4ftliche Mitteilung im Sinne des EHUG.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5576C90A.9010807>