diff mbox series

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

Message ID 20210407054938.312857-3-ravi.bangoria@linux.ibm.com (mailing list archive)
State New
Headers show
Series powerpc/selftests: Add Power10 2nd DAWR selftests | expand

Commit Message

Ravi Bangoria April 7, 2021, 5:49 a.m. UTC
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@linux.ibm.com>
---
 .../selftests/powerpc/ptrace/perf-hwbreak.c   | 78 +++++++++----------
 1 file changed, 38 insertions(+), 40 deletions(-)

Comments

Daniel Axtens April 9, 2021, 7:19 a.m. UTC | #1
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@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.

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

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

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

Kind regards,
Daniel
Ravi Bangoria April 12, 2021, 5:03 a.m. UTC | #2
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@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
diff mbox series

Patch

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;
+	attr->exclude_user   = exclude_user;
+	attr->disabled       = 1;
 }
 
-static inline bool breakpoint_test(int len)
+static int
+perf_process_event_open_exclude_user(__u32 type, __u64 addr, __u64 len, bool exclude_user)
 {
 	struct perf_event_attr attr;
+
+	perf_event_attr_set(&attr, type, addr, len, exclude_user);
+	return syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
+}
+
+static int perf_process_event_open(__u32 type, __u64 addr, __u64 len)
+{
+	struct perf_event_attr attr;
+
+	perf_event_attr_set(&attr, type, addr, len, 0);
+	return syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
+}
+
+static inline bool breakpoint_test(int len)
+{
 	int fd;
 
-	/* setup counters */
-	memset(&attr, 0, sizeof(attr));
-	attr.disabled = 1;
-	attr.type = PERF_TYPE_BREAKPOINT;
-	attr.bp_type = HW_BREAKPOINT_R;
 	/* bp_addr can point anywhere but needs to be aligned */
-	attr.bp_addr = (__u64)(&attr) & 0xfffffffffffff800;
-	attr.bp_len = len;
-	fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
+	fd = perf_process_event_open(HW_BREAKPOINT_R, (__u64)(&fd) & 0xfffffffffffff800, len);
 	if (fd < 0)
 		return false;
 	close(fd);
@@ -75,7 +93,6 @@  static inline bool dawr_supported(void)
 static int runtestsingle(int readwriteflag, int exclude_user, int arraytest)
 {
 	int i,j;
-	struct perf_event_attr attr;
 	size_t res;
 	unsigned long long breaks, needed;
 	int readint;
@@ -94,19 +111,11 @@  static int runtestsingle(int readwriteflag, int exclude_user, int arraytest)
 	if (arraytest)
 		ptr = &readintalign[0];
 
-	/* 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);
 	if (break_fd < 0) {
-		perror("sys_perf_event_open");
+		perror("perf_process_event_open_exclude_user");
 		exit(1);
 	}
 
@@ -153,7 +162,6 @@  static int runtest_dar_outside(void)
 	void *target;
 	volatile __u16 temp16;
 	volatile __u64 temp64;
-	struct perf_event_attr attr;
 	int break_fd;
 	unsigned long long breaks;
 	int fail = 0;
@@ -165,21 +173,11 @@  static int runtest_dar_outside(void)
 		exit(EXIT_FAILURE);
 	}
 
-	/* setup counters */
-	memset(&attr, 0, sizeof(attr));
-	attr.disabled = 1;
-	attr.type = PERF_TYPE_BREAKPOINT;
-	attr.exclude_kernel = 1;
-	attr.exclude_hv = 1;
-	attr.exclude_guest = 1;
-	attr.bp_type = HW_BREAKPOINT_RW;
 	/* watch middle half of target array */
-	attr.bp_addr = (__u64)(target + 2);
-	attr.bp_len = 4;
-	break_fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
+	break_fd = perf_process_event_open(HW_BREAKPOINT_RW, (__u64)(target + 2), 4);
 	if (break_fd < 0) {
 		free(target);
-		perror("sys_perf_event_open");
+		perror("perf_process_event_open");
 		exit(EXIT_FAILURE);
 	}