On Thursday 29 June 2017 01:11 AM, Thomas Gleixner wrote:
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);

Idea is to handle multiple event session for a given core and
yes, my bad, current implementation is racy/broken.
But an alternate approach is to have a per-core mutex and
per-core ref count to handle this.

event_init path:
per-core mutex lock
  if ( per-core[refcount] == 0) {
       rc = opal call to start the engine;
       if (rc on failure) {
       per-core mutex unlock;
       log error info;
       return error;
       }
  }
  increment the per-core[refcount];
per-core mutex unlock;


event_destroy path:
per-core mutex lock
  decrement the per-core[refcount];
  if ( per-core[refcount] == 0) {
     rc = opal call to stop the engine;
     if (rc on failure) {
       per-core mutex unlock;
       log the failure;
       return error;
     }
  } else if ( per-core[refcount] < 0) {
     WARN()
     per-core[refcount] = 0;
  }
per-core mutext unlock;

Kindly let me know your comment for this.

Maddy

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.

+                       pr_err("IMC: Unable to start the counters for core 
%d\n", core_id);
+                       return -ENODEV;
+               }
+       }
@@ -410,22 +658,38 @@ int init_imc_pmu(struct imc_events *events, int idx,
                 struct imc_pmu *pmu_ptr)
  {
        int ret = -ENODEV;
+
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?

+               break;
+       default:
+               return -1;      /* Unknown domain */
        }
        ret = update_events_in_group(events, idx, pmu_ptr);
        if (ret)
@@ -459,5 +723,10 @@ int init_imc_pmu(struct imc_events *events, int idx,
                        mutex_unlock(&imc_nest_inited_reserve);
                }
        }
+       /* For core_imc, we have allocated memory, we need to free it */
+       if (pmu_ptr->domain == IMC_DOMAIN_CORE) {
+               cleanup_all_core_imc_memory(pmu_ptr);
+               cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_CORE_IMC_ONLINE);
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


Reply via email to