[PATCH 0/9] powerpc/hv-24x7: Reorganize single_24x7_request()

[PATCH 0/9] powerpc/hv-24x7: Reorganize single_24x7_request()

[Sukadev Bhattiprolu]

We currently issue a new hcall for to retrieve the value of each 24x7
counter that we want to read.  However, the H_GET_24x7_DATA hcall can
retrieve several counters in a single call, which would be useful in
getting a more consistent snapshot of the system.

Reorganize the code that prepares a 24x7 hcall request, submits it and
processes the result to allow reading seveal counters at once. We still
submit a fresh hcall for each event for now. A follow-on patch-set will
build on this to submit multiple perf_events in a single hcall.

Thanks to Peter Zijlstra for his input.

Sukadev Bhattiprolu (9):
  powerpc/perf: hv-24x7: Modify definition of request and result buffers
  powerpc: perf/hv24x7: Remove unnecessary parameter
  powerpc: perf/hv-24x7: Drop event_24x7_request()
  powerpc: perf/hv24x7: Move debug prints to separate function
  perf: powerpc/hv-24x7: Rename hv_24x7_event_update
  powerpc: perf/hv-24x7: Define add_event_to_24x7_request()
  powerpc: perf/hv-24x7: Define update_event_count()
  powerpc: perf/hv-24x7: Break up single_24x7_request
  powerpc: perf/hv-24x7: Add missing put_cpu_var()

 arch/powerpc/perf/hv-24x7.c | 198 ++++++++++++++++++++++++++++----------------
 arch/powerpc/perf/hv-24x7.h |   8 +-
 2 files changed, 130 insertions(+), 76 deletions(-)

-- 
1.8.3.1

[PATCH 1/9] powerpc/hv-24x7: Modify definition of request and result buffers

[Sukadev Bhattiprolu]

The parameters to the 24x7 HCALL have variable number of elements in them.
Set the minimum number of such elements to 1 rather than 0 and eliminate
the temporary structures.

This would enable us to submit multiple counter requests and process
multiple results from a single HCALL (in a follow on patch).

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 arch/powerpc/perf/hv-24x7.c | 77 ++++++++++++++++++++++-----------------------
 arch/powerpc/perf/hv-24x7.h |  8 ++---
 2 files changed, 41 insertions(+), 44 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 9445a82..408e6e9 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -142,6 +142,15 @@ static struct attribute_group event_long_desc_group = {
 
 static struct kmem_cache *hv_page_cache;
 
+/*
+ * request_buffer and result_buffer are not required to be 4k aligned,
+ * but are not allowed to cross any 4k boundary. Aligning them to 4k is
+ * the simplest way to ensure that.
+ */
+#define H24x7_DATA_BUFFER_SIZE	4096
+DEFINE_PER_CPU(char, hv_24x7_reqb[H24x7_DATA_BUFFER_SIZE]) __aligned(4096);
+DEFINE_PER_CPU(char, hv_24x7_resb[H24x7_DATA_BUFFER_SIZE]) __aligned(4096);
+
 static char *event_name(struct hv_24x7_event_data *ev, int *len)
 {
 	*len = be16_to_cpu(ev->event_name_len) - 2;
@@ -976,31 +985,16 @@ static const struct attribute_group *attr_groups[] = {
 	NULL,
 };
 
-DEFINE_PER_CPU(char, hv_24x7_reqb[4096]) __aligned(4096);
-DEFINE_PER_CPU(char, hv_24x7_resb[4096]) __aligned(4096);
-
 static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
-					 u16 lpar, u64 *res,
+					 u16 lpar, u64 *count,
 					 bool success_expected)
 {
 	unsigned long ret;
 
-	/*
-	 * request_buffer and result_buffer are not required to be 4k aligned,
-	 * but are not allowed to cross any 4k boundary. Aligning them to 4k is
-	 * the simplest way to ensure that.
-	 */
-	struct reqb {
-		struct hv_24x7_request_buffer buf;
-		struct hv_24x7_request req;
-	} __packed *request_buffer;
-
-	struct {
-		struct hv_24x7_data_result_buffer buf;
-		struct hv_24x7_result res;
-		struct hv_24x7_result_element elem;
-		__be64 result;
-	} __packed *result_buffer;
+	struct hv_24x7_request_buffer *request_buffer;
+	struct hv_24x7_data_result_buffer *result_buffer;
+	struct hv_24x7_result *resb;
+	struct hv_24x7_request *req;
 
 	BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
 	BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
@@ -1011,38 +1005,41 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 	memset(request_buffer, 0, 4096);
 	memset(result_buffer, 0, 4096);
 
-	*request_buffer = (struct reqb) {
-		.buf = {
-			.interface_version = HV_24X7_IF_VERSION_CURRENT,
-			.num_requests = 1,
-		},
-		.req = {
-			.performance_domain = domain,
-			.data_size = cpu_to_be16(8),
-			.data_offset = cpu_to_be32(offset),
-			.starting_lpar_ix = cpu_to_be16(lpar),
-			.max_num_lpars = cpu_to_be16(1),
-			.starting_ix = cpu_to_be16(ix),
-			.max_ix = cpu_to_be16(1),
-		}
-	};
+	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
+	request_buffer->num_requests = 1;
+
+	req = &request_buffer->requests[0];
 
+	req->performance_domain = domain;
+	req->data_size = cpu_to_be16(8);
+	req->data_offset = cpu_to_be32(offset);
+	req->starting_lpar_ix = cpu_to_be16(lpar),
+	req->max_num_lpars = cpu_to_be16(1);
+	req->starting_ix = cpu_to_be16(ix);
+	req->max_ix = cpu_to_be16(1);
+
+	/*
+	 * NOTE: Due to variable number of array elements in request and
+	 *	 result buffer(s), sizeof() is not reliable. Use the actual
+	 *	 allocated buffer size, H24x7_DATA_BUFFER_SIZE.
+	 */
 	ret = plpar_hcall_norets(H_GET_24X7_DATA,
-			virt_to_phys(request_buffer), sizeof(*request_buffer),
-			virt_to_phys(result_buffer),  sizeof(*result_buffer));
+			virt_to_phys(request_buffer), H24x7_DATA_BUFFER_SIZE,
+			virt_to_phys(result_buffer),  H24x7_DATA_BUFFER_SIZE);
 
 	if (ret) {
 		if (success_expected)
 			pr_err_ratelimited("hcall failed: %d %#x %#x %d => "
 				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
 				domain, offset, ix, lpar, ret, ret,
-				result_buffer->buf.detailed_rc,
-				result_buffer->buf.failing_request_ix);
+				result_buffer->detailed_rc,
+				result_buffer->failing_request_ix);
 		goto out;
 	}
 
