[PATCH v3] timecounters: Fix timehand generation read/write

[Sebastian Huber]

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-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.
> This looks fine.  The only detail I changed is that I do not see a reason
> 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 
with GCC 4.9 on PowerPC), e.g. something like this:

static u_int
tc_getgen(struct timehands *th)
{
     u_int gen;

     gen = th->th_generation;
     __compiler_membar();
     return (gen);
}

static void
tc_setgen(struct timehands *th, u_int newgen)
{

     __compiler_membar();
     th->th_generation = newgen;
}

>
>> 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.
> 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 
cache line I don't think its possible to see the error case if no 
barriers are present.

>
>> ---
>>
>> v2: Don't use tc_getgen() in tc_windup() since in the only writer there 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);
>   }
>   
> +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 = 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

-- 
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 at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.