Re: [Patch v2 12/24] perf/x86/intel: Allocate arch-PEBS buffer and initialize PEBS_BASE MSR

[Mi, Dapeng]

On 2/25/2025 7:18 PM, Peter Zijlstra wrote:
> On Tue, Feb 18, 2025 at 03:28:06PM +0000, Dapeng Mi wrote:
>> Arch-PEBS introduces a new MSR IA32_PEBS_BASE to store the arch-PEBS
>> buffer physical address. This patch allocates arch-PEBS buffer and then
>> initialize IA32_PEBS_BASE MSR with the buffer physical address.
> Not loving how this patch obscures the whole DS area thing and naming.

arch-PEBS uses a totally independent buffer to save the PEBS records and
don't use the debug store area anymore. To reuse the original function as
much as possible and don't mislead users to think arch-PEBS has some
relationship with debug store, the original key word "ds" in the function
names are changed to "BTS_PEBS". I know the name maybe not perfect, do you
have any suggestion? Thanks.


>
>
>> @@ -624,13 +604,18 @@ static int alloc_pebs_buffer(int cpu)
>>  	int max, node = cpu_to_node(cpu);
>>  	void *buffer, *insn_buff, *cea;
>>  
>> -	if (!x86_pmu.ds_pebs)
>> +	if (!intel_pmu_has_pebs())
>>  		return 0;
>>  
>> -	buffer = dsalloc_pages(bsiz, GFP_KERNEL, cpu);
>> +	buffer = dsalloc_pages(bsiz, preemptible() ? GFP_KERNEL : GFP_ATOMIC, cpu);
> But this plain smells bad, what is this about?

In initial implementation, alloc_pebs_buffer() could be called in 
init_debug_store_on_cpu() which may be in a irq context. But it was dropped
in latest implementation. So this change is not needed any more. Would drop
it in next version.


>
>>  	if (unlikely(!buffer))
>>  		return -ENOMEM;
>>  
>> +	if (x86_pmu.arch_pebs) {
>> +		hwev->pebs_vaddr = buffer;
>> +		return 0;
>> +	}
>> +
>>  	/*
>>  	 * HSW+ already provides us the eventing ip; no need to allocate this
>>  	 * buffer then.