-	*res = be64_to_cpu(result_buffer->result);
+	resb = &result_buffer->results[0];
 
+	*count = be64_to_cpu(resb->elements[0].element_data[0]);
 out:
 	return ret;
 }
diff --git a/arch/powerpc/perf/hv-24x7.h b/arch/powerpc/perf/hv-24x7.h
index 69cd4e6..0f9fa21 100644
--- a/arch/powerpc/perf/hv-24x7.h
+++ b/arch/powerpc/perf/hv-24x7.h
@@ -50,7 +50,7 @@ struct hv_24x7_request_buffer {
 	__u8 interface_version;
 	__u8 num_requests;
 	__u8 reserved[0xE];
-	struct hv_24x7_request requests[];
+	struct hv_24x7_request requests[1];
 } __packed;
 
 struct hv_24x7_result_element {
@@ -66,7 +66,7 @@ struct hv_24x7_result_element {
 	__be32 lpar_cfg_instance_id;
 
 	/* size = @result_element_data_size of cointaining result. */
-	__u8 element_data[];
+	__u64 element_data[1];
 } __packed;
 
 struct hv_24x7_result {
@@ -87,7 +87,7 @@ struct hv_24x7_result {
 	/* WARNING: only valid for first result element due to variable sizes
 	 *          of result elements */
 	/* struct hv_24x7_result_element[@num_elements_returned] */
-	struct hv_24x7_result_element elements[];
+	struct hv_24x7_result_element elements[1];
 } __packed;
 
 struct hv_24x7_data_result_buffer {
@@ -103,7 +103,7 @@ struct hv_24x7_data_result_buffer {
 	__u8 reserved2[0x8];
 	/* WARNING: only valid for the first result due to variable sizes of
 	 *	    results */
-	struct hv_24x7_result results[]; /* [@num_results] */
+	struct hv_24x7_result results[1]; /* [@num_results] */
 } __packed;
 
 #endif
-- 
1.8.3.1

[PATCH 2/9] powerpc/hv24x7: Remove unnecessary parameter

[Sukadev Bhattiprolu]

Use pr_notice_ratelimited() to log error messages and remove
the 'success_expected' parameter.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 408e6e9..7856e38 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -986,8 +986,7 @@ static const struct attribute_group *attr_groups[] = {
 };
 
 static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
-					 u16 lpar, u64 *count,
-					 bool success_expected)
+					 u16 lpar, u64 *count)
 {
 	unsigned long ret;
 
@@ -1028,8 +1027,7 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 			virt_to_phys(result_buffer),  H24x7_DATA_BUFFER_SIZE);
 
 	if (ret) {
-		if (success_expected)
-			pr_err_ratelimited("hcall failed: %d %#x %#x %d => "
+		pr_notice_ratelimited("hcall failed: %d %#x %#x %d => "
 				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
 				domain, offset, ix, lpar, ret, ret,
 				result_buffer->detailed_rc,
@@ -1044,8 +1042,7 @@ out:
 	return ret;
 }
 
-static unsigned long event_24x7_request(struct perf_event *event, u64 *res,
-		bool success_expected)
+static unsigned long event_24x7_request(struct perf_event *event, u64 *res)
 {
 	u16 idx;
 	unsigned domain = event_get_domain(event);
@@ -1059,8 +1056,7 @@ static unsigned long event_24x7_request(struct perf_event *event, u64 *res,
 				event_get_offset(event),
 				idx,
 				event_get_lpar(event),
-				res,
-				success_expected);
+				res);
 }
 
 static int h_24x7_event_init(struct perf_event *event)
@@ -1130,7 +1126,7 @@ static int h_24x7_event_init(struct perf_event *event)
 	}
 
 	/* see if the event complains */
-	if (event_24x7_request(event, &ct, false)) {
+	if (event_24x7_request(event, &ct)) {
 		pr_devel("test hcall failed\n");
 		return -EIO;
 	}
@@ -1142,7 +1138,7 @@ static u64 h_24x7_get_value(struct perf_event *event)
 {
 	unsigned long ret;
 	u64 ct;
-	ret = event_24x7_request(event, &ct, true);
+	ret = event_24x7_request(event, &ct);
 	if (ret)
 		/* We checked this in event init, shouldn't fail here... */
 		return 0;
-- 
1.8.3.1

Re: [2/9] powerpc/hv24x7: Remove unnecessary parameter

[Michael Ellerman]

On Tue, 2015-17-02 at 22:00:27 UTC, Sukadev Bhattiprolu wrote:
> Use pr_notice_ratelimited() to log error messages and remove
> the 'success_expected' parameter.

I don't understand how this is equivalent?

The current code uses success_expected to indicate that once it's done the
request once and found that it works, it then expects the request to continue
working, and if it doesn't then that is an error.

Using pr_ratelimited() will do the opposite, ie. the first failure will print a
message, but that may not really indicate an error, it may just be a badly
configured request.

Or at least that's how I understand it, please convince me I'm wrong :)

cheers

Re: [2/9] powerpc/hv24x7: Remove unnecessary parameter

[Sukadev Bhattiprolu]

Michael Ellerman [mpe@ellerman.id.au] wrote:
| On Tue, 2015-17-02 at 22:00:27 UTC, Sukadev Bhattiprolu wrote:
| > Use pr_notice_ratelimited() to log error messages and remove
| > the 'success_expected' parameter.
| 
| I don't understand how this is equivalent?

They are two unrelated changes that I should have separated.
| 
| The current code uses success_expected to indicate that once it's done the
| request once and found that it works, it then expects the request to continue
| working, and if it doesn't then that is an error.

The current code is using success_expected to _not_ log an error if
that initial request fails. i.e we silently return -EIO here.

I think the 'success_expected' parameter is not really necessary.
We can simply log the message even for that initial request.

And we can log it a lower priority than KERN_ERR since the message
is mostly for developers rather than users who would use event names
(which encode/abstract the domain and offset values).

| 
| Using pr_ratelimited() will do the opposite, ie. the first failure will print a
| message, but that may not really indicate an error, it may just be a badly
| configured request.
| 
| Or at least that's how I understand it, please convince me I'm wrong :)
| 
| cheers
| _______________________________________________
| Linuxppc-dev mailing list
| Linuxppc-dev@lists.ozlabs.org
| https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 3/9] powerpc/hv-24x7: Drop event_24x7_request()

