[PATCH v2 00/10] powerpc/hv-24x7: Reorganize single_24x7_request()

[PATCH v2 00/10] 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.

Changelog[v2]
	- Split independent changes in patch 2 into patch 3.
	- Minor changes to pr_err() (Patch 4).
	- Move whitespace changes to separate patch (Patch  8).
	- Minor function renames (Patch 10).

Sukadev Bhattiprolu (10):
  powerpc/hv-24x7: Modify definition of request and result buffers
  powerpc/hv24x7: Remove unnecessary parameter
  perf/hv24x7: Use pr_devel() to log message
  powerpc/hv-24x7: Drop event_24x7_request()
  powerpc/hv24x7: Move debug prints to separate function
  powerpc/hv-24x7: Rename hv_24x7_event_update
  powerpc/hv-24x7: Define add_event_to_24x7_request()
  perf/hv24x7: Whitespace cleanup
  powerpc/hv-24x7: Define update_event_count()
  powerpc/hv-24x7: Break up single_24x7_request

 arch/powerpc/perf/hv-24x7.c | 212 +++++++++++++++++++++++++++-----------------
 arch/powerpc/perf/hv-24x7.h |   8 +-
 2 files changed, 137 insertions(+), 83 deletions(-)

-- 
1.8.3.1

[PATCH v2 01/10] 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 v2 02/10] powerpc/hv24x7: Remove unnecessary parameter

[Sukadev Bhattiprolu]

Remove the 'success_expected' parameter and log the message unconditionally.

Changelog[v2]
	[Michael Ellerman]: Move the change to reduce log message priority
		into a separate patch.

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

[PATCH v2 03/10] perf/hv24x7: Use pr_devel() to log message

[Sukadev Bhattiprolu]

Use pr_devel_ratelimited() to log error message when the 24x7 HCALL
fails. Since users specify events by their sysfs name, the HCALL should
succeed. Any errors reported by the HCALL would be of interest to the
developer, rather than the user/administrator.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index c185dcf..87c9905 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1027,7 +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) {
-		pr_err_ratelimited("hcall failed: %d %#x %#x %d => "
+		pr_devel_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,
-- 
1.8.3.1

[PATCH v2 04/10] 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.

Changelog [v2]:
	- Michael Ellerman: Use fields from the request structure rather
		than re-extracting from event structure.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 87c9905..f509f3b 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,8 @@ static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
 	if (ret) {
 		pr_devel_ratelimited("hcall failed: %d %#x %#x %d => "
 				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
-				domain, offset, ix, lpar, ret, ret,
+				req->performance_domain, req->data_offset,
+				idx, req->starting_lpar_ix, ret, ret,
 				result_buffer->detailed_rc,
 				result_buffer->failing_request_ix);
 		goto out;
@@ -1042,22 +1048,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 +1116,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 +1128,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

[PATCH v2 05/10] 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>

Conflicts:
	arch/powerpc/perf/hv-24x7.c
---
 arch/powerpc/perf/hv-24x7.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index f509f3b..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,12 +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_devel_ratelimited("hcall failed: %d %#x %#x %d => "
-				"0x%lx (%ld) detail=0x%x failing ix=%x\n",
-				req->performance_domain, req->data_offset,
-				idx, req->starting_lpar_ix, 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 v2 06/10] 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 v2 07/10] 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().

Changelog[v2]
	[Michael Ellerman]: Move white-space changes to separate patch.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index e78b127..fe74221 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
-- 
1.8.3.1

[PATCH v2 08/10] perf/hv24x7: Whitespace cleanup

[Sukadev Bhattiprolu]

Fix minor whitespace damages.

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

diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index fe74221..676fb2f9 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -161,6 +161,7 @@ static char *event_desc(struct hv_24x7_event_data *ev, int *len)
 {
 	unsigned nl = be16_to_cpu(ev->event_name_len);
 	__be16 *desc_len = (__be16 *)(ev->remainder + nl - 2);
+
 	*len = be16_to_cpu(*desc_len) - 2;
 	return (char *)ev->remainder + nl;
 }
@@ -171,6 +172,7 @@ static char *event_long_desc(struct hv_24x7_event_data *ev, int *len)
 	__be16 *desc_len_ = (__be16 *)(ev->remainder + nl - 2);
 	unsigned desc_len = be16_to_cpu(*desc_len_);
 	__be16 *long_desc_len = (__be16 *)(ev->remainder + nl + desc_len - 2);
+
 	*len = be16_to_cpu(*long_desc_len) - 2;
 	return (char *)ev->remainder + nl + desc_len;
 }
@@ -248,14 +250,12 @@ static unsigned long h_get_24x7_catalog_page_(unsigned long phys_4096,
 					      unsigned long index)
 {
 	pr_devel("h_get_24x7_catalog_page(0x%lx, %lu, %lu)",
-			phys_4096,
-			version,
-			index);
+			phys_4096, version, index);
+
 	WARN_ON(!IS_ALIGNED(phys_4096, 4096));
+
 	return plpar_hcall_norets(H_GET_24X7_CATALOG_PAGE,
-			phys_4096,
-			version,
-			index);
+			phys_4096, version, index);
 }
 
 static unsigned long h_get_24x7_catalog_page(char page[],
@@ -309,6 +309,7 @@ static ssize_t device_show_string(struct device *dev,
 	struct dev_ext_attribute *d;
 
 	d = container_of(attr, struct dev_ext_attribute, attr);
+
 	return sprintf(buf, "%s\n", (char *)d->var);
 }
 
@@ -323,6 +324,7 @@ static struct attribute *device_str_attr_create_(char *name, char *str)
 	attr->attr.attr.name = name;
 	attr->attr.attr.mode = 0444;
 	attr->attr.show = device_show_string;
+
 	return &attr->attr.attr;
 }
 
