From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965291AbcAUOmq (ORCPT ); Thu, 21 Jan 2016 09:42:46 -0500 Received: from mail-by2on0070.outbound.protection.outlook.com ([207.46.100.70]:62341 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1759583AbcAUOmn (ORCPT ); Thu, 21 Jan 2016 09:42:43 -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: 0O1B4V0-07-E8A-02 X-M-MSG: Date: Thu, 21 Jan 2016 22:42:35 +0800 From: Huang Rui To: Peter Zijlstra CC: Borislav Petkov , Ingo Molnar , "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 v2 5/5] perf/x86/amd/power: Add AMD accumulated power reporting mechanism Message-ID: <20160121144233.GA16294@hr-amur2> References: <1452739808-11871-1-git-send-email-ray.huang@amd.com> <1452739808-11871-6-git-send-email-ray.huang@amd.com> <20160119121250.GA6344@twins.programming.kicks-ass.net> <20160120044823.GA13477@hr-amur2> <20160120092244.GH6357@twins.programming.kicks-ass.net> <20160121070437.GA15130@hr-amur2> <20160121090257.GC6357@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160121090257.GC6357@twins.programming.kicks-ass.net> 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)(199003)(164054003)(189002)(24454002)(47776003)(2950100001)(93886004)(33716001)(46406003)(83506001)(54356999)(101416001)(50986999)(97756001)(76176999)(50466002)(4326007)(97736004)(106466001)(11100500001)(1220700001)(1096002)(23726003)(1076002)(105586002)(33656002)(2906002)(110136002)(4001350100001)(86362001)(77096005)(92566002)(87936001)(189998001)(586003)(5008740100001)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BY2PR12MB0710;H:atltwp01.amd.com;FPR:;SPF:None;PTR:InfoDomainNonexistent;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;2:Nj5A/AvXGCfp/METkqyBfOY2S4z/ZoZrTaO+jHx2c3GbLpwIiK+k8gVpaftkgrXxyf2cmxK2MaPCxq6bRVmw/FdVwCse9ME6ZMHcqCXO0suCCD/Ek2sNH0qWniTQ/NOhJsZp+ozaQ1Jwe+GgfDRJsA==;3:HqmeQJjMeseu0oSP/7QTQB0PWqYtD4ps8UaaUlOyhV3y2px/DqBWhHSaJR4o9KrKWuCuRK2+JApJRxDVq252shsrmli1YRp/R2hdg2IoKbAxxtx1WdP/GsftohHB/oTs4WTx+QZfcGfv5GLx0gj8s+lYSGq0VO24PZYLz05tv6mIGdUEjb5EsB6L1awPYOmhJyd1ziUvxVikS+J/fRFT/hPwAe/BG1Ic8nEIrNwudhw=;25:dmbILlBwIAvGwnYkaBWK3pNFh153CaTXH165/ZYax3FFes+8PxMHFqcM3sY64Js7eYdDFQQMMVd+i2K9aoaEAQAIhXqSA3j7kK3JAHGGH0HQ7g4/U11AHLbJ45lGxqYkjZo6pNeseZnaggwb7QOO5BJyxiKp5IHBMPqU7EVtVWwvLh0KdMqEpX7qkknWozuYrO5uB5VTOweYv7UynObuccp+khnZfgGhTRAozSIGziVQv29lAXZZo0pFzm4zS6va X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY2PR12MB0710; X-MS-Office365-Filtering-Correlation-Id: 17029951-6a71-4f75-0881-08d322711cb4 X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;20:BOfwHk1tGuwdtFWvMSNKln0Arxk6+nqQAr3BnPxzi861cXUtW9ZqLyZtsZ24tAoH59AVqQGozWcrGUt1GubOn8q3mVDNiHNg0XhMVLPgXisOH5OzqbZ6GUx6Pqs+HsvJcUUnARtleBtHY49aTlgQo9rsYBTSvAj6Fj7LnTQRA7PBD4z5dimplFgukA0q6Tbzp38m1cuQ3wiK03UPUEcf/wvm9GqRe1XmtZXPWfstxOyaTQ+pFUVYmWt4BgFpBCEdq8WJEuH5BGbqsFXHXNP2o/vTSnf+TdqXmv+LHfyf6vDSkanTW+F5diTrNrFUN+Oh9auDnNw804yH+Hjqs4ZGf31e9B8aBNqsNQlKWg6ptb0t0hJ27J4oTL95wadUfYBgUvldLVnNxu4WMSQgWH3Tt5abA2eMDQ67OaCngLfkWxlM6LsNahW1fNEAMDJxnN2zEC75ZVdagDaYAk4x4dfzd6GGHnaY8KpW2hGf++OmIFmMqqbMIV+M9u9wceFIP3Qv X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(13015025)(13024025)(520078)(13023025)(13018025)(8121501046)(13017025)(5005006)(10201501046)(3002001);SRVR:BY2PR12MB0710;BCL:0;PCL:0;RULEID:;SRVR:BY2PR12MB0710; X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;4:h4sn7JkfuBaoNKbS9j1WjAXnW0uTmGJJ1U2ZzKgm/T62A2gCT+gDVI+sMopKxwPGTdLzaOzLPLzUJG1eK5lyxMuhoyuYJhjO2fYRdZEZeIOHkQxUNfB8b6XR6WyFE4olQukT6vz+D55CXqQr53jEnpE6W8jN34HDVbSN/yASawZ9pXwGJ2q5cgLklQ/c6rrxH3xmibBqM7JGOpk334KvCUrAuPC3RkeU2rTaTjCvT+BMz4jKXDrHtnsrWSGm4pEcxRLVyaq3bRtzvXS4+cs1ATN2yyegU2rdXWp1cBfDsHYwAv6OezF38hJVsosw0CscnOYaDwHVXIBWzmWQ1n5cofOV5+5RixXGVTJAFIx21zlYhe5YgjEegB7VvZBZ20zMYpDkOXUNsDSr3Zif1WhfB2q8BUj8RBd3TQXPsB468H7C5UjfsTpidx/H9kOr86fkXOJJ8fFNLzBLBRSEfdtY4vxKaFzckwm67SU3/CwxtpA= X-Forefront-PRVS: 08286A0BE2 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BY2PR12MB0710;23:m59nqoQSvPqC7Nad3aSoVWnPiflO1VMRK2Ap3yna3?= =?us-ascii?Q?R5Id/dP02jmh8ebChlNImP3LQfZYQ97K4TjrmqesRzsEHSsxaXRCBGo+6Fkp?= =?us-ascii?Q?N77EhcCYCg68sx0oeXiVjAjK/wRMgUMljvPVYKDtHuoev0MLuyW0zofwSGr/?= =?us-ascii?Q?vmF3+yF1EKiskL+b2kV5xyE5Mrj0NluRmQvV2HCaKUbzT9FJOZXC/Cf2fMnS?= =?us-ascii?Q?hIQ51QuUUYolLymzpL4KjfIse9YaNx2gRjxO1OB96anS00YQ5IDva5EqvlsZ?= =?us-ascii?Q?PkPHwWeDf0bGkReR4XS6/zmWEQ7ZtO0YKsP7nyDJGcabh2+4DOXyW7Cq6lbM?= =?us-ascii?Q?0u5nqhE/MF2tzXclUh/gyyZqdhRp6O5h782J2d8maEbEbHkwL0tlDfhwcAnX?= =?us-ascii?Q?cvU7jpUhcBBlaS7W2hN2vgw+I5YPkNV3S5q1Yci1cur3rNXk/LtOZKhnAJ2r?= =?us-ascii?Q?JL+CfZRzirf9CxCB/O84vC5RACGjtG0KYtsiwh+9svYz2j3CKrEC/vDFfIlI?= =?us-ascii?Q?+V7mJRtG9B4N2FbJvuc8iGsZ9JYn4lhMaTTGDEN2SKosXD3N3VO3TiQwnnDX?= =?us-ascii?Q?589E4oJN8j/u8q0M1d0k3Ay+VQEP2X/PEIF0fsbd7qfM9/mpeL9cTK0T+R89?= =?us-ascii?Q?drvLG6odJ247ZkhU3z30f8pujns0mxvhaXkVvwi7CORI+TFk6JEkgA1miJxh?= =?us-ascii?Q?/pqYvmVX9kyPjSvzLWnm1I9TnQL/nAiqzCHJsQOxUV8fGWPID+iDgCDg8+6Z?= =?us-ascii?Q?ib8e6FiAAZLxQGdMGuAS3yjIfl8FqdZSkY3wnC90OhB2zUluDBKrND/MTf5R?= =?us-ascii?Q?mmdhgkKmzgEbHNZmkIowyPz5d47uwb+yc8fmyu1IEMaLiAqxTTmxqTSLLG/w?= =?us-ascii?Q?MeKQbmmGhnthPpSIIF4gEdMkbWO6dkdIH24WcaX3ddzA0femaQSt83/1SSuG?= =?us-ascii?Q?g6x4gxV+7nF6fMag0tT1SJNe7W/4wr0Uc9X+ZGirFNBmfgefQ1pWlAzpO4BP?= =?us-ascii?Q?FKdDXa4tvAjrVjPin2Tmbsw1Yv/J4WTWGZn5GPXrhYxKA=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;5:HPPXhd45tuHxi+jU3RUchogW/65x7juEciVlwySqGF9yk5bx+wHeFaQSHF8qZ7D1xytNkvc4KW8LAJaY8rZcb1PEu3wdQ+uw6wpHVql7cZLhSDCcJfpiURHMyNVW4lp8p0rgbufrRUOLr+Lw2rtGEg==;24:3gT+u+3a/1kMzywakw+Nb1gD8tUXnLdvXn7N8IZ/gY3zaKG50ZXbcoWgcX81qjP5n4bgq2XFHaeGp+WwJgtKDZLVxATM+taiI4mm0mEi3Kg=;20:wg3VEqmJDvUTwFZ5Rc8ge8ndVFPEsvLiT0s5xt16Fiu7CGzJMIujZk6q9l/ypKmwdACpKUsXzlvoKi/REX321bT2UKUiCH0pQW59K7v1lkYC+wIzbx6LXNeL9zuYTEGYQriC63E/t3yWWFhjQ+DUlEyzb3RlbdMxM4viTOgRV8zVeoSlP3uLui7khLf5MdmGvFA7y1mLHASPc5R/K8Gd34moV8IaJWdrnBQNNzcoJOwgRFhywUnkwW8md0CahmRy SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jan 2016 14:42:38.5393 (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: BY2PR12MB0710 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 21, 2016 at 10:02:57AM +0100, Peter Zijlstra wrote: > On Thu, Jan 21, 2016 at 03:04:38PM +0800, Huang Rui wrote: > > I just quickly looked at about the spinlock on -rt mode. Because > > realtime linux kernel provides two kinds of spinlock, the original > > spinlock_t will be replaced the one which is able to sleep, actually, > > like mutex. And another one (you mentioned here, raw_spinlock_t) can > > keep on non-sleep behavior, that is the real spinlock. > > > > And my lock here also will be nested under perf_event_context::lock, > > right? > > Yep. > > > > I have a lockdep patch somewhere that checks these ordering things; I > > > should rebase and post that again. > > > > > > > Can you CC me when you post that patch next time? > > Sure. > > > > One should not use GFP_ATOMIC if at all possible, also no, -rt cannot do > > > _any_ allocations from this site. > > > > > > > OK, that's because allocation might sleep when IRQ disabled. That's > > incorrect. > > Right. > > Its related to the above, the allocator locks are spinlock_t and as a > consequence of them becoming a blocking lock, spin_lock_irq() will also > no longer disable IRQs. > > The CPU_STARTING notifier however will still be called with IRQs > disabled because it is CPU bringup. > > So on -rt even GFP_ATOMIC will no longer work here. > OK, thanks to clarify it. > > I draft an update diff that based on original patch, please take a > > look. > > > > 8<-------------------------------------------------------------------------- > > > > diff --git a/arch/x86/kernel/cpu/perf_event_amd_power.c b/arch/x86/kernel/cpu/perf_event_amd_power.c > > index 69ef234..e71d993 100644 > > --- a/arch/x86/kernel/cpu/perf_event_amd_power.c > > +++ b/arch/x86/kernel/cpu/perf_event_amd_power.c > > @@ -46,10 +46,17 @@ static unsigned int cu_num; > > static u64 max_cu_acc_power; > > > > struct power_pmu { > > - spinlock_t lock; > > + raw_spinlock_t lock; > > struct list_head active_list; > > struct pmu *pmu; /* pointer to power_pmu_class */ > > local64_t cpu_sw_pwr_ptsc; > > + /* > > + * These two cpumasks is used for avoiding the allocations on > > + * CPU_STARTING phase. Because power_cpu_prepare will be > > + * called on IRQs disabled status. > > + */ > > + cpumask_var_t mask; > > + cpumask_var_t tmp_mask; > > }; > > > > static struct pmu pmu_class; > > @@ -126,9 +133,9 @@ static void pmu_event_start(struct perf_event *event, int mode) > > struct power_pmu *pmu = __this_cpu_read(amd_power_pmu); > > unsigned long flags; > > > > - spin_lock_irqsave(&pmu->lock, flags); > > + raw_spin_lock_irqsave(&pmu->lock, flags); > > __pmu_event_start(pmu, event); > > - spin_unlock_irqrestore(&pmu->lock, flags); > > + raw_spin_unlock_irqrestore(&pmu->lock, flags); > > } > > > > static void pmu_event_stop(struct perf_event *event, int mode) > > @@ -137,7 +144,7 @@ static void pmu_event_stop(struct perf_event *event, int mode) > > struct hw_perf_event *hwc = &event->hw; > > unsigned long flags; > > > > - spin_lock_irqsave(&pmu->lock, flags); > > + raw_spin_lock_irqsave(&pmu->lock, flags); > > > > /* mark event as deactivated and stopped */ > > if (!(hwc->state & PERF_HES_STOPPED)) { > > @@ -155,7 +162,7 @@ static void pmu_event_stop(struct perf_event *event, int mode) > > hwc->state |= PERF_HES_UPTODATE; > > } > > > > - spin_unlock_irqrestore(&pmu->lock, flags); > > + raw_spin_unlock_irqrestore(&pmu->lock, flags); > > } > > > > static int pmu_event_add(struct perf_event *event, int mode) > > @@ -164,14 +171,14 @@ static int pmu_event_add(struct perf_event *event, int mode) > > struct hw_perf_event *hwc = &event->hw; > > unsigned long flags; > > > > - spin_lock_irqsave(&pmu->lock, flags); > > + raw_spin_lock_irqsave(&pmu->lock, flags); > > > > hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED; > > > > if (mode & PERF_EF_START) > > __pmu_event_start(pmu, event); > > > > - spin_unlock_irqrestore(&pmu->lock, flags); > > + raw_spin_unlock_irqrestore(&pmu->lock, flags); > > > > return 0; > > } > > So for these 4 {start,stop,add,del} you can drop the irqsave/irqrestore > thing as its guaranteed that IRQs will be disabled. > OK, I will remove the lock. > > + cpumask_clear(pmu->mask); > > + cpumask_clear(pmu->tmp_mask); > > > > for (i = 0; i < cores_per_cu; i++) > > + cpumask_set_cpu(i, pmu->mask); > > > > + cpumask_shift_left(pmu->mask, pmu->mask, cu * cores_per_cu); > > Couldn't you simply use topology_sibling_cpumask(cpu) instead? > Looks like we couldn't. That's because cores number per cu (compute unit) is got by CPUID 0x8000001e EBX. That relies on the CPU hardware. > > > > static int power_cpu_init(int cpu) > > { > > + struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu); > > + int i, cu; > > > > + if (pmu) > > + return 0; > > > > + cu = cpu / cores_per_cu; > > > > for (i = 0; i < cores_per_cu; i++) > > + cpumask_set_cpu(i, pmu->mask); > > > > + cpumask_shift_left(pmu->mask, pmu->mask, cu * cores_per_cu); > > topology_sibling_cpumask(cpu) again? > Ditto. Thanks, Rui