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

[Waiman Long <waiman.long@hpe.com>]

On 04/09/2016 08:19 PM, Thomas Gleixner wrote:
> 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.

You are right. I should have used CPU instead.

>> 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?
>   

That is nothing wrong to enable it unconditionally. I am just not sure 
if that is the right thing to do. Since both you and Andy said we should 
enable it unconditionally, I will do so in the next version of the patch.

>> +/*
>>    * 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.

Will do so.

>> +	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

That part is leftover code from my testing and debugging effort. I think 
using local_irq_save() should allow the critical section to be executed 
without interruption. In this case, I should be able to remove the 
threshold checking code without harm.

Thanks for the review.

Cheers,
Longman