[PATCH 0/3] IMC code clean up and refactoring

[PATCH 0/3] IMC code clean up and refactoring

[Anju T Sudhakar]

The first patch removes the unused variable in the code for 
IMC(In-memory collection counters).

The second patch does some code refactoring.

The third patch in the series make struct imc_events as a parameter to the
function imc_parse_event().

Anju T Sudhakar (3):
  powerpc/perf: Remove thread_imc_pmu global variable from
	imc code
  powerpc/perf: IMC code cleanup with some code refactoring
  powerpc/perf: Passs struct imc_events as a parameter to
    imc_parse_event()

 arch/powerpc/include/asm/imc-pmu.h |  2 +-
 arch/powerpc/perf/imc-pmu.c        | 99 +++++++++++++++++++++++---------------
 2 files changed, 61 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH 1/3] powerpc/perf: Remove thread_imc_pmu global variable from

[Anju T Sudhakar]

Remove the global variable 'thread_imc_pmu', since it is not used in the code.

Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
---
 arch/powerpc/perf/imc-pmu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 4eb9e2b..ef7f9dd 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -40,7 +40,6 @@ static struct imc_pmu *core_imc_pmu;
 /* Thread IMC data structures and variables */
 
 static DEFINE_PER_CPU(u64 *, thread_imc_mem);
-static struct imc_pmu *thread_imc_pmu;
 static int thread_imc_mem_size;
 
 struct imc_pmu *imc_event_to_pmu(struct perf_event *event)
@@ -1263,7 +1262,6 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
 				return res;
 		}
 
-		thread_imc_pmu = pmu_ptr;
 		break;
 	default:
 		return -EINVAL;
-- 
2.7.4

Re: [PATCH 1/3] powerpc/perf: Remove thread_imc_pmu global variable from

[Madhavan Srinivasan]

On Monday 11 December 2017 11:28 AM, Anju T Sudhakar wrote:
> Remove the global variable 'thread_imc_pmu', since it is not used in the code.

Reviewed-by: madhavan Srinivasan <maddy@linux.vnet.ibm.com>

Maddy

> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
>   arch/powerpc/perf/imc-pmu.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 4eb9e2b..ef7f9dd 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -40,7 +40,6 @@ static struct imc_pmu *core_imc_pmu;
>   /* Thread IMC data structures and variables */
>
>   static DEFINE_PER_CPU(u64 *, thread_imc_mem);
> -static struct imc_pmu *thread_imc_pmu;
>   static int thread_imc_mem_size;
>
>   struct imc_pmu *imc_event_to_pmu(struct perf_event *event)
> @@ -1263,7 +1262,6 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
>   				return res;
>   		}
>
> -		thread_imc_pmu = pmu_ptr;
>   		break;
>   	default:
>   		return -EINVAL;

Re: [1/3] powerpc/perf: Remove thread_imc_pmu global variable from

[Michael Ellerman]

On Mon, 2017-12-11 at 05:58:35 UTC, Anju T Sudhakar wrote:
> Remove the global variable 'thread_imc_pmu', since it is not used in the code.
> 
> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> Reviewed-by: madhavan Srinivasan <maddy@linux.vnet.ibm.com>

Series applied to powerpc next, thanks.

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

cheers

[PATCH 2/3] powerpc/perf: IMC code cleanup with some code refactoring

[Anju T Sudhakar]

Factor out memory freeing part for attribute elements from
imc_common_cpuhp_mem_free().

Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
---
 arch/powerpc/perf/imc-pmu.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index ef7f9dd..71f425f 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -1157,6 +1157,15 @@ static void cleanup_all_thread_imc_memory(void)
 	}
 }
 
