On Thu, 29 Jun 2017, Anju T Sudhakar wrote:

+static void cleanup_all_core_imc_memory(struct imc_pmu *pmu_ptr)
+{
+	struct imc_mem_info *ptr;
+
+	for (ptr = pmu_ptr->mem_info; ptr; ptr++) {
+		if (ptr->vbase[0])
+			free_pages((u64)ptr->vbase[0], 0);
+	}

This loop is broken beyond repair. If pmu_ptr->mem_info is not NULL, then
ptr will happily increment to the point where it wraps around to
NULL. Oh well.

+	kfree(pmu_ptr->mem_info);

+bool is_core_imc_mem_inited(int cpu)

This function is global because?

+{
+	struct imc_mem_info *mem_info;
+	int core_id = (cpu / threads_per_core);
+
+	mem_info = &core_imc_pmu->mem_info[core_id];
+	if ((mem_info->id == core_id) && (mem_info->vbase[0] != NULL))
+		return true;
+
+	return false;
+}
+
+/*
+ * imc_mem_init : Function to support memory allocation for core imc.
+ */
+static int imc_mem_init(struct imc_pmu *pmu_ptr)
+{

The function placement is horrible. This function belongs to the pmu init
stuff and wants to be placed there and not five pages away in the middle of
unrelated functions.

+	int nr_cores;
+
+	if (pmu_ptr->imc_counter_mmaped)
+		return 0;
+	nr_cores = num_present_cpus() / threads_per_core;
+	pmu_ptr->mem_info = kzalloc((sizeof(struct imc_mem_info) * nr_cores), GFP_KERNEL);
+	if (!pmu_ptr->mem_info)
+		return -ENOMEM;
+	return 0;
+}
+static int core_imc_event_init(struct perf_event *event)
+{
+	int core_id, rc;
+	u64 config = event->attr.config;
+	struct imc_mem_info *pcmi;
+	struct imc_pmu *pmu;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/* Sampling not supported */
+	if (event->hw.sample_period)
+		return -EINVAL;
+
+	/* unsupported modes and filters */
+	if (event->attr.exclude_user   ||
+	    event->attr.exclude_kernel ||
+	    event->attr.exclude_hv     ||
+	    event->attr.exclude_idle   ||
+	    event->attr.exclude_host   ||
+	    event->attr.exclude_guest)
+		return -EINVAL;
+
+	if (event->cpu < 0)
+		return -EINVAL;
+
+	event->hw.idx = -1;
+	pmu = imc_event_to_pmu(event);
+
+	/* Sanity check for config (event offset and rvalue) */
+	if (((config & IMC_EVENT_OFFSET_MASK) > pmu->counter_mem_size) ||
+	    ((config & IMC_EVENT_RVALUE_MASK) != 0))
+		return -EINVAL;
+
+	if (!is_core_imc_mem_inited(event->cpu))
+		return -ENODEV;
+
+	core_id = event->cpu / threads_per_core;
+	pcmi = &pmu->mem_info[core_id];
+	if ((pcmi->id != core_id) || (!pcmi->vbase[0]))
+		return -ENODEV;
+
+	event->hw.event_base = (u64)pcmi->vbase[0] + (config & IMC_EVENT_OFFSET_MASK);
+	/*
+	 * Core pmu units are enabled only when it is used.
+	 * See if this is triggered for the first time.
+	 * If yes, take the mutex lock and enable the core counters.
+	 * If not, just increment the count in core_events.
+	 */
+	if (atomic_inc_return(&core_events[core_id]) == 1) {
+		mutex_lock(&imc_core_reserve);
+		rc = opal_imc_counters_start(OPAL_IMC_COUNTERS_CORE,
+					     get_hard_smp_processor_id(event->cpu));
+		mutex_unlock(&imc_core_reserve);

That machinery here is racy as hell in several aspects.

CPU0 	       	       	       	       CPU1

atomic_inc_ret(core_events[0]) -> 1

preemption()
			       	       atomic_inc_ret(core_events[0]) -> 2
				       return 0;
				       
				       Uses the event, without counters
				       being started until the preempted
				       task comes on the CPU again.

Here is another one:

CPU0 	       	       	       	       CPU1

atomic_dec_ret(core_events[0]) -> 0
				       atomic_inc_ret(core_events[1] -> 1
				       mutex_lock();
mutex_lock()			       start counter();
				       mutex_unlock()

stop_counter();
mutex_unlock();
				       Yay, another stale event!

Brilliant stuff that, or maybe not so much.

+		if (rc) {
+			atomic_dec_return(&core_events[core_id]);

What's the point of using atomic_dec_return here if you ignore the return
value? Not that it matters much as the whole logic is crap anyway.

Sure ret needs to stay initialized, just in case imc_mem_init() does not
return anything magically, right?

+	ret = imc_mem_init(pmu_ptr);
+	if (ret)
+		goto err_free;
 	/* Add cpumask and register for hotplug notification */
-	if (atomic_inc_return(&nest_pmus) == 1) {
-		/*
-		 * Nest imc pmu need only one cpu per chip, we initialize the
-		 * cpumask for the first nest imc pmu and use the same for the rest.
-		 * To handle the cpuhotplug callback unregister, we track
-		 * the number of nest pmus registers in "nest_pmus".
-		 * "nest_imc_cpumask_initialized" is set to zero during cpuhotplug
-		 * callback unregister.
-		 */
-		ret = nest_pmu_cpumask_init();
+	switch (pmu_ptr->domain) {
+	case IMC_DOMAIN_NEST:
+		if (atomic_inc_return(&nest_pmus) == 1) {
+			/*
+			 * Nest imc pmu need only one cpu per chip, we initialize
+			 * the cpumask for the first nest imc pmu and use the
+			 * same for the rest.
+			 * To handle the cpuhotplug callback unregister, we track
+			 * the number of nest pmus registers in "nest_pmus".
+			 * "nest_imc_cpumask_initialized" is set to zero during
+			 * cpuhotplug callback unregister.
+			 */
+			ret = nest_pmu_cpumask_init();
+			if (ret)
+				goto err_free;
+			mutex_lock(&imc_nest_inited_reserve);
+			nest_imc_cpumask_initialized = 1;
+			mutex_unlock(&imc_nest_inited_reserve);
+		}
+		break;
+	case IMC_DOMAIN_CORE:
+		ret = core_imc_pmu_cpumask_init();
 		if (ret)
-			goto err_free;
-		mutex_lock(&imc_nest_inited_reserve);
-		nest_imc_cpumask_initialized = 1;
-		mutex_unlock(&imc_nest_inited_reserve);
+			return ret;

Oh, so now you replaced the goto with ret. What is actually taking care of
the cleanup if that fails?

Cute. You cleanuo the memory stuff and then you let the hotplug core invoke
the offline callbacks which then deal with freed memory. 

Thanks,

	tglx