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

[Daniel Axtens <dja@axtens.net>]

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