From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933947AbdEOLGP (ORCPT ); Mon, 15 May 2017 07:06:15 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:32948 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750937AbdEOLGN (ORCPT ); Mon, 15 May 2017 07:06:13 -0400 Date: Mon, 15 May 2017 13:06:09 +0200 (CEST) From: Thomas Gleixner To: Madhavan Srinivasan cc: Anju T Sudhakar , mpe@ellerman.id.au, LKML , linuxppc-dev@lists.ozlabs.org, ego@linux.vnet.ibm.com, bsingharora@gmail.com, Anton Blanchard , sukadev@linux.vnet.ibm.com, mikey@neuling.org, stewart@linux.vnet.ibm.com, dja@axtens.net, eranian@google.com, hemant@linux.vnet.ibm.com, Peter Zijlstra Subject: Re: [PATCH v8 05/10] powerpc/perf: IMC pmu cpumask and cpuhotplug support In-Reply-To: Message-ID: References: <1493907596-11425-1-git-send-email-anju@linux.vnet.ibm.com> <1493907596-11425-6-git-send-email-anju@linux.vnet.ibm.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 15 May 2017, Madhavan Srinivasan wrote: > On Wednesday 10 May 2017 05:39 PM, Thomas Gleixner wrote: > > On Thu, 4 May 2017, Anju T Sudhakar wrote: > > > + /* > > > + * 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); > > > + } > > What disables the perf context if this was the last CPU on the node? > > My bad. i did not understand this. Is this regarding the updates > of the "flags" in the perf_event and hw_perf_event structs? Sorry, there is nothing to understand. It was me misreading it. > > > +static int nest_pmu_cpumask_init(void) > > > +{ > > > + const struct cpumask *l_cpumask; > > > + int cpu, nid; > > > + int *cpus_opal_rc; > > > + > > > + if (!cpumask_empty(&nest_imc_cpumask)) > > > + return 0; > > What's that for? Paranoia engineering? > > No. The idea here is to generate the cpu_mask attribute > field only for the first "nest" pmu and use the same > for other "nest" units. Why is nest_pmu_cpumask_init() called more than once? If it is then you should have a proper flag marking it initialiazed rather than making a completely obscure check for a cpu mask. That check could be empty for the second invocation when the first invocation fails. > > But this whole function is completely overengineered. If you make that > > nest_init() part of the online function then this works also for nodes > > which come online later and it simplifies to: > > > > 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 IMC is > > * initialized already. > > */ > > if (cpumask_and(&tmp_mask, l_cpumask, &nest_imc_cpumask)) > > return 0; > > > > /* > > * If this fails, IMC is not usable. > > * > > * FIXME: Add a understandable comment what this actually does > > * and why it can fail. > > */ > > 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); > > return 0; > > } > > > > static int nest_pmu_cpumask_init(void) > > { > > return cpuhp_setup_state(CPUHP_AP_PERF_POWERPC_NEST_ONLINE, > > "perf/powerpc/imc:online", > > ppc_nest_imc_cpu_online, > > ppc_nest_imc_cpu_offline); > > } > > > > Hmm? > > Yes this make sense. But we need to first designate a cpu in each > chip at init setup and use opal api to disable the engine in the same. > So probably, after cpuhp_setup_state, can we do that? Errm. That's what ppc_nest_imc_cpu_online() does. It checks whether this is the first online cpu on a node and if yes, it calls opal_imc_counters_stop() and sets that cpu in nest_imc_cpumask. cpuhp_setup_state() invokes the callback on each online CPU. Seo evrything is set up proper after that. Thanks, tglx