[PATCH 1/2] powerpc/perf/hv-24x7: Fix usage with chip events

[PATCH 1/2] powerpc/perf/hv-24x7: Fix usage with chip events

[Sukadev Bhattiprolu]

>From 9b5848ce1834a4d82fc251022035d36d9e26b500 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Sat, 23 Jan 2016 03:58:12 -0500
Subject: [PATCH 1/2] powerpc/perf/hv-24x7: Fix usage with chip events.

24x7 counters can belong to different domains (core, chip, virtual CPU
etc). For events in the 'chip' domain, sysfs entry currently looks like:

	$ cd /sys/bus/event_source/devices/hv_24x7/events
	$ cat PM_XLINK_CYCLES__PHYS_CHIP
	domain=0x1,offset=0x230,core=?,lpar=0x0

where the required parameter, 'core=?' is specified with perf as:

	perf stat -C 0 -e hv_24x7/PM_XLINK_CYCLES__PHYS_CHIP,core=1/ \
		/bin/true

This is inconsistent in that 'core' is a required parameter for a chip
event.  Instead, have the the sysfs entry display 'chip=?' for chip
events:

	$ cd /sys/bus/event_source/devices/hv_24x7/events
	$ cat PM_XLINK_CYCLES__PHYS_CHIP
	domain=0x1,offset=0x230,chip=?,lpar=0x0

We also need to add a 'chip' entry in the sysfs format directory:

	$ ls /sys/bus/event_source/devices/hv_24x7/format
	chip  core  domain  lpar  offset  vcpu
	^^^^
	(new)

so the perf tool can automatically check usage and format the chip
parameter correctly:

	$ perf stat -C 0 -v -e hv_24x7/PM_XLINK_CYCLES__PHYS_CHIP/ \
		/bin/true
	Required parameter 'chip' not specified
	invalid or unsupported event: 'hv_24x7/PM_XLINK_CYCLES__PHYS_CHIP/'

	$ perf stat -C 0 -v -e hv_24x7/PM_XLINK_CYCLES__PHYS_CHIP,chip=1/ \
		/bin/true
	hv_24x7/PM_XLINK_CYCLES__PHYS_CHIP,chip=1/: 0 6628908 6628908

	 Performance counter stats for 'CPU(s) 0':

	         0      hv_24x7/PM_XLINK_CYCLES__PHYS_CHIP,chip=1/

	    0.006606970 seconds time elapsed

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9f9dfda..b7a9a03 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -101,6 +101,7 @@ static bool catalog_entry_domain_is_valid(unsigned domain)
 EVENT_DEFINE_RANGE_FORMAT(domain, config, 0, 3);
 /* u16 */
 EVENT_DEFINE_RANGE_FORMAT(core, config, 16, 31);
+EVENT_DEFINE_RANGE_FORMAT(chip, config, 16, 31);
 EVENT_DEFINE_RANGE_FORMAT(vcpu, config, 16, 31);
 /* u32, see "data_offset" */
 EVENT_DEFINE_RANGE_FORMAT(offset, config, 32, 63);
