From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756885AbcCCQTh (ORCPT ); Thu, 3 Mar 2016 11:19:37 -0500 Received: from mail-bn1on0098.outbound.protection.outlook.com ([157.56.110.98]:53782 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751435AbcCCQTf (ORCPT ); Thu, 3 Mar 2016 11:19:35 -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: 0O3H1C8-08-98I-02 X-M-MSG: Date: Fri, 4 Mar 2016 00:18:11 +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: <20160303161809.GA2791@hr-amur2> 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> 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)(199003)(24454002)(189002)(164054003)(86362001)(189998001)(5008740100001)(97756001)(92566002)(106466001)(105586002)(110136002)(77096005)(81166005)(1220700001)(2950100001)(1076002)(101416001)(4326007)(76176999)(50466002)(50986999)(23726003)(1096002)(2906002)(47776003)(33716001)(54356999)(586003)(11100500001)(46406003)(4001350100001)(33656002)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BY2PR12MB0710;H:atltwp02.amd.com;FPR:;SPF:None;MLV:sfv;A:1;MX:1;LANG:en; X-MS-Office365-Filtering-Correlation-Id: c48ea7c1-b312-46f9-9a4d-08d3437f968c X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;2:+aF1WuGdjGJBiGKIFmBjyr9LB40o1E8TOaVTd0L8r1L+FaMyed3XO7FoIOZp/RGT6oKVRLzvKLNBgnWwp/FiqRzrfBZWP7p4ntuIWZmv+q3RIqcnHOzmNC2jtiPjxQMAQDT30GdeiIBAas9DmD6iFPfvPjcD3SwxaD7zCMJduLSuWV/ygoRrzFEpqgQWy6QJ;3:EiFRd9fIcFIOnYbS7EvT2dXoM4VtrSQKf+F7bWHBIAQDnlehkW849KnBS+sg8eExk+Z6lKievP9Cuccl+LTqxQE6Sb1+W3O9bgZ4H1iXoQFHoz0sEE+iNizQR44CopmeAfV0NZk6ct70uLBJszsxt5cxM2ISkTKhE/qSp3dSojrxCtRJRrii7EpPMeUt88KT7rzLrKkklAWXLMP40MkNaTv2xRqwOs/YI2YdEdVfHQY=;25:13hn4e49EZ7HxqkhSJOt2KEMTfS6NzcJJV6UrmcjRhdDvkWIW6JuGOkmmhhDOrNpfF5PgWSc4w5+eoRNFRwj4zWlqDzCb2Fc6oJGI0M8WP93UIY4AN9548hwqf9tIB+HqKf5KNBLuW1Z2UuMy/A0bpvtxRSssc+/0FuxBdMxOxebRGU0ilkVj+VraIAhS2B/I0ffu5BFIarGIYZHvJBozzqTbJa5FKVocg8SKK/eH65YQw+4LbQpg+h1oE7cwyukCwE9UY8P92tEHAf39M7vGxKTJgiLuuZ2WUrBmChscvavOHjYZQq1oBtLo4GQePuRQEKEiEip/+TpiCmwAYqz8g== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY2PR12MB0710; X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;20:zY48Fl/QMhYDcQTzBNau9VZqq+u24348PDjLTdsi2nbuE8aEkld+lq80bWB1GgE5ABdJbXT5wpoA1Ci107iwedqiUlu8xSkkhcbIdX5oOpjzrv5RdNEiW4FjanvI0eT2lDxfW2USqn6kf/+hI3ie8zw4Xjm1Hb1csYRCrlV8HLqnqs7qY6t0iUeA9Ut1O4yXjw7QZz3qLjeWkGmaqRvA94CTA1gObuF9+mqlnFSvRjTDWK1iUIQuH/mvZo2aajmb3s9/zffoGOkvGDdaTwM9HKkqq5A8jYUYahC8fgI1o8VDibyamu/4vyIAUGAWkzVH3dBLiXZxgTOojr0p+FjDNV5e6+K/7IJBvZ8gTXHxEoaGkSGiKFBV6FyvixtZpBD3Ta5y7kEls4Po+O1/lSGKLB7l8j9ZTQZui3SFKswX6zM68wJVPqdGW2mY3tnZCnwIuFc+8kC+KDk2+cCRmnJtj5AtnvVNJwKybczzWa/2mEPDtJXkD7R5gIgg/Dk1Effp 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:BY2PR12MB0710;BCL:0;PCL:0;RULEID:;SRVR:BY2PR12MB0710; X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;4:dJkrNb5rPld/Qxnp34HammXMyp9joi2ikyslIJ272Jlw805v2gZzm+MwpYhrGfLRT6aIr+np1z4SuWeO4FAkxhqjaVEX4lRv0/Cda0Sa4w9QASUgS7Ii+ZIQ8coLmLYI9QcTehmcrn4YVcw8wGPOT/u0JNfVqdUxtYTwogQE4sUgV7wElcqvL+G9Pf3JPOGJMkeS7Z4wqJcX3cHZucn3DVnFD9MXuC6l3c3RTgKxldQJEblw/16tMkGRFlc+crBchCVQOOI++/Ievr/4k4/nCdY91JObMp/QD/CyXsrEjqNgkSe+sm7xCYx34V6eNiaxnweSNPclfOFTjWBNpStPE+N2UWTxQcMFZST2O2ZxSwvt8ozWjv3rEi6BE5BwtCJtLCnuwqbt/yWS9VnrlfeZAVWc9z9pdF+YLrWOKGxZu/NvBzTOVDA0Qq061HlSSJyxk2pk19imUsJcK7NpJuX1MA== X-Forefront-PRVS: 0870212862 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BY2PR12MB0710;23:HYJ4cgcKV39UHX2X71lK3N2lihmmPtfAaTqx6Xxbv?= =?us-ascii?Q?AJ5k/+pVCr2/g5s/vYnAQmzw94gVLiVliAfzSGeSc2W4NlY8Z+47Y6Mu3nPj?= =?us-ascii?Q?/56NP441vI3hTicYhH+08Yl4RPPsmqgS0+VF2xg0kOTWsbeeII+zg7VDuOTX?= =?us-ascii?Q?hM9g6CZZt0PAi7uL1YyyFjW2SqKTJRq66hx0ZsBzDT9c92IT1IcVikIuXTzX?= =?us-ascii?Q?WRlbmZU7P1/37qHIyzgn/0IljEBGvoqyi182FPeRuATp69qqA4Ss5/wn7Pxy?= =?us-ascii?Q?KOSyf4t0scbnH4xtnNXCSDAxS99ZhaVT8ZPQxB6eVKb2czG/otOyawQwea4e?= =?us-ascii?Q?tMfvXUVpSjBQQ77mGPVZZsmpKHxJuj7OPQeAXyWmrRtViHkZrkhy7cB5T5C/?= =?us-ascii?Q?AHUBj9KAc+6pfvwKyetJzkKR+j/S6GbLQnyl60HMSmykuBWAHxYBVvjAnNjB?= =?us-ascii?Q?6piCv/y1Dq5Br8kLhvCKboXehEW4KzrffXr3W8ZL/euXeTXpSoQyYDuUfyXx?= =?us-ascii?Q?5hNkinRddEugj7M4AAFRQMFigAH63ejDBbQWzrJosO+EYajY5DprbjB/PGPY?= =?us-ascii?Q?rUttydbl2ty1qVdHocA6qp6BzgFE293yUWyLmR6f4WCECt1vKjYR//SXMAGD?= =?us-ascii?Q?H9o+08H2P/u0ELnpC6HIJoajTxJ5EYQZm9YBCSCu4m4GIZqq1x7YbLMqL3X+?= =?us-ascii?Q?Em+3jUsjCU0Trge+LLcmbKAnkaz8on16LaOPnRCuFXL7KHBBvtaR3w+5+kuc?= =?us-ascii?Q?nhn34S/jrGt3W7kOmBTKbEtFbUv+Ab9NqRt1WrS10QtO7sUB+RGMra4sIeJH?= =?us-ascii?Q?Am69eT2anOW2OxZjHIfIjtD3td0P9f1nE3jkveg01fpaiPAiXrF+7jo3bngZ?= =?us-ascii?Q?ELq10h6pSnghdxTZMryDbREQ2B3/VManlXfNOurb4GGeiPo8NgJMpvfpUR85?= =?us-ascii?Q?sw4l/H4hQm3nSHdqnjKAaxa+7pA3SCN3VxOfzCpZw=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;BY2PR12MB0710;5:JjDt4DC0xXjJ/mBquM0Mz9lzCbEJ+N2dnjoiInb3kq0cFTNQKGuwcxvYPYI3Lmv+IlcJhuwI3OoJs9LBnAlkOJuhEpcKJl4iBLt0JLa5oqp8CgklNrSATjmEREL3RVoHgTlvKQkwRm5Y7kJOSIWkIg==;24:pZ4UdR/ukYdHrRQZbI2VhJiwWeMYxUyPgqS+v2nIAQcomNzsIwgxR8m7LmNk7zGKSLS2lYQ+AsNNVMRAemwdvh0yC/RA4H7WwNyChFeTBNU=;20:AufvPmWbCfHM3OYbU/iwhMifBfgBFNnjwgRQhhCUBZDJEP9OqpTlSXmbwrsKaAm6JnBE9+IpyG/GX2s48BqD3XEiElfQNUBug697vrk1cRqFNIR8D+igkLy+RU/GKgGVY8Zn4I8VQkLQIlS6jmvQCJG6aRHji6KfSU6WQdvyNW3Z8cyS9ErBR1+MjmndvlORxlMHzciaQo1YehE62oKv4ddn6Ws1nzcByumXQoiesfVVNVJv+rgTgQsPvCUTKndA SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2016 16:19:24.4138 (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: BY2PR12MB0710 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 03, 2016 at 04:26:46PM +0100, Thomas Gleixner wrote: > 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? > But looks like power_cpu_init cannot handle it if we don't take any action here. e. g. cpu_mask: 0000 and online mask: 1111 -> power_cpu_init(0) -> cpu_mask is still: 0000 topology_sibling_cpumask(0): 0011 target: 1 (i. e. we cannot do cpumask_set_cpu(0, &cpu_mask)) Maybe, we need to think out a stronger power_cpu_init if you want to only do init thing at for_each_online_cpu. > > > > + } > > > > + > > > > + 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? > Yep. > > > > + 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. > OK, I will remove unnecessary cu_num next version. Thanks, Rui