[Sukadev Bhattiprolu]

The function event_24x7_request() is essentially a wrapper to the
function single_24x7_request() and can be dropped to simplify code.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 7856e38..c189e75 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -985,9 +985,9 @@ static const struct attribute_group *attr_groups[] = {
 	NULL,
 };
 
-static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
-					 u16 lpar, u64 *count)
+static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 {
+	u16 idx;
 	unsigned long ret;
 
 	struct hv_24x7_request_buffer *request_buffer;
@@ -1004,17 +1004,22 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 	memset(request_buffer, 0, 4096);
 	memset(result_buffer, 0, 4096);
 
+	if (is_physical_domain(event_get_domain(event)))
+		idx = event_get_core(event);
+	else
+		idx = event_get_vcpu(event);
+
 	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
 	request_buffer->num_requests = 1;
 
 	req = &request_buffer->requests[0];
 
-	req->performance_domain = domain;
+	req->performance_domain = event_get_domain(event);
 	req->data_size = cpu_to_be16(8);
-	req->data_offset = cpu_to_be32(offset);
-	req->starting_lpar_ix = cpu_to_be16(lpar),
+	req->data_offset = cpu_to_be32(event_get_offset(event));
+	req->starting_lpar_ix = cpu_to_be16(event_get_lpar(event)),
 	req->max_num_lpars = cpu_to_be16(1);
-	req->starting_ix = cpu_to_be16(ix);
+	req->starting_ix = cpu_to_be16(idx);
 	req->max_ix = cpu_to_be16(1);
 
 	/*
@@ -1029,7 +1034,9 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 	if (ret) {
 		pr_notice_ratelimited("hcall failed: %d %#x %#x %d => "
 				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
-				domain, offset, ix, lpar, ret, ret,
+				(int)event_get_domain(event),
+				(unsigned int)event_get_offset(event),
+				idx, (int)event_get_lpar(event), ret, ret,
 				result_buffer->detailed_rc,
 				result_buffer->failing_request_ix);
 		goto out;
@@ -1042,22 +1049,6 @@ out:
 	return ret;
 }
 
-static unsigned long event_24x7_request(struct perf_event *event, u64 *res)
-{
-	u16 idx;
-	unsigned domain = event_get_domain(event);
-
-	if (is_physical_domain(domain))
-		idx = event_get_core(event);
-	else
-		idx = event_get_vcpu(event);
-
-	return single_24x7_request(event_get_domain(event),
-				event_get_offset(event),
-				idx,
-				event_get_lpar(event),
-				res);
-}
 
 static int h_24x7_event_init(struct perf_event *event)
 {
@@ -1126,7 +1117,7 @@ static int h_24x7_event_init(struct perf_event *event)
 	}
 
 	/* see if the event complains */
-	if (event_24x7_request(event, &ct)) {
+	if (single_24x7_request(event, &ct)) {
 		pr_devel("test hcall failed\n");
 		return -EIO;
 	}
@@ -1138,7 +1129,7 @@ static u64 h_24x7_get_value(struct perf_event *event)
 {
 	unsigned long ret;
 	u64 ct;
-	ret = event_24x7_request(event, &ct);
+	ret = single_24x7_request(event, &ct);
 	if (ret)
 		/* We checked this in event init, shouldn't fail here... */
 		return 0;
-- 
1.8.3.1

Re: [3/9] powerpc/hv-24x7: Drop event_24x7_request()

[Michael Ellerman]

On Tue, 2015-17-02 at 22:00:28 UTC, Sukadev Bhattiprolu wrote:
> The function event_24x7_request() is essentially a wrapper to the
> function single_24x7_request() and can be dropped to simplify code.
> 
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 7856e38..c189e75 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -1004,17 +1004,22 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
>  	memset(request_buffer, 0, 4096);
>  	memset(result_buffer, 0, 4096);
>  
> +	if (is_physical_domain(event_get_domain(event)))
> +		idx = event_get_core(event);
> +	else
> +		idx = event_get_vcpu(event);
> +
>  	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
>  	request_buffer->num_requests = 1;
>  
>  	req = &request_buffer->requests[0];
>  
> -	req->performance_domain = domain;
> +	req->performance_domain = event_get_domain(event);
>  	req->data_size = cpu_to_be16(8);
> -	req->data_offset = cpu_to_be32(offset);
> -	req->starting_lpar_ix = cpu_to_be16(lpar),
> +	req->data_offset = cpu_to_be32(event_get_offset(event));
> +	req->starting_lpar_ix = cpu_to_be16(event_get_lpar(event)),
>  	req->max_num_lpars = cpu_to_be16(1);
> -	req->starting_ix = cpu_to_be16(ix);
> +	req->starting_ix = cpu_to_be16(idx);
>  	req->max_ix = cpu_to_be16(1);
>  
>  	/*
> @@ -1029,7 +1034,9 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
>  	if (ret) {
>  		pr_notice_ratelimited("hcall failed: %d %#x %#x %d => "
>  				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
> -				domain, offset, ix, lpar, ret, ret,
> +				(int)event_get_domain(event),
> +				(unsigned int)event_get_offset(event),
> +				idx, (int)event_get_lpar(event), ret, ret,

It seems more natural here to print the req->performance_domain etc. rather
then re-extracting them from the event?

cheers

[PATCH 4/9] powerpc/hv24x7: Move debug prints to separate function

[Sukadev Bhattiprolu]

To simplify/cleanup code, move the rather long printk() to a separate
function.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index c189e75..a58a1df 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -985,6 +985,21 @@ static const struct attribute_group *attr_groups[] = {
 	NULL,
 };
 
+static void log_24x7_hcall(struct hv_24x7_request_buffer *request_buffer,
+			struct hv_24x7_data_result_buffer *result_buffer,
+			unsigned long ret)
+{
+	struct hv_24x7_request *req;
+
+	req = &request_buffer->requests[0];
+	pr_notice_ratelimited("hcall failed: [%d %#x %#x %d] => "
+			"ret 0x%lx (%ld) detail=0x%x failing ix=%x\n",
+			req->performance_domain, req->data_offset,
+			req->starting_ix, req->starting_lpar_ix, ret, ret,
+			result_buffer->detailed_rc,
+			result_buffer->failing_request_ix);
+}
+
 static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 {
 	u16 idx;
@@ -1032,13 +1047,7 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 			virt_to_phys(result_buffer),  H24x7_DATA_BUFFER_SIZE);
 
 	if (ret) {
-		pr_notice_ratelimited("hcall failed: %d %#x %#x %d => "
-				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
-				(int)event_get_domain(event),
-				(unsigned int)event_get_offset(event),
-				idx, (int)event_get_lpar(event), ret, ret,
-				result_buffer->detailed_rc,
-				result_buffer->failing_request_ix);
+		log_24x7_hcall(request_buffer, result_buffer, ret);
 		goto out;
 	}
 
-- 
1.8.3.1

[PATCH 5/9] powerpc/hv-24x7: Rename hv_24x7_event_update

[Sukadev Bhattiprolu]

For consistency with the pmu operation ->read() and with other
pmus, rename hv_24x7_event_update() to hv_24x7_event_read().

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index a58a1df..e78b127 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1146,7 +1146,7 @@ static u64 h_24x7_get_value(struct perf_event *event)
 	return ct;
 }
 
-static void h_24x7_event_update(struct perf_event *event)
+static void h_24x7_event_read(struct perf_event *event)
 {
 	s64 prev;
 	u64 now;
@@ -1163,7 +1163,7 @@ static void h_24x7_event_start(struct perf_event *event, int flags)
 
 static void h_24x7_event_stop(struct perf_event *event, int flags)
 {
-	h_24x7_event_update(event);
+	h_24x7_event_read(event);
 }
 
 static int h_24x7_event_add(struct perf_event *event, int flags)
@@ -1184,7 +1184,7 @@ static struct pmu h_24x7_pmu = {
 	.del         = h_24x7_event_stop,
 	.start       = h_24x7_event_start,
 	.stop        = h_24x7_event_stop,
-	.read        = h_24x7_event_update,
+	.read        = h_24x7_event_read,
 };
 
 static int hv_24x7_init(void)
-- 
1.8.3.1

[PATCH 6/9] powerpc/hv-24x7: Define add_event_to_24x7_request()

[Sukadev Bhattiprolu]

Move code that maps a perf_event to a 24x7 request buffer into a
separate function, add_event_to_24x7_request().

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index e78b127..76c649a 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1000,15 +1000,52 @@ static void log_24x7_hcall(struct hv_24x7_request_buffer *request_buffer,
 			result_buffer->failing_request_ix);
 }
 
-static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
+/*
+ * Add the given @event to the next slot in the 24x7 request_buffer.
+ *
+ * Note that H_GET_24X7_DATA hcall allows reading several counters'
+ * values in a single HCALL. We expect the caller to add events to the
+ * request buffer one by one, make the HCALL and process the results.
+ */
+static int add_event_to_24x7_request(struct perf_event *event,
+				struct hv_24x7_request_buffer *request_buffer)
 {
 	u16 idx;
+	int i;
+	struct hv_24x7_request *req;
+
+	if (request_buffer->num_requests > 254) {
+		pr_devel("Too many requests for 24x7 HCALL %d\n",
+				request_buffer->num_requests);
+		return -EINVAL;
+	}
+
+	if (is_physical_domain(event_get_domain(event)))
+		idx = event_get_core(event);
+	else
+		idx = event_get_vcpu(event);
+
+	i = request_buffer->num_requests++;
+	req = &request_buffer->requests[i];
+
+	req->performance_domain = event_get_domain(event);
+	req->data_size = cpu_to_be16(8);
+	req->data_offset = cpu_to_be32(event_get_offset(event));
+	req->starting_lpar_ix = cpu_to_be16(event_get_lpar(event)),
+	req->max_num_lpars = cpu_to_be16(1);
+	req->starting_ix = cpu_to_be16(idx);
+	req->max_ix = cpu_to_be16(1);
+
+	return 0;
+}
+
+static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
+{
 	unsigned long ret;
 
 	struct hv_24x7_request_buffer *request_buffer;
 	struct hv_24x7_data_result_buffer *result_buffer;
 	struct hv_24x7_result *resb;
-	struct hv_24x7_request *req;
 
 	BUILD_BUG_ON(sizeof(*request_buffer) > 4096);
 	BUILD_BUG_ON(sizeof(*result_buffer) > 4096);
@@ -1019,23 +1056,11 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 	memset(request_buffer, 0, 4096);
 	memset(result_buffer, 0, 4096);
 
-	if (is_physical_domain(event_get_domain(event)))
-		idx = event_get_core(event);
-	else
-		idx = event_get_vcpu(event);
-
 	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
-	request_buffer->num_requests = 1;
 
-	req = &request_buffer->requests[0];
-
-	req->performance_domain = event_get_domain(event);
-	req->data_size = cpu_to_be16(8);
-	req->data_offset = cpu_to_be32(event_get_offset(event));
-	req->starting_lpar_ix = cpu_to_be16(event_get_lpar(event)),
-	req->max_num_lpars = cpu_to_be16(1);
-	req->starting_ix = cpu_to_be16(idx);
-	req->max_ix = cpu_to_be16(1);
+	ret = add_event_to_24x7_request(event, request_buffer);
+	if (ret)
+		return ret;
 
 	/*
 	 * NOTE: Due to variable number of array elements in request and
@@ -1052,7 +1077,6 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 	}
 
 	resb = &result_buffer->results[0];
-
 	*count = be64_to_cpu(resb->elements[0].element_data[0]);
 out:
 	return ret;
@@ -1150,6 +1174,7 @@ static void h_24x7_event_read(struct perf_event *event)
 {
 	s64 prev;
 	u64 now;
+
 	now = h_24x7_get_value(event);
 	prev = local64_xchg(&event->hw.prev_count, now);
 	local64_add(now - prev, &event->count);
-- 
1.8.3.1

Re: [6/9] powerpc/hv-24x7: Define add_event_to_24x7_request()

[Michael Ellerman]

On Tue, 2015-17-02 at 22:00:31 UTC, Sukadev Bhattiprolu wrote:
> Move code that maps a perf_event to a 24x7 request buffer into a
> separate function, add_event_to_24x7_request().
> 
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index e78b127..76c649a 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -1052,7 +1077,6 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
>  	}
>  
>  	resb = &result_buffer->results[0];
> -
>  	*count = be64_to_cpu(resb->elements[0].element_data[0]);
>  out:
>  	return ret;
> @@ -1150,6 +1174,7 @@ static void h_24x7_event_read(struct perf_event *event)
>  {
>  	s64 prev;
>  	u64 now;
> +
>  	now = h_24x7_get_value(event);
>  	prev = local64_xchg(&event->hw.prev_count, now);
>  	local64_add(now - prev, &event->count);

I'm a fan of whitespace for readability in cases like this, but do it as a
separate patch.

cheers

[PATCH 7/9] powerpc/hv-24x7: Define update_event_count()

[Sukadev Bhattiprolu]

Move the code to update an event count into a new function,
update_event_count().

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 76c649a..3c36694 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1170,16 +1170,22 @@ static u64 h_24x7_get_value(struct perf_event *event)
 	return ct;
 }
 
-static void h_24x7_event_read(struct perf_event *event)
+static void update_event_count(struct perf_event *event, u64 now)
 {
 	s64 prev;
-	u64 now;
 
-	now = h_24x7_get_value(event);
 	prev = local64_xchg(&event->hw.prev_count, now);
 	local64_add(now - prev, &event->count);
 }
 
+static void h_24x7_event_read(struct perf_event *event)
+{
+	u64 now;
+
+	now = h_24x7_get_value(event);
+	update_event_count(event, now);
+}
+
 static void h_24x7_event_start(struct perf_event *event, int flags)
 {
 	if (flags & PERF_EF_RELOAD)
-- 
1.8.3.1

[PATCH 8/9] powerpc/hv-24x7: Break up single_24x7_request

[Sukadev Bhattiprolu]

Break up the function single_24x7_request() into smaller functions.
This would later enable us to "prepare" a multi-event request
buffer and then submit a single hcall for several events.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 3c36694..fde6211 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1001,6 +1001,44 @@ static void log_24x7_hcall(struct hv_24x7_request_buffer *request_buffer,
 }
 
 /*
+ * Start the process for a new H_GET_24x7_DATA hcall.
+ */
+static void start_24x7_get_data(struct hv_24x7_request_buffer *request_buffer,
+			struct hv_24x7_data_result_buffer *result_buffer)
+{
+
+	memset(request_buffer, 0, 4096);
+	memset(result_buffer, 0, 4096);
+
+	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
+	/* memset above set request_buffer->num_requests to 0 */
+}
+
+/*
+ * Commit (i.e perform) the H_GET_24x7_DATA hcall using the data collected
+ * by 'start_24x7_get_data()' and 'add_event_to_24x7_request()'.
+ */
+static int commit_24x7_get_data(struct hv_24x7_request_buffer *request_buffer,
+			struct hv_24x7_data_result_buffer *result_buffer)
+{
+	unsigned long ret;
+
+	/*
+	 * NOTE: Due to variable number of array elements in request and
+	 *	 result buffer(s), sizeof() is not reliable. Use the actual
+	 *	 allocated buffer size, H24x7_DATA_BUFFER_SIZE.
+	 */
+	ret = plpar_hcall_norets(H_GET_24X7_DATA,
+			virt_to_phys(request_buffer), H24x7_DATA_BUFFER_SIZE,
+			virt_to_phys(result_buffer),  H24x7_DATA_BUFFER_SIZE);
+
+	if (ret)
+		log_24x7_hcall(request_buffer, result_buffer, ret);
+
+	return ret;
+}
+
+/*
  * Add the given @event to the next slot in the 24x7 request_buffer.
  *
  * Note that H_GET_24X7_DATA hcall allows reading several counters'
@@ -1042,7 +1080,6 @@ static int add_event_to_24x7_request(struct perf_event *event,
 static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 {
 	unsigned long ret;
-
 	struct hv_24x7_request_buffer *request_buffer;
 	struct hv_24x7_data_result_buffer *result_buffer;
 	struct hv_24x7_result *resb;
@@ -1053,31 +1090,22 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 	request_buffer = (void *)get_cpu_var(hv_24x7_reqb);
 	result_buffer = (void *)get_cpu_var(hv_24x7_resb);
 
-	memset(request_buffer, 0, 4096);
-	memset(result_buffer, 0, 4096);
-
-	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
+	start_24x7_get_data(request_buffer, result_buffer);
 
 	ret = add_event_to_24x7_request(event, request_buffer);
 	if (ret)
 		return ret;
 
-	/*
-	 * NOTE: Due to variable number of array elements in request and
-	 *	 result buffer(s), sizeof() is not reliable. Use the actual
-	 *	 allocated buffer size, H24x7_DATA_BUFFER_SIZE.
-	 */
-	ret = plpar_hcall_norets(H_GET_24X7_DATA,
-			virt_to_phys(request_buffer), H24x7_DATA_BUFFER_SIZE,
-			virt_to_phys(result_buffer),  H24x7_DATA_BUFFER_SIZE);
-
+	ret = commit_24x7_get_data(request_buffer, result_buffer);
 	if (ret) {
 		log_24x7_hcall(request_buffer, result_buffer, ret);
 		goto out;
 	}
 
+	/* process result from hcall */
 	resb = &result_buffer->results[0];
 	*count = be64_to_cpu(resb->elements[0].element_data[0]);
+
 out:
 	return ret;
 }
-- 
1.8.3.1

Re: [8/9] powerpc/hv-24x7: Break up single_24x7_request

[Michael Ellerman]

On Tue, 2015-17-02 at 22:00:33 UTC, Sukadev Bhattiprolu wrote:
> Break up the function single_24x7_request() into smaller functions.
> This would later enable us to "prepare" a multi-event request
> buffer and then submit a single hcall for several events.

This looks fine, though the names are a bit laboured.
> 
> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
> index 3c36694..fde6211 100644
> --- a/arch/powerpc/perf/hv-24x7.c
> +++ b/arch/powerpc/perf/hv-24x7.c
> @@ -1001,6 +1001,44 @@ static void log_24x7_hcall(struct hv_24x7_request_buffer *request_buffer,
>  }
>  
>  /*
> + * Start the process for a new H_GET_24x7_DATA hcall.
> + */
> +static void start_24x7_get_data(struct hv_24x7_request_buffer *request_buffer,
> +			struct hv_24x7_data_result_buffer *result_buffer)
> +{

Just init_24x7_request() ?

> +
> +	memset(request_buffer, 0, 4096);
> +	memset(result_buffer, 0, 4096);
> +
> +	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
> +	/* memset above set request_buffer->num_requests to 0 */
> +}
> +
> +/*
> + * Commit (i.e perform) the H_GET_24x7_DATA hcall using the data collected
> + * by 'start_24x7_get_data()' and 'add_event_to_24x7_request()'.
> + */
> +static int commit_24x7_get_data(struct hv_24x7_request_buffer *request_buffer,
> +			struct hv_24x7_data_result_buffer *result_buffer)
> +{

It don't like "commit" that is a loaded term.

Just make_24x7_request() perhaps?


cheers

Re: [8/9] powerpc/hv-24x7: Break up single_24x7_request

[Sukadev Bhattiprolu]

Michael Ellerman [mpe@ellerman.id.au] wrote:
| > +static void start_24x7_get_data(struct hv_24x7_request_buffer *request_buffer,
| > +			struct hv_24x7_data_result_buffer *result_buffer)
| > +{
| 
| Just init_24x7_request() ?

Sure.

| 
| > +
| > +	memset(request_buffer, 0, 4096);
| > +	memset(result_buffer, 0, 4096);
| > +
| > +	request_buffer->interface_version = HV_24X7_IF_VERSION_CURRENT;
| > +	/* memset above set request_buffer->num_requests to 0 */
| > +}
| > +
| > +/*
| > + * Commit (i.e perform) the H_GET_24x7_DATA hcall using the data collected
| > + * by 'start_24x7_get_data()' and 'add_event_to_24x7_request()'.
| > + */
| > +static int commit_24x7_get_data(struct hv_24x7_request_buffer *request_buffer,
| > +			struct hv_24x7_data_result_buffer *result_buffer)
| > +{
| 
| It don't like "commit" that is a loaded term.
| 
| Just make_24x7_request() perhaps?

Sure.

Thanks

Sukadev

[PATCH 9/9] powerpc/hv-24x7: Add missing put_cpu_var()

[Sukadev Bhattiprolu]

Add missing put_cpu_var() for 24x7 requests.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index fde6211..7d578d6 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1094,7 +1094,7 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 
 	ret = add_event_to_24x7_request(event, request_buffer);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = commit_24x7_get_data(request_buffer, result_buffer);
 	if (ret) {
@@ -1107,6 +1107,8 @@ static unsigned long single_24x7_request(struct perf_event *event, u64 *count)
 	*count = be64_to_cpu(resb->elements[0].element_data[0]);
 
 out:
+	put_cpu_var(hv_24x7_reqb);
+	put_cpu_var(hv_24x7_resb);
 	return ret;
 }
 
-- 
1.8.3.1

Re: [9/9] powerpc/hv-24x7: Add missing put_cpu_var()

[Michael Ellerman]

On Tue, 2015-17-02 at 22:00:34 UTC, Sukadev Bhattiprolu wrote:
> Add missing put_cpu_var() for 24x7 requests.

When did it go missing? I assume in upstream, in which case this should be a
separate patch which I could merge for 4.0.

cheers

Re: [9/9] powerpc/hv-24x7: Add missing put_cpu_var()

[Sukadev Bhattiprolu]

Michael Ellerman [mpe@ellerman.id.au] wrote:
| On Tue, 2015-17-02 at 22:00:34 UTC, Sukadev Bhattiprolu wrote:
| > Add missing put_cpu_var() for 24x7 requests.
| 
| When did it go missing? I assume in upstream, in which case this should be a
| separate patch which I could merge for 4.0.

It went missing in 3.18-rc3 (commit f34b6c7). I will resend this
separately.

Sukadev