@@ -115,6 +116,7 @@ static struct attribute *format_attrs[] = {
 	&format_attr_domain.attr,
 	&format_attr_offset.attr,
 	&format_attr_core.attr,
+	&format_attr_chip.attr,
 	&format_attr_vcpu.attr,
 	&format_attr_lpar.attr,
 	NULL,
@@ -289,10 +291,16 @@ static char *event_fmt(struct hv_24x7_event_data *event, unsigned domain)
 	const char *sindex;
 	const char *lpar;
 
-	if (is_physical_domain(domain)) {
+	switch (domain) {
+	case HV_PERF_DOMAIN_PHYS_CHIP:
+		lpar = "0x0";
+		sindex = "chip";
+		break;
+	case HV_PERF_DOMAIN_PHYS_CORE:
 		lpar = "0x0";
 		sindex = "core";
-	} else {
+		break;
+	default:
 		lpar = "?";
 		sindex = "vcpu";
 	}
@@ -1089,10 +1097,16 @@ static int add_event_to_24x7_request(struct perf_event *event,
 		return -EINVAL;
 	}
 
-	if (is_physical_domain(event_get_domain(event)))
+	switch (event_get_domain(event)) {
+	case HV_PERF_DOMAIN_PHYS_CHIP:
+		idx = event_get_chip(event);
+		break;
+	case HV_PERF_DOMAIN_PHYS_CORE:
 		idx = event_get_core(event);
-	else
+		break;
+	default:
 		idx = event_get_vcpu(event);
+	}
 
 	i = request_buffer->num_requests++;
 	req = &request_buffer->requests[i];
-- 
2.5.3

[PATCH 2/2] powerpc/perf/hv-24x7: Display change in counter values

[Sukadev Bhattiprolu]

>From a1aa992fb25fb8e98a5c5724376ae8cc91463de3 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Mon, 25 Jan 2016 23:05:36 -0500
Subject: [PATCH 2/2] powerpc/perf/hv-24x7: Display change in counter values

For 24x7 counters, perf displays the raw value of the 24x7 counter, which
is a monotonically increasing value.

	perf stat -C 0 -e \
		'hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/' \
		sleep 1

 Performance counter stats for 'CPU(s) 0':

     9,105,403,170      hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/

       0.000425751 seconds time elapsed

In the typical usage of 'perf stat' this counter value is not as useful
as the _change_ in the counter value over the duration of the application.

Have h_24x7_event_init() set the event's prev_count to the raw value of
the 24x7 counter at the time of initialization. When the application
terminates, hv_24x7_event_read() will compute the change in value and
report to the perf tool. Similarly, for the transaction interface, clear
the event count to 0 at the beginning of the transaction.

	perf stat -C 0 -e \
		'hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/' \
		sleep 1

 Performance counter stats for 'CPU(s) 0':

           245,758      hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/

       1.006366383 seconds time elapsed

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index b7a9a03..77b958f 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1222,11 +1222,12 @@ static int h_24x7_event_init(struct perf_event *event)
 		return -EACCES;
 	}
 
-	/* see if the event complains */
+	/* Get the initial value of the counter for this event */
 	if (single_24x7_request(event, &ct)) {
 		pr_devel("test hcall failed\n");
 		return -EIO;
 	}
+	(void)local64_xchg(&event->hw.prev_count, ct);
 
 	return 0;
 }
@@ -1289,6 +1290,16 @@ static void h_24x7_event_read(struct perf_event *event)
 			h24x7hw = &get_cpu_var(hv_24x7_hw);
 			h24x7hw->events[i] = event;
 			put_cpu_var(h24x7hw);
+			/*
+			 * Clear the event count so we can compute the _change_
+			 * in the 24x7 raw counter value at the end of the txn.
+			 *
+			 * Note that we could alternatively read the 24x7 value
+			 * now and save its value in event->hw.prev_count. But
+			 * that would require issuing a hcall, which would then
+			 * defeat the purpose of using the txn interface.
+			 */
+			local64_set(&event->count, 0);
 		}
 
 		put_cpu_var(hv_24x7_reqb);
-- 
2.5.3

Re: [PATCH 2/2] powerpc/perf/hv-24x7: Display change in counter values

[Madhavan Srinivasan]

On Saturday 30 January 2016 08:37 AM, Sukadev Bhattiprolu wrote:
> From a1aa992fb25fb8e98a5c5724376ae8cc91463de3 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Mon, 25 Jan 2016 23:05:36 -0500
> Subject: [PATCH 2/2] powerpc/perf/hv-24x7: Display change in counter values
>
> For 24x7 counters, perf displays the raw value of the 24x7 counter, which
> is a monotonically increasing value.
>
> 	perf stat -C 0 -e \
> 		'hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/' \
> 		sleep 1
>
>  Performance counter stats for 'CPU(s) 0':
>
>      9,105,403,170      hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/
>
>        0.000425751 seconds time elapsed
>
> In the typical usage of 'perf stat' this counter value is not as useful
> as the _change_ in the counter value over the duration of the application.

This may break application using this interface right? i.e, since
for all this time, counter output was raw values and application
may be post processing to calculate the difference, now with
this patch, application may need some change? Also,
should not this be documented somewhere?

Maddy

