From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758145AbcCCPO4 (ORCPT ); Thu, 3 Mar 2016 10:14:56 -0500 Received: from mail-bn1bon0086.outbound.protection.outlook.com ([157.56.111.86]:63776 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754666AbcCCPOy (ORCPT ); Thu, 3 Mar 2016 10:14:54 -0500 Authentication-Results: spf=none (sender IP is 165.204.84.222) smtp.mailfrom=amd.com; alien8.de; dkim=none (message not signed) header.d=none;alien8.de; dmarc=permerror action=none header.from=amd.com; X-WSS-ID: 0O3GYCJ-08-552-02 X-M-MSG: Date: Thu, 3 Mar 2016 23:13:58 +0800 From: Huang Rui To: Thomas Gleixner CC: Borislav Petkov , Peter Zijlstra , "Ingo Molnar" , Andy Lutomirski , "Robert Richter" , Jacob Shin , "Arnaldo Carvalho de Melo" , Kan Liang , , , , 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 Message-ID: <20160303151357.GA2154@hr-amur2> References: <1456992284-4808-1-git-send-email-ray.huang@amd.com> <1456992284-4808-3-git-send-email-ray.huang@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:165.204.84.222;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(2980300002)(428002)(164054003)(24454002)(189002)(199003)(46406003)(33716001)(33656002)(5001960100004)(106466001)(105586002)(11100500001)(92566002)(47776003)(110136002)(5008740100001)(189998001)(586003)(50986999)(86362001)(2906002)(76176999)(2950100001)(4326007)(4001350100001)(97756001)(101416001)(54356999)(77096005)(1096002)(50466002)(1220700001)(1076002)(23726003)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BLUPR12MB0708;H:atltwp02.amd.com;FPR:;SPF:None;MLV:sfv;MX:1;A:1;LANG:en; X-MS-Office365-Filtering-Correlation-Id: bbc0ab97-16c5-41d4-fa00-08d343769094 X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0708;2:xahwaAGzRvOpUg3KuOiIFoclfEXvebE19HbITVVWn1KBFNSjPLyY1RBeM51grjMbk5aBPLUgwXRcdwg2Tx1aimc4E3VjK9YHPA9FhbJ8tXZHSvqty4lxVdINJis0Q6r2Yl0GsPQLuM4XA3mweuGugoA0FwAwVFuixqUlmY6njLUt7erm+V6r/sTNEEaWH5No;3:8Zr/ffv1p4ogw1O+RR/kwpIgnf/0KyRJOvw9YkXkCz1LTMEEpj+9z1gstGBODQ1WLEuEn7g1Ot9YZxJxZKzxR9jZ8c6Z9hi1CQJPR8PbgVc1CW2lp+qYjEUBOzNyRxEzb/skZtH1Iiw2ONW6/QuwLRIUlij763srDNchzOIaVKLFSDQXArJUzsEvzik3BrnRgrmerDumxQpfH6G7azhowdPmBmGng9d8olBMtiJlm70=;25:1gwLHSZsqqvwwRYgt6XDb9rLBipW0ul+khqs0V5o4vHX/QLYwSRYsWWxWfbyVodQzEmzMu7rsDV48vE85QemIKEFimIg7wQVoPLB5xpgn0+uXGTyqdqEI5wGFfTmHfrPbIJtvQvTDwfkSPI+kqgWZZ3rHMbkgQAdseuKrfLXd/5Nt3G0CBPen3t+IlS4Ffbg9ZSb4imogmGMpAuVaasR8wEuR5inKiRTy1JM5/B3nCc9eLzGam1fIzx4oAYJ0jm80NykRDT3HIcCxbVbEf4LHsp/iQ/YBudR5TinQtBGvVLbBb8yzzZzhc+vd97J9nZB4SbuDJLiUaMezwQ2oRnuNg== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BLUPR12MB0708; X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0708;20:6RcZA+rKQU6DOjTNVerkk2j84CuCPw+tUzs2RSE0wSAJ8CYokiL65JO9LdwubsZUJZwg3c5s6R0GoElEqr4nuWargVlfYM2qgDjVYNbcjeNrBDLjG5OyOfWI+yFbJ3TOP9rs24Dbd5dFeT8XMcOzDbwuDrM3y7iRDrIaAQ+LAPGr9KuQDa4nIjs4B+xHz5umy6St52mQAadnytSC++fkJHitbRoHgi+ImtpT83NqKkYVXkjSDvEJ7lc3/U1CtywT2+uDy530T4p++2Msg+L54Um167IussJ+8vvm72yUJsYPByWNQeGZaIInFmLQbP8vJ8T7T9gsRbaA3xSQiSrZCUGRU3+3nTOi6PyjCE2EaFJmAv2pYz+qgCswDjtK1/7OCJb+L3QORW/w3zpwvorYQ22D3S1QoBKi/H2f9YVRsJnFXR/QdqmPHNVY29O06yQVdWd4ylQTt0qzHQJB0d/zCCpUb8m81+CeaoFzxlc9bqwDNtCAIIK9LsJ9swrWIuOT X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(13015025)(13017025)(13023025)(13024025)(5005006)(8121501046)(13018025)(3002001)(10201501046);SRVR:BLUPR12MB0708;BCL:0;PCL:0;RULEID:;SRVR:BLUPR12MB0708; X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0708;4:CG9JEBFQDRKuPhqJUqzwEmpNg/YPKRk9krHYFTdXThSbRMqJvxwuIOWyt+av//L/ZDw45WyVhXQMSt7hO2+46fJXSciLZt9jEnqClZfJ4k/8TyQt3qYXXbAR6s8wd03UZQMB1wFyvBnDM+U1hhtoH1dvCots2dsQVATpIaMK+XwkYSUoNCvFgum/OH3ELLNAjNL9nzIZDvjZ3zkp/5WVHEG4G6UPalyNYLBrbIPahwVbp/zKw9u3uz+FPRefnM0jZ1mA/vXI53S0XP2tpZXNl4OXq86vaIixqbUuzmdYxINtGBaDds69F/DYkCk8Z9eHhPEAL4IEVyn1R36tW0M8fxYrNW+Wtot6HhlKX9ZBNzyxRz3pcIzW32ZUqLhMumP4dGGo/2lCs4hKHecCPAZzt/aOC7Zd9H8reNd0m0fizHEbEeRXvy//SDjHhGt84xJWIwectad20f3yE0lssiv6UQ== X-Forefront-PRVS: 0870212862 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BLUPR12MB0708;23:lRw8cHTmzxluoomgtx11a8gxrg73FALDGCEN1+IWx?= =?us-ascii?Q?xvrtoolvpr62aoaj361b+1wf2B12xswP7Lgl9PA7/ZU6pfB6Gx3VnBO41jWS?= =?us-ascii?Q?LNQdBBRH1jdMtNtR8vuTaqRHNnrBBiNfPy8X2/FNdgn4JYLcuqnPrPOy4Z/M?= =?us-ascii?Q?cvkcItFTMuWthoTYIaeIABxmAUN9sYFNimVm577cM3HwOslcMIsD4zm7LOut?= =?us-ascii?Q?ZBEtVI0MX71jV4zumJloXZtimBbbAwC1vhY05hgKXdcKowxp9/xDCK2wzrtH?= =?us-ascii?Q?Kp9mwSe2hh+wjAvYATVJK2Qn5BASWKBhQC42dFwTsOn0Tec00yZd6FZiSKIC?= =?us-ascii?Q?MAYmJZf754zw52uX7hQ+7lrtHyUr7V7dwLpJXvl+dRqNvhKaWVp2ueItZAMI?= =?us-ascii?Q?VipeQnxQyQpKWeIHmV2usIfc2J8G6MAuRqMjTF7RbydmfhxRlCDGc77Nwt4Z?= =?us-ascii?Q?N0bn34GlHiNmCxd4Q9FromzNxQK1Yvbsy1t6O1btttXoJ/AOHHr58jbWURAi?= =?us-ascii?Q?Co5NmIfugkuwwWv0L2b11uslz/3OO9KDPRG7v/jiBpzaubzQSFOWKifRFNW+?= =?us-ascii?Q?O/i4lx5+EcDLI5/vUP+K9mH8J5nm8W1K6clDNherjm1tUNal1ludw5YdCY78?= =?us-ascii?Q?EBv7QTMkipBSDApp+MvEBQ3ecyX5Gt3f96TqLxu9VyyH6XK3Mf37oe2SEFAR?= =?us-ascii?Q?0fQ2uUtWvGGJl59K7fs5POQnA3Cpgc17BShawnn1yZ5tn7WbSV6DhudxXdg5?= =?us-ascii?Q?pKKevlni5aMqK16i2OYZSU+dbWRcBZr9oLZLnxuStymyn9DtWWZ4w8wr/rv2?= =?us-ascii?Q?ZZUlkIZUJauAtKrVFpJeviYqK1wZuPZJLL5W+EjDtFWSQAMSYqwPZnhJzDTd?= =?us-ascii?Q?N7kFtCrKgxjy8pQbofGgABlbd6fYFCbvEZdXEFXko+vonFdZli2Hrfbu9lUr?= =?us-ascii?Q?cGddjS5qXhc7oWC+9tRB3423WFXzZpiH0bw61u9xiy280yFtOz4aQRg92R+b?= =?us-ascii?Q?dU=3D?= X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0708;5:1+yHXcwTidGhCtYYi9GrObXMG7SR3V17K4NloPoOg7uBciHgkFyEDrrIxgWjChWq/g8hGS0ExHWcJyUMVYB29xb4IgYyopHiZBMr6dRXwXKb81skzko4r3qUIBQlhBfKbkvoWCUrnPg/gJrssHqKYw==;24:MwXH5Ou0Cpr1blah+4oKbKau5QmlSlQOU8FRO2MMYGu59YM3OQnoy2a+HKrthPiXvX2+TIngfFJTxbcRhMnvBr/GiD2vv4Mk4VadMgJW60Q=;20:uCxFofrVLexx/fcyA6EWXPn5u5/K0I4VmzKtJ2Z8vn48mA4rxxQ1CYPn3O5zNSPPzH2fF8bT1CQ1cm1bFLyJ0KHM0qWsRuiB956IFIGE9SCdkerix/yUye3s/SpHohN6bL4uRoxsHuSpX4wgbIP8aasW/LbEwIKuJtxYFbO+nArtfaUbV5kkLhGxsDW5y8/JeFAL/RO41s2n67CWpMRqRk2qnkm/Wc3LJmv7foJG/EGnhtmf6BdHGpmiKA+LOGyP SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2016 15:14:48.9318 (UTC) X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.222];Helo=[atltwp02.amd.com] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR12MB0708 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 03, 2016 at 09:50:11AM +0100, Thomas Gleixner wrote: > On Thu, 3 Mar 2016, Huang Rui wrote: > > + > > +static void power_cpu_init(int cpu) > > +{ > > + /* > > + * 1) If any CPU is set at cpu_mask in the same compute unit, do > > + * nothing. > > + * 2) If no CPU is set at cpu_mask in the same compute unit, > > + * set current STARTING CPU. > > + * > > + * If cpu_mask and topology_sibling_cpumask has intersected > > + * bits, that means any CPU is set in the same compute unit. > > + * But cpumask_weight(topology_sibling_cpumask(cpu)) == 1 > > + * means no CPU is set on cpu_mask in the same compute unit > > + * before init current STARTING CPU. > > + */ > > + if (!cpumask_intersects(&cpu_mask, topology_sibling_cpumask(cpu)) && > > + cpumask_weight(topology_sibling_cpumask(cpu)) == 1) > > + cpumask_set_cpu(cpu, &cpu_mask); > > I don't think you need that complexity. > > target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu); > if (target >= nr_cpumask_bits) > cpumask_set_cpu(cpu, &cpu_mask); > > Simply because if there is a cpu aside of the new one already in the sibling > mask, then it is also in cpu_mask. Hmm? > Make sense. Thanks. Will update. > > +static int > > +power_cpu_notifier(struct notifier_block *self, unsigned long action, void *hcpu) > > +{ > > + unsigned int cpu = (long)hcpu; > > + > > + switch (action & ~CPU_TASKS_FROZEN) { > > + case CPU_STARTING: > > + power_cpu_init(cpu); > > + break; > > + case CPU_DOWN_PREPARE: > > + power_cpu_exit(cpu); > > + break; > > 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? > > +static int __init amd_power_pmu_init(void) > > +{ > > + int i, ret; > > + u64 tmp; > > + > > + if (!x86_match_cpu(cpu_match)) > > + return 0; > > + > > + if (!boot_cpu_has(X86_FEATURE_ACC_POWER)) > > + return -ENODEV; > > + > > + cu_num = boot_cpu_data.x86_max_cores / smp_num_siblings; > > + > > + cpu_pwr_sample_ratio = cpuid_ecx(0x80000007); > > + > > + if (rdmsrl_safe(MSR_F15H_CU_MAX_PWR_ACCUMULATOR, &tmp)) { > > + pr_err("Failed to read max compute unit power accumulator MSR\n"); > > + return -ENODEV; > > + } > > + max_cu_acc_power = tmp; > > Why do you need an intermediate 'tmp' for this? > Will use max_cu_acc_power directly. > > + 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); } > > + } > > + > > + 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. > > + goto out; > > + } > > + > > + 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? Thanks, Rui