+/* Function to free the attr_groups which are dynamically allocated */
+static void imc_common_mem_free(struct imc_pmu *pmu_ptr)
+{
+	if (pmu_ptr->attr_groups[IMC_EVENT_ATTR])
+		kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]->attrs);
+	kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]);
+	kfree(pmu_ptr);
+}
+
 /*
  * Common function to unregister cpu hotplug callback and
  * free the memory.
@@ -1189,13 +1198,6 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
 		cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE);
 		cleanup_all_thread_imc_memory();
 	}
-
-	/* Only free the attr_groups which are dynamically allocated  */
-	if (pmu_ptr->attr_groups[IMC_EVENT_ATTR])
-		kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]->attrs);
-	kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]);
-	kfree(pmu_ptr);
-	return;
 }
 
 
@@ -1244,8 +1246,10 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
 		core_imc_refc = kcalloc(nr_cores, sizeof(struct imc_pmu_ref),
 								GFP_KERNEL);
 
-		if (!core_imc_refc)
+		if (!core_imc_refc) {
+			kfree(pmu_ptr->mem_info);
 			return -ENOMEM;
+		}
 
 		core_imc_pmu = pmu_ptr;
 		break;
@@ -1258,8 +1262,10 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
 		thread_imc_mem_size = pmu_ptr->counter_mem_size;
 		for_each_online_cpu(cpu) {
 			res = thread_imc_mem_alloc(cpu, pmu_ptr->counter_mem_size);
-			if (res)
+			if (res) {
+				cleanup_all_thread_imc_memory();
 				return res;
+			}
 		}
 
 		break;
@@ -1285,8 +1291,10 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
 	int ret;
 
 	ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
-	if (ret)
-		goto err_free;
+	if (ret) {
+		imc_common_mem_free(pmu_ptr);
+		return ret;
+	}
 
 	switch (pmu_ptr->domain) {
 	case IMC_DOMAIN_NEST:
@@ -1353,6 +1361,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
 	return 0;
 
 err_free:
+	imc_common_mem_free(pmu_ptr);
 	imc_common_cpuhp_mem_free(pmu_ptr);
 	return ret;
 }
-- 
2.7.4

Re: [PATCH 2/3] powerpc/perf: IMC code cleanup with some code refactoring

[Madhavan Srinivasan]

On Monday 11 December 2017 11:28 AM, Anju T Sudhakar wrote:
> Factor out memory freeing part for attribute elements from
> imc_common_cpuhp_mem_free().

Looks good to me.

Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>

> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
>   arch/powerpc/perf/imc-pmu.c | 31 ++++++++++++++++++++-----------
>   1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index ef7f9dd..71f425f 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1157,6 +1157,15 @@ static void cleanup_all_thread_imc_memory(void)
>   	}
>   }
>
> +/* Function to free the attr_groups which are dynamically allocated */
> +static void imc_common_mem_free(struct imc_pmu *pmu_ptr)
> +{
> +	if (pmu_ptr->attr_groups[IMC_EVENT_ATTR])
> +		kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]->attrs);
> +	kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]);
> +	kfree(pmu_ptr);
> +}
> +
>   /*
>    * Common function to unregister cpu hotplug callback and
>    * free the memory.
> @@ -1189,13 +1198,6 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
>   		cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE);
>   		cleanup_all_thread_imc_memory();
>   	}
> -
> -	/* Only free the attr_groups which are dynamically allocated  */
> -	if (pmu_ptr->attr_groups[IMC_EVENT_ATTR])
> -		kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]->attrs);
> -	kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]);
> -	kfree(pmu_ptr);
> -	return;
>   }
>
>
> @@ -1244,8 +1246,10 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
>   		core_imc_refc = kcalloc(nr_cores, sizeof(struct imc_pmu_ref),
>   								GFP_KERNEL);
>
> -		if (!core_imc_refc)
> +		if (!core_imc_refc) {
> +			kfree(pmu_ptr->mem_info);
>   			return -ENOMEM;
> +		}
>
>   		core_imc_pmu = pmu_ptr;
>   		break;
> @@ -1258,8 +1262,10 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent,
>   		thread_imc_mem_size = pmu_ptr->counter_mem_size;
>   		for_each_online_cpu(cpu) {
>   			res = thread_imc_mem_alloc(cpu, pmu_ptr->counter_mem_size);
> -			if (res)
> +			if (res) {
> +				cleanup_all_thread_imc_memory();
>   				return res;
> +			}
>   		}
>
>   		break;
> @@ -1285,8 +1291,10 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   	int ret;
>
>   	ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
> -	if (ret)
> -		goto err_free;
> +	if (ret) {
> +		imc_common_mem_free(pmu_ptr);
> +		return ret;
> +	}
>
>   	switch (pmu_ptr->domain) {
>   	case IMC_DOMAIN_NEST:
> @@ -1353,6 +1361,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   	return 0;
>
>   err_free:
> +	imc_common_mem_free(pmu_ptr);
>   	imc_common_cpuhp_mem_free(pmu_ptr);
>   	return ret;
>   }