@@ -396,7 +398,6 @@ static struct attribute *event_to_attr(unsigned ix,
 		a_ev_name = kasprintf(GFP_KERNEL, "%.*s%s__%d",
 				(int)event_name_len, ev_name, ev_suffix, nonce);
 
-
 	if (!a_ev_name)
 		goto out_val;
 
@@ -881,6 +882,7 @@ static ssize_t catalog_read(struct file *filp, struct kobject *kobj,
 	uint64_t catalog_version_num = 0;
 	void *page = kmem_cache_alloc(hv_page_cache, GFP_USER);
 	struct hv_24x7_catalog_page_0 *page_0 = page;
+
 	if (!page)
 		return -ENOMEM;
 
@@ -1077,7 +1079,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;
@@ -1175,6 +1176,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: [PATCH v2 08/10] perf/hv24x7: Whitespace cleanup

[Joe Perches]

On Mon, 2015-03-30 at 18:53 -0700, Sukadev Bhattiprolu wrote:
> Fix minor whitespace damages.

If you are going to do whitespace cleaning,
please verify the patches with scripts/checkpatch.pl --strict.

> diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
[]
> @@ -1077,7 +1079,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;

Does this deletion make the code easier to read?

It might be better just to use:

	be64 val;
	...
	val = result_buffer->results[0].elements[0].element_data[0];
	*count = be64_to_cpu(val);

Re: [PATCH v2 08/10] perf/hv24x7: Whitespace cleanup

[Sukadev Bhattiprolu]

Joe Perches [joe@perches.com] wrote:
| On Mon, 2015-03-30 at 18:53 -0700, Sukadev Bhattiprolu wrote:
| > Fix minor whitespace damages.
| 
| If you are going to do whitespace cleaning,
| please verify the patches with scripts/checkpatch.pl --strict.

Ok. Will fix up parameter alignment with the open parens in a
follow on patch.

| 
| > diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
| []
| > @@ -1077,7 +1079,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;
| 
| Does this deletion make the code easier to read?
| 
| It might be better just to use:
| 
| 	be64 val;
| 	...
| 	val = result_buffer->results[0].elements[0].element_data[0];
| 	*count = be64_to_cpu(val);

Agree. Will include in a follow on patch.

Sukadev

[PATCH v2 09/10] 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 676fb2f9..cf82026 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1172,16 +1172,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 v2 10/10] 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.

Changelog[v2]:
	[Michael Ellerman] Rename start_24x7_get_data() to init_24x7_request()
		and commit_24x7_get_data make_24x7_request().

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 cf82026..46be032 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -1003,6 +1003,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 init_24x7_request(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 'init_24x7_request()' and 'add_event_to_24x7_request()'.
+ */
+static int make_24x7_request(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'
@@ -1044,7 +1082,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;
@@ -1055,31 +1092,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;
+	init_24x7_request(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 = make_24x7_request(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