> Have h_24x7_event_init() set the event's prev_count to the raw value of
> the 24x7 counter at the time of initialization. When the application
> terminates, hv_24x7_event_read() will compute the change in value and
> report to the perf tool. Similarly, for the transaction interface, clear
> the event count to 0 at the beginning of the transaction.
>
> 	perf stat -C 0 -e \
> 		'hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/' \
> 		sleep 1
>
>  Performance counter stats for 'CPU(s) 0':
>
>            245,758      hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/
>
>        1.006366383 seconds time elapsed
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
>  arch/powerpc/perf/hv-24x7.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index b7a9a03..77b958f 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -1222,11 +1222,12 @@ static int h_24x7_event_init(struct perf_event *event)
>  		return -EACCES;
>  	}
>  
> -	/* see if the event complains */
> +	/* Get the initial value of the counter for this event */
>  	if (single_24x7_request(event, &ct)) {
>  		pr_devel("test hcall failed\n");
>  		return -EIO;
>  	}
> +	(void)local64_xchg(&event->hw.prev_count, ct);
>  
>  	return 0;
>  }
> @@ -1289,6 +1290,16 @@ static void h_24x7_event_read(struct perf_event *event)
>  			h24x7hw = &get_cpu_var(hv_24x7_hw);
>  			h24x7hw->events[i] = event;
>  			put_cpu_var(h24x7hw);
> +			/*
> +			 * Clear the event count so we can compute the _change_
> +			 * in the 24x7 raw counter value at the end of the txn.
> +			 *
> +			 * Note that we could alternatively read the 24x7 value
> +			 * now and save its value in event->hw.prev_count. But
> +			 * that would require issuing a hcall, which would then
> +			 * defeat the purpose of using the txn interface.
> +			 */
> +			local64_set(&event->count, 0);
>  		}
>  
>  		put_cpu_var(hv_24x7_reqb);

Re: [PATCH 2/2] powerpc/perf/hv-24x7: Display change in counter values

[Sukadev Bhattiprolu]

Madhavan Srinivasan [maddy@linux.vnet.ibm.com] wrote:
> 
> 
> On Saturday 30 January 2016 08:37 AM, Sukadev Bhattiprolu wrote:
> > From a1aa992fb25fb8e98a5c5724376ae8cc91463de3 Mon Sep 17 00:00:00 2001
> > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Date: Mon, 25 Jan 2016 23:05:36 -0500
> > Subject: [PATCH 2/2] powerpc/perf/hv-24x7: Display change in counter values
> >
> > For 24x7 counters, perf displays the raw value of the 24x7 counter, which
> > is a monotonically increasing value.
> >
> > 	perf stat -C 0 -e \
> > 		'hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/' \
> > 		sleep 1
> >
> >  Performance counter stats for 'CPU(s) 0':
> >
> >      9,105,403,170      hv_24x7/HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE,core=1/
> >
> >        0.000425751 seconds time elapsed
> >
> > In the typical usage of 'perf stat' this counter value is not as useful
> > as the _change_ in the counter value over the duration of the application.
> 
> This may break application using this interface right? i.e, since
> for all this time, counter output was raw values and application
> may be post processing to calculate the difference, now with
> this patch, application may need some change? Also,
> should not this be documented somewhere?

Agree that it does change the behavior. I am checking to see if it
was explicitly documented that the values would be raw. But current
behavior seems counter-intuitive and inconsistent with 'perf stat'.

If we run something like:

	perf stat -C 0 -e <24x7-event> make

we see the large number (raw value of the counter) when the application
terminates. The raw value not very useful. To effectively use the counter
in this scenario, user would ahve to run:

	perf stat -C 0 -e <24x7-event> sleep 1
	#note raw value 1

	perf stat -C 0 -e <24x7-event> make
	# note raw value 2

	# compute diff of value2 and value1.

Reporting the change in value seems to be consistent with normal usage of
perf stat with events like cycles or instructions:

Thanks,

Sukadev

Re: [1/2] powerpc/perf/hv-24x7: Fix usage with chip events

[Michael Ellerman]

On Sat, 2016-30-01 at 03:07:10 UTC, Sukadev Bhattiprolu wrote:
> >From 9b5848ce1834a4d82fc251022035d36d9e26b500 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Sat, 23 Jan 2016 03:58:12 -0500
> Subject: [PATCH 1/2] powerpc/perf/hv-24x7: Fix usage with chip events.
> 
> 24x7 counters can belong to different domains (core, chip, virtual CPU
> etc). For events in the 'chip' domain, sysfs entry currently looks like:
> 
> 	$ cd /sys/bus/event_source/devices/hv_24x7/events
> 	$ cat PM_XLINK_CYCLES__PHYS_CHIP
> 	domain=0x1,offset=0x230,core=?,lpar=0x0
...

> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e5a5886d7ae32b7afebfffecca

cheers