[PATCH 3/3] powerpc/perf: Passs struct imc_events as a parameter to imc_parse_event()

[Anju T Sudhakar]

Remove the allocation of struct imc_events from imc_parse_event(). Instead pass
imc_events as a parameter to imc_parse_event(), which is a pointer to a slot in
the array allocated in update_events_in_group().

Reported by: Dan Carpenter ("powerpc/perf: Fix a sizeof() typo so we allocate less memory")
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/imc-pmu.h |  2 +-
 arch/powerpc/perf/imc-pmu.c        | 66 +++++++++++++++++++++++---------------
 2 files changed, 41 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/include/asm/imc-pmu.h b/arch/powerpc/include/asm/imc-pmu.h
index fad0e6f..080731d 100644
--- a/arch/powerpc/include/asm/imc-pmu.h
+++ b/arch/powerpc/include/asm/imc-pmu.h
@@ -71,7 +71,7 @@ struct imc_events {
 struct imc_pmu {
 	struct pmu pmu;
 	struct imc_mem_info *mem_info;
-	struct imc_events **events;
+	struct imc_events *events;
 	/*
 	 * Attribute groups for the PMU. Slot 0 used for
 	 * format attribute, slot 1 used for cpusmask attribute,
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 71f425f..5cb1f31 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -116,17 +116,13 @@ static struct attribute *device_str_attr_create(const char *name, const char *st
 	return &attr->attr.attr;
 }
 
-struct imc_events *imc_parse_event(struct device_node *np, const char *scale,
-				  const char *unit, const char *prefix, u32 base)
+static int imc_parse_event(struct device_node *np, const char *scale,
+				  const char *unit, const char *prefix,
+				  u32 base, struct imc_events *event)
 {
-	struct imc_events *event;
 	const char *s;
 	u32 reg;
 
-	event = kzalloc(sizeof(struct imc_events), GFP_KERNEL);
-	if (!event)
-		return NULL;
-
 	if (of_property_read_u32(np, "reg", &reg))
 		goto error;
 	/* Add the base_reg value to the "reg" */
@@ -157,14 +153,32 @@ struct imc_events *imc_parse_event(struct device_node *np, const char *scale,
 			goto error;
 	}
 
-	return event;
+	return 0;
 error:
 	kfree(event->unit);
 	kfree(event->scale);
 	kfree(event->name);
-	kfree(event);
+	return -EINVAL;
+}
+
+/*
+ * imc_free_events: Function to cleanup the events list, having
+ * 		    "nr_entries".
+ */
+static void imc_free_events(struct imc_events *events, int nr_entries)
+{
+	int i;
+
+	/* Nothing to clean, return */
+	if (!events)
+		return;
+	for (i = 0; i < nr_entries; i++) {
+		kfree(events[i].unit);
+		kfree(events[i].scale);
+		kfree(events[i].name);
+	}
 
-	return NULL;
+	kfree(events);
 }
 
 /*
@@ -176,9 +190,8 @@ static int update_events_in_group(struct device_node *node, struct imc_pmu *pmu)
 	struct attribute_group *attr_group;
 	struct attribute **attrs, *dev_str;
 	struct device_node *np, *pmu_events;
-	struct imc_events *ev;
 	u32 handle, base_reg;
-	int i=0, j=0, ct;
+	int i = 0, j = 0, ct, ret;
 	const char *prefix, *g_scale, *g_unit;
 	const char *ev_val_str, *ev_scale_str, *ev_unit_str;
 
@@ -216,15 +229,17 @@ static int update_events_in_group(struct device_node *node, struct imc_pmu *pmu)
 	ct = 0;
 	/* Parse the events and update the struct */
 	for_each_child_of_node(pmu_events, np) {
-		ev = imc_parse_event(np, g_scale, g_unit, prefix, base_reg);
-		if (ev)
-			pmu->events[ct++] = ev;
+		ret = imc_parse_event(np, g_scale, g_unit, prefix, base_reg, &pmu->events[ct]);
+		if (!ret)
+			ct++;
 	}
 
 	/* Allocate memory for attribute group */
 	attr_group = kzalloc(sizeof(*attr_group), GFP_KERNEL);
-	if (!attr_group)
+	if (!attr_group) {
+		imc_free_events(pmu->events, ct);
 		return -ENOMEM;
+	}
 
 	/*
 	 * Allocate memory for attributes.
@@ -237,31 +252,31 @@ static int update_events_in_group(struct device_node *node, struct imc_pmu *pmu)
 	attrs = kcalloc(((ct * 3) + 1), sizeof(struct attribute *), GFP_KERNEL);
 	if (!attrs) {
 		kfree(attr_group);
-		kfree(pmu->events);
+		imc_free_events(pmu->events, ct);
 		return -ENOMEM;
 	}
 
 	attr_group->name = "events";
 	attr_group->attrs = attrs;
 	do {
-		ev_val_str = kasprintf(GFP_KERNEL, "event=0x%x", pmu->events[i]->value);
-		dev_str = device_str_attr_create(pmu->events[i]->name, ev_val_str);
+		ev_val_str = kasprintf(GFP_KERNEL, "event=0x%x", pmu->events[i].value);
+		dev_str = device_str_attr_create(pmu->events[i].name, ev_val_str);
 		if (!dev_str)
 			continue;
 
 		attrs[j++] = dev_str;
-		if (pmu->events[i]->scale) {
-			ev_scale_str = kasprintf(GFP_KERNEL, "%s.scale",pmu->events[i]->name);
-			dev_str = device_str_attr_create(ev_scale_str, pmu->events[i]->scale);
+		if (pmu->events[i].scale) {
+			ev_scale_str = kasprintf(GFP_KERNEL, "%s.scale", pmu->events[i].name);
+			dev_str = device_str_attr_create(ev_scale_str, pmu->events[i].scale);
 			if (!dev_str)
 				continue;
 
 			attrs[j++] = dev_str;
 		}
 
-		if (pmu->events[i]->unit) {
-			ev_unit_str = kasprintf(GFP_KERNEL, "%s.unit",pmu->events[i]->name);
-			dev_str = device_str_attr_create(ev_unit_str, pmu->events[i]->unit);
+		if (pmu->events[i].unit) {
+			ev_unit_str = kasprintf(GFP_KERNEL, "%s.unit", pmu->events[i].name);
+			dev_str = device_str_attr_create(ev_unit_str, pmu->events[i].unit);
 			if (!dev_str)
 				continue;
 
@@ -272,7 +287,6 @@ static int update_events_in_group(struct device_node *node, struct imc_pmu *pmu)
 	/* Save the event attribute */
 	pmu->attr_groups[IMC_EVENT_ATTR] = attr_group;
 
-	kfree(pmu->events);
 	return 0;
 }
 
-- 
2.7.4