[PATCH v2 2/4] powerpc/selftests/perf-hwbreak: Coalesce event creation code

[Ravi Bangoria]

On 4/9/21 12:49 PM, Daniel Axtens wrote:
> Hi Ravi,
> 
>> perf-hwbreak selftest opens hw-breakpoint event at multiple places for
>> which it has same code repeated. Coalesce that code into a function.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria at linux.ibm.com>
>> ---
>>   .../selftests/powerpc/ptrace/perf-hwbreak.c   | 78 +++++++++----------
> 
> This doesn't simplify things very much for the code as it stands now,
> but I think your next patch adds a bunch of calls to these functions, so
> I agree that it makes sense to consolidate them now.

Right. This helps in next patch.

>>   1 file changed, 38 insertions(+), 40 deletions(-)
>>
>> diff --git a/tools/testing/selftests/powerpc/ptrace/perf-hwbreak.c b/tools/testing/selftests/powerpc/ptrace/perf-hwbreak.c
>> index c1f324afdbf3..bde475341c8a 100644
>> --- a/tools/testing/selftests/powerpc/ptrace/perf-hwbreak.c
>> +++ b/tools/testing/selftests/powerpc/ptrace/perf-hwbreak.c
>> @@ -34,28 +34,46 @@
>>   
>>   #define DAWR_LENGTH_MAX ((0x3f + 1) * 8)
>>   
>> -static inline int sys_perf_event_open(struct perf_event_attr *attr, pid_t pid,
>> -				      int cpu, int group_fd,
>> -				      unsigned long flags)
>> +static void perf_event_attr_set(struct perf_event_attr *attr,
>> +				__u32 type, __u64 addr, __u64 len,
>> +				bool exclude_user)
>>   {
>> -	attr->size = sizeof(*attr);
>> -	return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
>> +	memset(attr, 0, sizeof(struct perf_event_attr));
>> +	attr->type           = PERF_TYPE_BREAKPOINT;
>> +	attr->size           = sizeof(struct perf_event_attr);
>> +	attr->bp_type        = type;
>> +	attr->bp_addr        = addr;
>> +	attr->bp_len         = len;
>> +	attr->exclude_kernel = 1;
>> +	attr->exclude_hv     = 1;
>> +	attr->exclude_guest  = 1;
> 
> Only 1 of the calls to perf sets exclude_{kernel,hv,guest} - I assume
> there's no issue with setting them always but I wanted to check.

True. there is no issue in setting them as this test does all
load/stores in userspace. So all events will be recorded
irrespective of settings of exclude_{kernel,hv,guest}.

> 
>> +	attr->exclude_user   = exclude_user;
>> +	attr->disabled       = 1;
>>   }
>>   
>> -	/* setup counters */
>> -	memset(&attr, 0, sizeof(attr));
>> -	attr.disabled = 1;
>> -	attr.type = PERF_TYPE_BREAKPOINT;
>> -	attr.bp_type = readwriteflag;
>> -	attr.bp_addr = (__u64)ptr;
>> -	attr.bp_len = sizeof(int);
>> -	if (arraytest)
>> -		attr.bp_len = DAWR_LENGTH_MAX;
>> -	attr.exclude_user = exclude_user;
>> -	break_fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
>> +	break_fd = perf_process_event_open_exclude_user(readwriteflag, (__u64)ptr,
>> +				arraytest ? DAWR_LENGTH_MAX : sizeof(int),
>> +				exclude_user);
> 
> checkpatch doesn't like this very much:
> 
> CHECK: Alignment should match open parenthesis
> #103: FILE: tools/testing/selftests/powerpc/ptrace/perf-hwbreak.c:115:
> +	break_fd = perf_process_event_open_exclude_user(readwriteflag, (__u64)ptr,
> +				arraytest ? DAWR_LENGTH_MAX : sizeof(int),

Sure will fix it.

> 
> Apart from that, this seems good but I haven't checked in super fine
> detail just yet :)

Thanks for the review.
-Ravi