On 08/10/2016 11:29 AM, Waiman Long wrote:
> +static cycle_t read_hpet(struct clocksource *cs)
> +{
> + int seq;
> +
> + seq = READ_ONCE(hpet_save.seq);
> + if (!HPET_SEQ_LOCKED(seq)) {
...
> + }
> +
> + /*
> + * Wait until the locked sequence number changes which indicates
> + * that the saved HPET value is up-to-date.
> + */
> + while (READ_ONCE(hpet_save.seq) == seq) {
> + /*
> + * Since reading the HPET is much slower than a single
> + * cpu_relax() instruction, we use two here in an attempt
> + * to reduce the amount of cacheline contention in the
> + * hpet_save.seq cacheline.
> + */
> + cpu_relax();
> + cpu_relax();
> + }
> +
> + return (cycle_t)READ_ONCE(hpet_save.hpet);
> +}
It's a real bummer that this all has to be open-coded. I have to wonder
if there were any alternatives that you tried that were simpler.
Is READ_ONCE()/smp_store_release() really strong enough here? It
guarantees ordering, but you need ordering *and* a guarantee that your
write is visible to the reader. Don't you need actual barriers for
that? Otherwise, you might be seeing a stale HPET value, and the spin
loop that you did waiting for it to be up-to-date was worthless. The
seqlock code, uses barriers, btw.
Also, since you're fundamentally reading a second-hand HPET value, does
that have any impact on the precision of the HPET as a timesource? Or,
is it so coarse already that this isn't an issue?
