lkml.org 
[lkml]   [2017]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v9 05/10] powerpc/perf: IMC pmu cpumask and cpuhotplug support
On Mon, 5 Jun 2017, Anju T Sudhakar wrote:
> +/*
> + * nest_imc_stop : Does OPAL call to stop nest engine.
> + */
> +static void nest_imc_stop(int *cpu_opal_rc)
> +{
> + int rc;
> +
> + rc = opal_imc_counters_stop(OPAL_IMC_COUNTERS_NEST);
> + if (rc)
> + cpu_opal_rc[smp_processor_id()] = 1;

What's wrong with just assigning the result directly?

> +/* nest_imc_start: Does the OPAL call to enable nest counters. */
> +static void nest_imc_start(int *cpu_opal_rc)
> +{
> + int rc;
> +
> + rc = opal_imc_counters_start(OPAL_IMC_COUNTERS_NEST);
> + if (rc)
> + cpu_opal_rc[smp_processor_id()] = 1;
> +}
> +
> +/*
> + * nest_imc_control: Function to start and stop nest imc engine.
> + * The function is primarily called from event init
> + * and event destroy.

As I don't see other call sites, what's the secondary usage?

> + */
> +static int nest_imc_control(int operation)
> +{
> + int *cpus_opal_rc, cpu;
> +
> + /*
> + * Memory for OPAL call return value.
> + */
> + cpus_opal_rc = kzalloc((sizeof(int) * nr_cpu_ids), GFP_KERNEL);
> + if (!cpus_opal_rc)
> + return -ENOMEM;
> + switch (operation) {
> + case IMC_COUNTER_ENABLE:
> + /* Initialize Nest PMUs in each node using designated cpus */
> + on_each_cpu_mask(&nest_imc_cpumask, (smp_call_func_t)nest_imc_start,
> + cpus_opal_rc, 1);

That's one indentation level too deep.

> + break;
> + case IMC_COUNTER_DISABLE:
> + /* Disable the counters */
> + on_each_cpu_mask(&nest_imc_cpumask, (smp_call_func_t)nest_imc_stop,
> + cpus_opal_rc, 1);
> + break;
> + default:
> + kfree(cpus_opal_rc);
> + return -EINVAL;
> +
> + }
> +
> + /* Check return value array for any OPAL call failure */
> + for_each_cpu(cpu, &nest_imc_cpumask) {
> + if (cpus_opal_rc[cpu]) {
> + kfree(cpus_opal_rc);
> + return -ENODEV;
> + }
> + }
> + kfree(cpus_opal_rc);

So you have THREE places where you do kfree(cpus_opal_rc). Sigh.

You can spare that hole alloc/free dance by simply having

static struct cpumask nest_imc_result;

mutex_lock(&imc_nest_reserve);
memset(&nest_imc_result, 0, sizeof(nest_imc_result));

switch (op) {
case IMC_COUNTER_ENABLE:
on_each_cpu_mask(&nest_imc_cpumask, nest_imc_start, NULL, 1);
break;
....
}

res = cpumask_empty(&nest_imc_result) ? 0 : -ENODEV;
mutex_unlock(&imc_nest_reserve);
return res;

And in the start/stop functions:

if (opal_imc_counters_xxx(OPAL_IMC_COUNTERS_NEST))
cpumask_set_cpu(smp_processor_id(), &nest_imc_result);

> +static void nest_change_cpu_context(int old_cpu, int new_cpu)
> +{
> + struct imc_pmu **pn = per_nest_pmu_arr;
> + int i;
> +
> + if (old_cpu < 0 || new_cpu < 0)
> + return;
> +
> + for (i = 0; *pn && i < IMC_MAX_PMUS; i++, pn++)
> + perf_pmu_migrate_context(&(*pn)->pmu, old_cpu, new_cpu);
> +
> +}
> +
> +static int ppc_nest_imc_cpu_offline(unsigned int cpu)
> +{
> + int nid, target = -1;
> + const struct cpumask *l_cpumask;
> +
> + /*
> + * Check in the designated list for this cpu. Dont bother
> + * if not one of them.
> + */
> + if (!cpumask_test_and_clear_cpu(cpu, &nest_imc_cpumask))
> + return 0;
> +
> + /*
> + * Now that this cpu is one of the designated,
> + * find a next cpu a) which is online and b) in same chip.
> + */
> + nid = cpu_to_node(cpu);
> + l_cpumask = cpumask_of_node(nid);
> + target = cpumask_next(cpu, l_cpumask);

So if the outgoing CPU is the highest numbered CPU on the node, then
cpumask_next() will not find a CPU, while there still might be other lower
numbered CPUs online in the node.

target = cpumask_any_but(l_cpumask, cpu);

is what you want.

> +
> + /*
> + * Update the cpumask with the target cpu and
> + * migrate the context if needed
> + */
> + if (target >= 0 && target < nr_cpu_ids) {
> + cpumask_set_cpu(target, &nest_imc_cpumask);
> + nest_change_cpu_context(cpu, target);
> + } else {
> + target = -1;
> + opal_imc_counters_stop(OPAL_IMC_COUNTERS_NEST);
> + nest_change_cpu_context(cpu, target);

Why are you calling nest_change_cpu_context()? You already know that it
will return immediately due to target < 0.

> + }
> + return 0;
> +}
> +
> +static int ppc_nest_imc_cpu_online(unsigned int cpu)
> +{
> + const struct cpumask *l_cpumask;
> + static struct cpumask tmp_mask;
> + int res;
> +
> + /* Get the cpumask of this node */
> + l_cpumask = cpumask_of_node(cpu_to_node(cpu));
> +
> + /*
> + * If this is not the first online CPU on this node, then
> + * just return.
> + */
> + if (cpumask_and(&tmp_mask, l_cpumask, &nest_imc_cpumask))
> + return 0;
> +
> + /*
> + * If this is the first online cpu on this node
> + * disable the nest counters by making an OPAL call.
> + */
> + res = opal_imc_counters_stop(OPAL_IMC_COUNTERS_NEST);
> + if (res)
> + return res;
> +
> + /* Make this CPU the designated target for counter collection */
> + cpumask_set_cpu(cpu, &nest_imc_cpumask);
> + nest_change_cpu_context(-1, cpu);

Ditto

> static int nest_imc_event_init(struct perf_event *event)
> {
> - int chip_id;
> + int chip_id, rc;
> u32 config = event->attr.config;
> struct imc_mem_info *pcni;
> struct imc_pmu *pmu;
> @@ -80,6 +277,20 @@ static int nest_imc_event_init(struct perf_event *event)
> * "chip_id" and add "config" to it".
> */
> event->hw.event_base = pcni->vbase[config/PAGE_SIZE] + (config & ~PAGE_MASK);
> + /*
> + * Nest 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 nest counters.
> + * If not, just increment the count in nest_events.
> + */
> + if (atomic_inc_return(&nest_events) == 1) {
> + mutex_lock(&imc_nest_reserve);
> + rc = nest_imc_control(IMC_COUNTER_ENABLE);
> + mutex_unlock(&imc_nest_reserve);
> + if (rc)
> + pr_err("IMC: Unable to start the counters\n");

So if that fails, nest_events will stay incremented. Is that on purpose?

> + }
> + event->destroy = nest_imc_counters_release;
> return 0;

And this happily returns success even if the enable call failed ....

Thanks,

tglx
\
 
 \ /
  Last update: 2017-06-12 00:41    [W:0.526 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site