Re: [PATCH v5 2/2] x86, mwaitt: introduce mwaix delay with a configurable timer

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jun 15, 2015 at 06:48:04PM +0800, Huang Rui wrote:
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index 1fbc89d..47f3540 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -14,6 +14,9 @@
>  #define CPUID5_ECX_INTERRUPT_BREAK	0x2
>  
>  #define MWAIT_ECX_INTERRUPT_BREAK	0x1
> +#define MWAITX_ECX_TIMER_ENABLE		BIT(1)
> +#define MWAITX_MAX_LOOPS		((u32)-1)
> +#define MWAITX_DISABLE_CSTATES		0xf
>  
>  static inline void __monitor(const void *eax, unsigned long ecx,
>  			     unsigned long edx)

Should this hunk not be part of the previous patch?

>  /*
> + * On AMD platforms mwaitx has a configurable 32-bit timer, that counts
> + * with TSC frequency. And the input value is the loop of the counter, it
> + * will exit with the timer expired.
> + */
> +static void delay_mwaitx(unsigned long __loops)
> +{
> +	u32 end, start, delay, loops = __loops;
> +
> +	rdtsc_barrier();
> +	rdtscl(start);
> +
> +	for (;;) {
> +		delay = min(MWAITX_MAX_LOOPS, loops);
> +
> +		/*
> +		 * Use cpu_tss as a cacheline-aligned, seldomly
> +		 * accessed per-cpu variable as the monitor target.
> +		 */
> +		__monitorx(this_cpu_ptr(&cpu_tss), 0, 0);
> +		/*
> +		 * AMD, like Intel, supports the EAX hint and EAX=0xf
> +		 * means, do not enter any deep C-state and we use it
> +		 * here in delay() to minimize wakeup latency.
> +		 */
> +		__mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE);
> +
> +		rdtsc_barrier();
> +		rdtscl(end);
> +
> +		if (loops <= end - start)
> +			break;
> +
> +		loops -= end - start;
> +
> +		start = end;
> +	}
> +}

OK, so what is not explained is how this delay is 'better' than the
TSC delay loop we currently have.

Seeing how we disable C states, its unlikely to use less energy, so what
exactly is its benefit, other than using fancy new instructions?