From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757297AbcCCP2O (ORCPT ); Thu, 3 Mar 2016 10:28:14 -0500 Received: from www.linutronix.de ([62.245.132.108]:59365 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753149AbcCCP2N (ORCPT ); Thu, 3 Mar 2016 10:28:13 -0500 Date: Thu, 3 Mar 2016 16:26:46 +0100 (CET) From: Thomas Gleixner To: Huang Rui cc: Borislav Petkov , Peter Zijlstra , Ingo Molnar , Andy Lutomirski , Robert Richter , Jacob Shin , Arnaldo Carvalho de Melo , Kan Liang , linux-kernel@vger.kernel.org, spg_linux_kernel@amd.com, x86@kernel.org, Suravee Suthikulpanit , Aravind Gopalakrishnan , Borislav Petkov , Fengguang Wu , Guenter Roeck Subject: Re: [PATCH v6 2/2] perf/x86/amd/power: Add AMD accumulated power reporting mechanism In-Reply-To: <20160303151357.GA2154@hr-amur2> Message-ID: References: <1456992284-4808-1-git-send-email-ray.huang@amd.com> <1456992284-4808-3-git-send-email-ray.huang@amd.com> <20160303151357.GA2154@hr-amur2> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 3 Mar 2016, Huang Rui wrote: > On Thu, Mar 03, 2016 at 09:50:11AM +0100, Thomas Gleixner wrote: > > On Thu, 3 Mar 2016, Huang Rui wrote: > > And of course if CPU_DOWN_PREPARE fails and this is the last cpu in the > > compute unit, nothing takes over the duty for this compute unit. So you need > > to handle CPU_DOWN_FAILED .... > > > > OK, so I need to do power_cpu_init when notified CPU_DOWN_FAILED, am I > right? Yes. > > > + cpu_notifier_register_begin(); > > > + > > > + /* Choose one online core of each compute unit. */ > > > + for (i = 0; i < boot_cpu_data.x86_max_cores; i += smp_num_siblings) { > > > + WARN_ON(cpumask_empty(topology_sibling_cpumask(i))); > > > > Err. What guarantees that in each compute unit is one sibling online? And what > > value has that WARN_ON? We don't care about the stack trace here, because it's > > known already. > > > > When this driver is not as module before, I think there should be one > sibling online at least at initialization phase. But now, you're > right, we cannot guarantee it. > > > > + cpumask_set_cpu(cpumask_any(topology_sibling_cpumask(i)), &cpu_mask); > > > > Of course you just continue in that case and end up with: > > > > cpumask_set_cpu(nr_cpu_ids, &cpu_mask); > > > > i.e. you try to do that on an invalid bit, which will trigger a justified > > warning in cpumask_set_cpu() if CONFIG_DEBUG_PER_CPU_MAPS is enabled. > > > > Aside of that this only handles a single socket. And why do you do the above > > if you handle the same thing in the loop below? > > > > Because the sibling online shouldn't be empty at initialization phase > if the driver is not module before. So... > > Thanks to catch it. > > How about below update: > > for (i = 0; i < boot_cpu_data.x86_max_cores; i += smp_num_siblings) { > if (!cpumask_empty(topology_sibling_cpumask(i))) > cpumask_set_cpu(cpumask_any(topology_sibling_cpumask(i)), &cpu_mask); > } Why? You do a full for_each_online_cpu(i) loop after that, which does exactly the same thing, right? > > > + } > > > + > > > + for_each_online_cpu(i) > > > + power_cpu_init(i); > > > + > > > + __register_cpu_notifier(&power_cpu_notifier_nb); > > > + > > > + ret = perf_pmu_register(&pmu_class, "power", -1); > > > + if (WARN_ON(ret)) { > > > + pr_warn("AMD Power PMU registration failed\n"); > > > > This still leaks the cpu notifier. ..... > > > > OK, so I should do __unregister_cpu_notifier(&power_cpu_notifier_nb) > here. You can register the notifier after perf_pmu_register succeeded, right? > > > + pr_info("AMD Power PMU detected, %d compute units\n", cu_num); > > > > Why is the number of compute units interesting at all? > > > > Because the accumulated power bases on compute units. > We can see the mask from /sys/devices/power/cpumask and number of > compute units to know if all compute units are set at cpumask. > So I add a printk here, does it make sense? No, because it's completely non intuitive. How on earth am I supposed to get the connection between /sys/devices/power/cpumask and number of compute units without staring at the code? So this is only interesting for a developer who can deduce that number from /proc/cpuinfo or dmesg as well. Thanks, tglx