Received-SPF: pass (google.com: domain of linux-kernel+bounces-120603-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <b204b992-8ad4-48d6-a0dd-d297ae23a5a2@linux.intel.com>
Date: Wed, 27 Mar 2024 17:18:01 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [kvm-unit-tests Patch v3 09/11] x86: pmu: Improve LLC misses
 event verification
Content-Language: en-US
To: Mingwei Zhang <mizhang@google.com>
Cc: Sean Christopherson <seanjc@google.com>,
 Paolo Bonzini <pbonzini@redhat.com>, Jim Mattson <jmattson@google.com>,
 kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
 Zhenyu Wang <zhenyuw@linux.intel.com>, Zhang Xiong
 <xiong.y.zhang@intel.com>, Like Xu <like.xu.linux@gmail.com>,
 Jinrong Liang <cloudliang@tencent.com>, Dapeng Mi <dapeng1.mi@intel.com>
References: <20240103031409.2504051-1-dapeng1.mi@linux.intel.com>
 <20240103031409.2504051-10-dapeng1.mi@linux.intel.com>
 <ZgO7Wr0URLc_ru1S@google.com>
From: "Mi, Dapeng" <dapeng1.mi@linux.intel.com>
In-Reply-To: <ZgO7Wr0URLc_ru1S@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 3/27/2024 2:23 PM, Mingwei Zhang wrote:
> On Wed, Jan 03, 2024, Dapeng Mi wrote:
>> When running pmu test on SPR, sometimes the following failure is
>> reported.
>>
>> 1 <= 0 <= 1000000
>> FAIL: Intel: llc misses-4
>>
>> Currently The LLC misses occurring only depends on probability. It's
>> possible that there is no LLC misses happened in the whole loop(),
>> especially along with processors have larger and larger cache size just
>> like what we observed on SPR.
>>
>> Thus, add clflush instruction into the loop() asm blob and ensure once
>> LLC miss is triggered at least.
>>
>> Suggested-by: Jim Mattson <jmattson@google.com>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> I wonder if we can skip all LLC tests if CPU does not have
> clflush/clflushopt properties?

Looks reasonable, the LLC miss event should be skipped at least if 
clflush/clflushopt are not supported since the validation can't grantee 
the correctness. But for LLC reference event, I think we can keep it 
since the asm_loop() would always 1 LLC reference at least.

If no disapproval, I would change code to skip LLC miss event if 
clflush/clflushopt are not supported in next version. Thanks.


>> ---
>>   x86/pmu.c | 43 ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/x86/pmu.c b/x86/pmu.c
>> index b764827c1c3d..8fd3db0fbf81 100644
>> --- a/x86/pmu.c
>> +++ b/x86/pmu.c
>> @@ -20,19 +20,21 @@
>>   
>>   // Instrustion number of LOOP_ASM code
>>   #define LOOP_INSTRNS	10
>> -#define LOOP_ASM					\
>> +#define LOOP_ASM(_clflush)				\
>> +	_clflush "\n\t"                                 \
>> +	"mfence;\n\t"                                   \
>>   	"1: mov (%1), %2; add $64, %1;\n\t"		\
>>   	"nop; nop; nop; nop; nop; nop; nop;\n\t"	\
>>   	"loop 1b;\n\t"
>>   
>> -/*Enable GLOBAL_CTRL + disable GLOBAL_CTRL instructions */
>> -#define PRECISE_EXTRA_INSTRNS  (2 + 4)
>> +/*Enable GLOBAL_CTRL + disable GLOBAL_CTRL + clflush/mfence instructions */
>> +#define PRECISE_EXTRA_INSTRNS  (2 + 4 + 2)
>>   #define PRECISE_LOOP_INSTRNS   (N * LOOP_INSTRNS + PRECISE_EXTRA_INSTRNS)
>>   #define PRECISE_LOOP_BRANCHES  (N)
>> -#define PRECISE_LOOP_ASM						\
>> +#define PRECISE_LOOP_ASM(_clflush)					\
>>   	"wrmsr;\n\t"							\
>>   	"mov %%ecx, %%edi; mov %%ebx, %%ecx;\n\t"			\
>> -	LOOP_ASM							\
>> +	LOOP_ASM(_clflush)						\
>>   	"mov %%edi, %%ecx; xor %%eax, %%eax; xor %%edx, %%edx;\n\t"	\
>>   	"wrmsr;\n\t"
>>   
>> @@ -72,14 +74,30 @@ char *buf;
>>   static struct pmu_event *gp_events;
>>   static unsigned int gp_events_size;
>>   
>> +#define _loop_asm(_clflush)					\
>> +do {								\
>> +	asm volatile(LOOP_ASM(_clflush)				\
>> +		     : "=c"(tmp), "=r"(tmp2), "=r"(tmp3)	\
>> +		     : "0"(N), "1"(buf));			\
>> +} while (0)
>> +
>> +#define _precise_loop_asm(_clflush)				\
>> +do {								\
>> +	asm volatile(PRECISE_LOOP_ASM(_clflush)			\
>> +		     : "=b"(tmp), "=r"(tmp2), "=r"(tmp3)	\
>> +		     : "a"(eax), "d"(edx), "c"(global_ctl),	\
>> +		       "0"(N), "1"(buf)				\
>> +		     : "edi");					\
>> +} while (0)
>>   
>>   static inline void __loop(void)
>>   {
>>   	unsigned long tmp, tmp2, tmp3;
>>   
>> -	asm volatile(LOOP_ASM
>> -		     : "=c"(tmp), "=r"(tmp2), "=r"(tmp3)
>> -		     : "0"(N), "1"(buf));
>> +	if (this_cpu_has(X86_FEATURE_CLFLUSH))
>> +		_loop_asm("clflush (%1)");
>> +	else
>> +		_loop_asm("nop");
>>   }
>>   
>>   /*
>> @@ -96,11 +114,10 @@ static inline void __precise_count_loop(u64 cntrs)
>>   	u32 eax = cntrs & (BIT_ULL(32) - 1);
>>   	u32 edx = cntrs >> 32;
>>   
>> -	asm volatile(PRECISE_LOOP_ASM
>> -		     : "=b"(tmp), "=r"(tmp2), "=r"(tmp3)
>> -		     : "a"(eax), "d"(edx), "c"(global_ctl),
>> -		       "0"(N), "1"(buf)
>> -		     : "edi");
>> +	if (this_cpu_has(X86_FEATURE_CLFLUSH))
>> +		_precise_loop_asm("clflush (%1)");
>> +	else
>> +		_precise_loop_asm("nop");
>>   }
>>   
>>   static inline void loop(u64 cntrs)
>> -- 
>> 2.34.1
>>