Re: [PATCH v2] x86/hpet: Reduce HPET counter read contention

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, 8 Apr 2016, Waiman Long wrote:
> This patch attempts to reduce HPET read contention by using the fact
> that if more than one task are trying to access HPET at the same time,
> it will be more efficient if one task in the group reads the HPET
> counter and shares it with the rest of the group instead of each
> group member reads the HPET counter individually.

That has nothing to do with tasks. clocksource reads can happen from almost
any context. The problem is concurrent access on multiple cpus.

> This optimization is enabled on systems with more than 32 CPUs. It can
> also be explicitly enabled or disabled by using the new opt_read_hpet
> kernel parameter.

Please not. What's wrong with enabling it unconditionally?
 
> +/*
>   * Clock source related code
>   */
>  static cycle_t read_hpet(struct clocksource *cs)
>  {
> -	return (cycle_t)hpet_readl(HPET_COUNTER);
> +	int seq, cnt = 0;
> +	u32 time;
> +
> +	if (opt_read_hpet <= 0)
> +		return (cycle_t)hpet_readl(HPET_COUNTER);

This wants to be conditional on CONFIG_SMP. No point in having all that muck
around for an UP kernel.

> +	seq = READ_ONCE(hpet_save.seq);
> +	if (!HPET_SEQ_LOCKED(seq)) {
> +		int old, new = seq + 1;
> +		unsigned long flags;
> +
> +		local_irq_save(flags);
> +		/*
> +		 * Set the lock bit (lsb) to get the right to read HPET
> +		 * counter directly. If successful, read the counter, save
> +		 * its value, and increment the sequence number. Otherwise,
> +		 * increment the sequnce number to the expected locked value
> +		 * for comparison later on.
> +		 */
> +		old = cmpxchg(&hpet_save.seq, seq, new);
> +		if (old == seq) {
> +			time = hpet_readl(HPET_COUNTER);
> +			WRITE_ONCE(hpet_save.hpet, time);
> +
> +			/* Unlock */
> +			smp_store_release(&hpet_save.seq, new + 1);
> +			local_irq_restore(flags);
> +			return (cycle_t)time;
> +		}
> +		local_irq_restore(flags);
> +		seq = new;
> +	}
> +
> +	/*
> +	 * 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();
> +
> +		if (likely(++cnt <= HPET_RESET_THRESHOLD))
> +			continue;
> +
> +		/*
> +		 * In the unlikely event that it takes too long for the lock
> +		 * holder to read the HPET, we do it ourselves and try to
> +		 * reset the lock. This will also break a deadlock if it
> +		 * happens, for example, when the process context lock holder
> +		 * gets killed in the middle of reading the HPET counter.
> +		 */
> +		time = hpet_readl(HPET_COUNTER);
> +		WRITE_ONCE(hpet_save.hpet, time);
> +		if (READ_ONCE(hpet_save.seq) == seq) {
> +			if (cmpxchg(&hpet_save.seq, seq, seq + 1) == seq)
> +				pr_warn("read_hpet: reset hpet seq to 0x%x\n",
> +					seq + 1);

This is voodoo programming and actively dangerous.

CPU0 	        CPU1	       		CPU2
lock_hpet()
T1=read_hpet()	wait_for_unlock()	
store_hpet(T1)	
		....			
		T2 = read_hpet()
unlock_hpet()				
					lock_hpet()
					T3 = read_hpet()
					store_hpet(T3)
					unlock_hpet()
					return T3
lock_hpet()
T4 = read_hpet()			wait_for_unlock()
store_hpet(T4)	
		store_hpet(T2)			
unlock_hpet()				return T2

CPU2 will observe time going backwards.

Thanks,

	tglx