From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965670AbcAZNrF (ORCPT ); Tue, 26 Jan 2016 08:47:05 -0500 Received: from mail-bn1bon0089.outbound.protection.outlook.com ([157.56.111.89]:44960 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932472AbcAZNrB (ORCPT ); Tue, 26 Jan 2016 08:47:01 -0500 Authentication-Results: spf=none (sender IP is 165.204.84.221) 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: 0O1KBM6-07-2RR-02 X-M-MSG: Date: Tue, 26 Jan 2016 21:47:09 +0800 From: Huang Rui To: Ingo Molnar CC: Borislav Petkov , Peter Zijlstra , "Andy Lutomirski" , Thomas Gleixner , Robert Richter , Jacob Shin , "John Stultz" , =?utf-8?B?RnLvv71k77+9cmlj?= Weisbecker , , , , Guenter Roeck , Andreas Herrmann , Suravee Suthikulpanit , Aravind Gopalakrishnan , Borislav Petkov , "Fengguang Wu" , Aaron Lu Subject: Re: [PATCH v3] perf/x86/amd/power: Add AMD accumulated power reporting mechanism Message-ID: <20160126134708.GE23394@hr-amur2> References: <1453710723-1616-1-git-send-email-ray.huang@amd.com> <20160126082823.GA8729@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160126082823.GA8729@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-EOPAttributedMessage: 0 X-Forefront-Antispam-Report: CIP:165.204.84.221;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(2980300002)(428002)(189002)(164054003)(24454002)(199003)(4326007)(19580405001)(50466002)(87936001)(97756001)(2906002)(83506001)(92566002)(86362001)(46406003)(33716001)(1220700001)(77096005)(1096002)(23726003)(110136002)(11100500001)(551984002)(54356999)(2950100001)(106466001)(19580395003)(33656002)(1076002)(189998001)(97736004)(47776003)(5008740100001)(586003)(50986999)(105586002)(4001350100001)(76176999)(101416001)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BN4PR12MB0852;H:atltwp01.amd.com;FPR:;SPF:None;PTR:InfoDomainNonexistent;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BN4PR12MB0852;2:/3U5pKhY/bngq0cURx/qlkmgYsMJp6yCsfTRtnP8we/UbQTKu/Mxv3HDawuPX+Ruuh7dKnVONrOp8wnl+Z/DqHcsKrXiIOT+8mjbaEfLTsbxSvc63xUODGn8RiSvDk0KUokcWD7ayhT/LzHkvnVj8Q==;3:uWGc57+pNE8gOktpw9BgYVD9TfXJzWnMRqqe7SHPpsaRFs6Cwo6kbqbQK48mn23/7lOjI9X1209SbDRW8+fk9maCvbHjv0Okeir3cbATKqtM/4h+rPcM/dh0gIp1+kEFq//fpav3VD+CIhPNNHlSdY8DSfLk7CJqkX4BQnMKnMTMF7t3M+z6lal8El5Jyl5pI3ZzcsKwldzqPov/hkfKaXV3v/qElajctyMAC+d7H04=;25:QaFDRHt3sFkqYSYdFOiyva3PNwklJe6zRAQ6T7wJSp3+fGzhNdzWns+85FlluPRCJITKRYRyIsDIBsvC05MBjbsZ37X00kKi77PkJjhrdte1bFpi+Sk6n5xMPZSG+5hZC5iEOq9EKd4JOaju6417FOVxMzYw+d285AUXz26APXLJkWRgLSEp4y8SzefVGuOuOAISDnpIu6dyPKsDTcKXpqnXEW9v5UziVe2jnmfcxLEhtFjjyTfjpB9q+lREC62b X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN4PR12MB0852; X-MS-Office365-Filtering-Correlation-Id: 51b61d0a-d84e-4158-b070-08d3265728e0 X-Microsoft-Exchange-Diagnostics: 1;BN4PR12MB0852;20:0oEds0J3DY6MsfpG66w18NtkhPnHws/6ZF21JIEQQEFSwNOYcXAMN35CxDGluzJ5MJcAM2rIwvGrg3rPb4SQw55CpKnP0Tdx1bHiLgidrZkt/5JRZLFJ6tB5oLTQAvIEhlnqKwbG4yX3QR4+4vAUoXNK2BNQqCC/H2kKCxIO5X+fnwN5ZyJvTV8znfZulHGlmT6aaRkadUUoHLs+oN8dhEuyXFopiVqLoVXBRb68mH9Xj9N3UCSn65zOS1d/pyvBHJerxnQCPZ+tXCjuMad1ZlFQj+4g1/PpgeFghvENEifEqQjpBDEzhmHrjT5BdN/1GXBdOnSj+zNQZeLQJY/F/ezFUdQiIsOkWCT4F/OZJZRz065U6hufQacaPSx+l5+cFvfxYO4n+hCCA8ePJ8L6Qo0/iUTW8KZ6YCxwO39km73QHsHVazZqvHzkxh5RObDcFy9Qe6krTL8aTdF/Rp2MCd7GGaETkUaH2Ibt+4i4m1L+wGRrNoINUYXVPcrCAlZ0 X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(767451399110); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(123027)(601004)(2401047)(13024025)(13017025)(13018025)(5005006)(13015025)(13023025)(520078)(8121501046)(10201501046)(3002001);SRVR:BN4PR12MB0852;BCL:0;PCL:0;RULEID:;SRVR:BN4PR12MB0852; X-Microsoft-Exchange-Diagnostics: 1;BN4PR12MB0852;4:TKY2DoTyKxIjR0h5mrLd3o+QISlnUyaEH7Tak+EFVveX6H3IuCVw0WGx8xapW3giWC9S81VLYuF38VPrmXgjk31pSpO3Cg0tYeW/z92Bh3AeNkV7htefsjUl/HcYizb9Zi6UqdhF1EjNa6zAW85HM0W7FxiH5D9kDgHqU+9ZjeuX33tcms2YCQLrvaXIe2C9mLPHL11SxKi3sMe5PjsBgFXUn2rmIu9haomt0YPOQHJdrJxhBy0lu68rBfVKp5hTnNWCJgyhyaQlfvPdddX6VGuvVp0NWp6VhgVQE0+SlmGjWoG6//0P/m/3+FlwFv/5Sle9kaOagoqeTYKnxGJizK/5w/cqunbcZorha02wfzxQFGEKED0zWZydVaAGL/O1tT9Su0yt5HE9Beq3m8nLhAMTdj3EvM94fiWNO+3fTG6uLjZRvx4Y8K3rU9j3DSP7L8CpNuV2ZtiyyA/yv7KqZ5wPzNstHzjWmFv5c5+rMXUMbkAaKabHChoC5uiSAp5N5yhnUPuRffRbjJOVKzRK4Q== X-Forefront-PRVS: 08331F819E X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BN4PR12MB0852;23:himF0rTGijQNv2xCARgX9lz9Nd6SEHuIfqLgi+H+f?= =?us-ascii?Q?snOc/5rXgsM/tUPN8hMcswYm/oSXqcbNX8jWF7G0Pm1vGI/GAj2J+K4FfX6U?= =?us-ascii?Q?GUzcLASvYNpiNRgcYwAZjMS0FaT0QcWjh9pkNWQ0MZISnC8TStizC68keq/h?= =?us-ascii?Q?aFzPC+fgxA+2hNafvHwbaDw4JSKy3axQmhFjsFoIxdZ2ouJm2UkMvxIDNpFm?= =?us-ascii?Q?Vwwcl6MENoi6wzypoc+pF9QFfCP1amXPESbfR3DrE1Eyw7hjJNiEiQ3HYPJU?= =?us-ascii?Q?j6DW4pS1a3zU3Sc/LOl9rNu5Z2b4TbMQ+gkMLLICc4MShn2bZJfkG8LDtQRk?= =?us-ascii?Q?J4wPCGEltEYSRejushRfmJ7uscr1JPbgdoqkmYi45wsRDQ/ehAzBracaaOur?= =?us-ascii?Q?V7i+eCynBRImiUdnbopexC8f0AW4WKMemTIBPPl0rca219ltQH4yEIRZFu4I?= =?us-ascii?Q?KT34m075HRxQGZI0dSb9Gdx8oWplf2VA+xDFflZMHSM6ZdiD6uPW64nbs0t5?= =?us-ascii?Q?20psboVyY11FN3QmFbCnO33jecxt3WkcljcJBNQiBvjUcGne+DYZTu7M+nMw?= =?us-ascii?Q?ZQU8V3Sm+oKwTdyxblxweF9+Tir8unJIFvZMwHse1XBYbU76+4m4rGuIcOBz?= =?us-ascii?Q?lboBZF/rah+G/m80bRF+RpBARNH1uUDWXUMCiKrM+BtMY+YQiHsrVdRLcxUs?= =?us-ascii?Q?eLV+rwJNVIOXJDbToFCAH6c4YmDBKq5FfH17o6116Qo7N6QtbRhdvJdHMarP?= =?us-ascii?Q?g8eijSD2VGEaiWGdAPPMohFWiNqnCT5/WQcebr/MwreEnfBHxTJGhlC4APEh?= =?us-ascii?Q?IerNE5bcoXaLMIbzOniP2wdBKIJ8TFboDbyQm3eV4xM/MhwENvm83onBrzsE?= =?us-ascii?Q?rJwT6qAuAdj2WDKtD2Jb+sVxWnDVfpoJVc3uRf7Jh/j3yeJG1LgWSTyfyFKh?= =?us-ascii?Q?mwUYD3B8rmh6uRilldppOwtjL0F1eE4cton0nzrrnrclJnuATiHM5DmeND5M?= =?us-ascii?Q?hOmw6cVlxcRlFU4ld8y48RF7dRFke++APig6yyh+vkTgmLyFtWSEaEkCPi5S?= =?us-ascii?Q?JvfB1/ERbyXwTtn3IkD3i46IcrN966Ms6NPdEqD/K2SQTWOxg=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;BN4PR12MB0852;5:sdY+pUf4KgALGQfnA6kUJI3nzdMIqQkoB4W3RfN+2EYyUKYK0poT4FjOKl/MW9KRR5UNULOnTMXuZv88D3ieVhAZ7ED4UByrNHLxdFZCGHLq5nWuY6u6zNsJyNtUgW/D6LhryOQQ2z6VLjZcd5FLxg==;24:EW9hIHIANQAwaVpe+RebrVU52F6Tm25UL9IL2MetJrHairl3lVBxW5ChVgRQmqz31YOlFQZFz1BEnsZpoSzHf7nRga+AU6Yw4TPbuIFzZ9A=;20:3iJcWXcu7yG7jMKstu9heslepWNuAc5v6xkk/25fZqzMh42cjD5WA6CnfC0Ku60s6EeIF+qdzfXWf4+I7isxACsPHOx9/3YtbrHZ4aT54jRjUnYeIsLRpMYh1PXUfre6OMBMy1WX0RHlNENdrDCmudt5WbS+XD/r358guJ+3ZWjmlGczLEwTo27AjhOP0fVx3jz+n/qTsZaLBSJXx4gj3cPwIJvCPcYO32L+xxMzRb/GjmLGL7fWwY10KBQDXFVk SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2016 13:46:55.9664 (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.221];Helo=[atltwp01.amd.com] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN4PR12MB0852 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 26, 2016 at 09:28:23AM +0100, Ingo Molnar wrote: > > * Huang Rui wrote: > > > +/* > > + * Acc power status counters > > + */ > > +#define AMD_POWER_PKG_ID 0 > > +#define AMD_POWER_EVENTSEL_PKG 1 > > > +/* > > + * the ratio of compute unit power accumulator sample period to the > > + * PTSC period > > + */ > > > +/* > > + * Accumulated power is to measure the sum of each compute unit's > > + * power consumption. So it picks only one core from each compute unit > > + * to get the power with MSR_F15H_CU_PWR_ACCUMULATOR. The cpu_mask > > + * represents CPU bit map of all cores which are picked to measure the > > + * power for the compute units that they belong to. > > + */ > > +static cpumask_t cpu_mask; > > > + /* > > + * calculate the power comsumption for each compute unit over > > + * a time period, the unit of final value (delta) is > > + * micro-Watts. Then add it into event count. > > + */ > > Please capitalize sentences consistently - half of the comments you added start > lower-case. > Some of lower-case starting cases are not complete sentences such as: /* * the ratio of compute unit power accumulator sample period to the * PTSC period */ /* maximum accumulated power of a compute unit */ So can I check again and capitalize comment if it is complete sentence? > > > + if (cfg == AMD_POWER_EVENTSEL_PKG) > > + bit = AMD_POWER_PKG_ID; > > + else > > + return -EINVAL; > > + > > + event->hw.event_base = MSR_F15H_CU_PWR_ACCUMULATOR; > > + event->hw.config = cfg; > > + event->hw.idx = bit; > > + > > + return ret; > > so this control flow looks pretty weird. Why not: > > > + if (cfg != AMD_POWER_EVENTSEL_PKG) > > + return -EINVAL; > > + > > + event->hw.event_base = MSR_F15H_CU_PWR_ACCUMULATOR; > > + event->hw.config = cfg; > > + event->hw.idx = AMD_POWER_PKG_ID; > > + > > + return ret; > > ? > Looks better. > > +static int power_cpu_init(int cpu) > > +{ > > + struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu); > > + > > + if (pmu) > > + return 0; > > + > > + if (!cpumask_and(pmu->mask, topology_sibling_cpumask(cpu), > > + &cpu_mask)) > > + cpumask_set_cpu(cpu, &cpu_mask); > > + > > + return 0; > > +} > > Hm, has this function ever been runtime tested? This function either does nothing > (contrary to the clear intention of twiddling the cpu_mask), or crashes on a NULL > pointer. > > ( Also, the code has an annoying line-break. Don't pacify checkpatch by making the > code harder to read. ) > OK, that should be "if (!pmu)", thanks to check so carefully. I tested to make the core offline and check the event context migration with power_cpu_exit, but might miss this scenario. I will do more testing on runtime case. Will fix it on next version. Thanks, Rui