From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752534AbcAVIFA (ORCPT ); Fri, 22 Jan 2016 03:05:00 -0500 Received: from mail-bl2on0057.outbound.protection.outlook.com ([65.55.169.57]:11470 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751617AbcAVIEx (ORCPT ); Fri, 22 Jan 2016 03:04:53 -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: 0O1CH3Z-08-HU0-02 X-M-MSG: Date: Fri, 22 Jan 2016 16:04:40 +0800 From: Huang Rui To: Borislav Petkov , Peter Zijlstra CC: 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 , Fengguang Wu , Aaron Lu Subject: Re: [PATCH v2 5/5] perf/x86/amd/power: Add AMD accumulated power reporting mechanism Message-ID: <20160122080439.GB16975@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> <20160121144233.GA16294@hr-amur2> <20160121151040.GO6356@twins.programming.kicks-ass.net> <20160121165958.GF21930@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20160121165958.GF21930@pd.tnic> 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)(199003)(189002)(92566002)(97736004)(54356999)(46406003)(101416001)(586003)(86362001)(76176999)(2906002)(5008740100001)(2950100001)(23726003)(77096005)(5001770100001)(4326007)(11100500001)(1220700001)(50986999)(1076002)(4001350100001)(47776003)(1096002)(33716001)(106466001)(93886004)(50466002)(97756001)(189998001)(87936001)(105586002)(83506001)(33656002)(107986001);DIR:OUT;SFP:1101;SCL:1;SRVR:BLUPR12MB0707;H:atltwp02.amd.com;FPR:;SPF:None;PTR:InfoDomainNonexistent;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0707;2:3fDG7wUnP/wi9LdZmhlq0K9/44cx0KiYjo+iSsxoC081UyNRmmnneHZQNr9IXyehBBTW5eMgjBkYAEemTM3wx9f7dqBHMLbqiiHJvnhW3PnnMsPOQEQD/LEOYcs+EWBjHYOmPAxOtjRMdQv1hFKDqA==;3:mF/Xazy6YZktHaFM6ERQ3oZ3A8eUSJzWn3GhC7iTydeZ0paYY2NH64c1EG+fzu1Ih/Apz8WUWTaVzOiHn1m6MimTf7nBRMFh7W+ndzGnpHdwOKBS46X0eZ98FyZycbGWrWdx3h7IP5Eq6DP7shgmd9oh1m/ohYtm5nKDyhCOnr9czA8S+SCY32jLDd/btr1Ol54gxsOphbDqK6B1oHnHVObsFSfDcWEeP6x1ZXslzCQ=;25:0mcIWrZDXPDddbSItg/nbf4B4wJ1coP9lQ1bHpo1wExw4jn4nIBm/yDvxDpQJJRgVlZ3aemvPLbWKkPGhvYcSnNycOanIuMN9XgnRRdTuuXvo5NZw10DEy/j8DeHbWc+VDy1TfReQ31A/M5XVL+c7uXVAbEd9S4AfsWCmWrXnkNa+f1tg5u1+XzGBMJwYAkOFQCX1tce6Z9eAMc0A47ikCal0tiE2uu8wMuGTcoFkymBz+pEYtBK36CGP0Kv/mlj X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BLUPR12MB0707; X-MS-Office365-Filtering-Correlation-Id: 90a637de-b485-4e63-961e-08d32302b38e X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0707;20:eZ9Wi+1OLq9w+lwoSsZP3OtQfs8QPtln8OhEc0raxVccS7gfY38O1QgC9hTu6nt7Y+lBRSRu0GXwz3eezihGXL+XloebjRM/OCKw+ZwLsPjTstflKRqZP7a8Yqnq7yY5+S38sMuW7EGMu5WPoub5gTH+Z9Amp6av8EPzEGlp3f7tSN9pWYF01h7/Xf/3TDaapmTw7afp3clPgRJvD1VEQLPK+VXirVfm9KI88xMZtXIQ29DVxbyy3BBTeCTka7whcBMtjaQabSjLKPgimFMDICetuLiFPFwK3PqjX2vCWU6a7rFhwP0+0Ms2A6MXdhcFfyGyTzTl5gLYir0ysN1FzIUbu3L1dJ3mZGNUNg/XdZi2cZHArmvjscArKv/uoLuRGL0BsfxW0Fpddh5HFSN6Zbw3in6nyUtqDXdz/zQaDbE/cmOogJZTaE8gwGmLKQcGiCoEaiWHz1V+wMgdW3pVZFlYIH0ysfWPLA5P6ml1qZL40YuFJBaTGtMr+hF5h4Su X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(123027)(601004)(2401047)(13023025)(13018025)(13024025)(13017025)(13015025)(5005006)(520078)(8121501046)(3002001)(10201501046);SRVR:BLUPR12MB0707;BCL:0;PCL:0;RULEID:;SRVR:BLUPR12MB0707; X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0707;4:tLPRESdB1a27CT8gR4DO45WNqoFj75GN6LWde6bvWWXCgX/RpW8afK7aKgo9F7H0FpUcpmwXkY1UCQKHN+eKjtXASwMnaF1ALTR2wRmWQnrzQwgFiRQMLIbEcjX1DVsh0Egz74XXHRPHyw79qz8H1InCiF1aT8wYVNOh2mKs7N7YmIJ4Njn9GWG67ujojeKyKfChKXlsig8Oza7bAjx9rEgcuxuPhbuvbg4oX6Vao1011c3u3bM/aJAWAN74d7vonfLYAzZczwBwhRXTosjbA2zaEGvv6TplTkHoPrNxhWb34FuTyDEdjVtJ6Tn9JzyPGajnQT0IONppNe/QISr+Id8KNrSjYgIUhCpZgj8l9H95SLBwTeTXQ0HCtDqs8qPpQI6tu/trjMreYT6LU/4RbuiNl7eh3Yx5AlxW/3HAEXrws/B1tCw+dINeyWa4pGixpiQRs2wVEJNNWNAcowNtn50xZe1uaFPfjKZDiVOJR1k= X-Forefront-PRVS: 08296C9B35 X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;BLUPR12MB0707;23:CsG3r9U7qhO5iga/ygGV/wKjPrau40z/4pf2HiY9F?= =?us-ascii?Q?YaDaKqjCbrX9LohI+oTEqjVniB//GWs6fbeT2OnmBN3qaTFNOi+FCv6EI/IS?= =?us-ascii?Q?DcJS4y2r1SBQ7Y1Vk/e4wR8Pm09hZLwlEnrJYSjChI07uyp1wr/ZfIhtyWqL?= =?us-ascii?Q?GBpTsi1a9/1KtB3/lzfsN8Uhk6n5gSIZKITw0NAqiCMXWFuHdds8TrWDr3ZF?= =?us-ascii?Q?L0xCVrRxkOsRMhDr/9BVVu66PVmuR5bUrZACP7K0bPHPqFSBbMl1Y4PiDzxB?= =?us-ascii?Q?baLlRmUBz20c+IjbYY+hc5j9ppX3QuEPAiMbweFxPthrrpKiEqGQsCPh4vbu?= =?us-ascii?Q?pHo6komv0QfIILJg9RyT1Cf+zhdZLifjpFe2EmJGQz3q9z0M8c+zFaB/icAQ?= =?us-ascii?Q?uLfjkR75/UH+jc/Jz5V0g6q7F98G9ypc+x2wh8nLnJyDeCRDtmz6T1PdZmF1?= =?us-ascii?Q?rnY5blw5R0VDOd6BFUSC0m2SJFA6rnXlD0tGI8DvFUSYoSfdz/zasxHOAZWB?= =?us-ascii?Q?nlDmjo93vgncXmdamg3cXoqohtpE4Jrrg2fx3CblK6FAyrt2C62WOtZYYasl?= =?us-ascii?Q?4TOEK90+3BG7Oyi7smxCmKAk0vwEwsgw7rV+gUvOqnYEUqsceSXXUS1LhUzE?= =?us-ascii?Q?X19mC2KrzjaX5JKnT4xy/0ifnSSHTQ+v53CI6cXLjuy0MoJPRzyBLvPLBT8s?= =?us-ascii?Q?O+gHC8wGL1FCuiiksu2Et5OMxbK3JR64m8hTZykPIW5qg+4oDsD9K49KzKkM?= =?us-ascii?Q?idT1MevG6h8w40o3scby6zk8GS7qTOtmytLDCUSod/qZbmRDrf4sstTQ7bFy?= =?us-ascii?Q?t1WCssQSIndNsbHZz2kZeDxKFO41aCPkuhP3fNXM3bK+y04OKjswe3SF3TNn?= =?us-ascii?Q?9GPjqboXvb25f+OnzB3IFHTiPx8Zqyu0XDcA9VY8M8fv+QnhHM0MUHRMvSBG?= =?us-ascii?Q?hO40SnDcXtItRYTOorkhmL+gTk7gG6N1zwMPJXJl/WpIp8Qu2/EXb2JSlzuT?= =?us-ascii?Q?tYrqQJGBjm7jDbHXIDa794UOTAFqzCWYIOa9NHVZ4LvTP3N8IfojeNwnmzSu?= =?us-ascii?Q?/nsoqc=3D?= X-Microsoft-Exchange-Diagnostics: 1;BLUPR12MB0707;5:qcvYOfb0dFCh1NK0JHbjRM1dApazCSmXCMPQ1ivEUieF4btw0rlFeFLmj04hcYCGw63XxAXDzDASp5vgbpDt73heg5/ppfeEmtBk0G1Nik/+y3U/TjgheYN/xskHVQlzlJfB1CqYZxuljYi3+Dj88Q==;24:6sw1qd17tbB90hVhpSMeUqZ/gOE0zbmsiQNMDqcoYWqsCiPHnumhSBvTekrhN/5y3FLEe32yWDLAbriERGIkKb5nU60uK6q/P9JHrH1wVzQ=;20:Z4nhJrjpHDtCw6omgncFx8PbyHc8lFXTmju6imhFFSwehxhKs0cgMXxh+GR0JDfUW5mgbB2PANdBVZ8GtkvD3ZvUPyV2UYOAZC8SN9zOt6s5j3AS7X4blFL6DjwSkP5bbz9w1+tjyANpIl9p9ISHeEu0aGBPyWa1G/Z3fKF93q8Encyd8ibhAM/a3W8fTVOoX2WQch2pIUDziWNpF2SaUV/eb1EiSzULN7h7UFg2al4mAl9F1uUdCmQy7R4FMKNl SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2016 08:04:48.7338 (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: BLUPR12MB0707 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 21, 2016 at 05:59:58PM +0100, Borislav Petkov wrote: > On Thu, Jan 21, 2016 at 04:10:40PM +0100, Peter Zijlstra wrote: > > > > > + 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. > > > > Borislav? I thought the AMD compute unit stuff was modeled as the SMT > > topology. > > I would think so too: > > smp_num_siblings = ((ebx >> 8) & 3) + 1; > > gets set based on that CPUID leaf above. And that value is > CoresPerComputeUnit which needs to be incremented by 1 to get the actual > count of cores in a compute unit. > > And that participates in the setting of topology_sibling_cpumask() in > set_cpu_sibling_map(). > > And that looks correct on my system here: > > $ grep -EriIn . /sys/devices/system/cpu/cpu?/topology/* | grep thread_siblings > /sys/devices/system/cpu/cpu0/topology/thread_siblings:1:03 > /sys/devices/system/cpu/cpu0/topology/thread_siblings_list:1:0-1 > /sys/devices/system/cpu/cpu1/topology/thread_siblings:1:03 > /sys/devices/system/cpu/cpu1/topology/thread_siblings_list:1:0-1 > /sys/devices/system/cpu/cpu2/topology/thread_siblings:1:0c > /sys/devices/system/cpu/cpu2/topology/thread_siblings_list:1:2-3 > /sys/devices/system/cpu/cpu3/topology/thread_siblings:1:0c > /sys/devices/system/cpu/cpu3/topology/thread_siblings_list:1:2-3 > /sys/devices/system/cpu/cpu4/topology/thread_siblings:1:30 > /sys/devices/system/cpu/cpu4/topology/thread_siblings_list:1:4-5 > /sys/devices/system/cpu/cpu5/topology/thread_siblings:1:30 > /sys/devices/system/cpu/cpu5/topology/thread_siblings_list:1:4-5 > /sys/devices/system/cpu/cpu6/topology/thread_siblings:1:c0 > /sys/devices/system/cpu/cpu6/topology/thread_siblings_list:1:6-7 > /sys/devices/system/cpu/cpu7/topology/thread_siblings:1:c0 > /sys/devices/system/cpu/cpu7/topology/thread_siblings_list:1:6-7 > > and when we look at what CPUID reports: > > $ cpuid -r | grep -E "^\s+0x8000001e" | awk '{ print $4 }' > ebx=0x00000100 > ebx=0x00000100 > ebx=0x00000101 > ebx=0x00000101 > ebx=0x00000102 > ebx=0x00000102 > ebx=0x00000103 > ebx=0x00000103 > > We see that [15:8] is CoresPerComputeUnit which is + 1, so 2 cores per > compute unit. > > And slice [7:0] gives the compute unit (CU) id of each core, so cores 0 > and 1 are CU0, 2 and 3 are CU1 and so on... > > So Rui, why do you say you can't use topology_sibling_cpumask()? > OK, you're right. Peter, Boris, thanks for your information. I might need look at topology deeper. :-) So how about below update: 8<-------------------------------------------------------------------------- diff --git a/arch/x86/kernel/cpu/perf_event_amd_power.c b/arch/x86/kernel/cpu/perf_event_amd_power.c index 1f31157..d387fe7 100644 --- a/arch/x86/kernel/cpu/perf_event_amd_power.c +++ b/arch/x86/kernel/cpu/perf_event_amd_power.c @@ -301,18 +301,12 @@ static struct pmu pmu_class = { static int power_cpu_exit(int cpu) { struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu); - int i, cu, ret = 0; + int ret = 0; int target = nr_cpumask_bits; - cu = cpu / cores_per_cu; - 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); + cpumask_copy(pmu->mask, topology_sibling_cpumask(cpu)); cpumask_clear_cpu(cpu, &cpu_mask); cpumask_clear_cpu(cpu, pmu->mask); @@ -345,19 +339,12 @@ out: 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); - - if (!cpumask_and(pmu->tmp_mask, pmu->mask, &cpu_mask)) + if (!cpumask_and(pmu->mask, topology_sibling_cpumask(cpu), + &cpu_mask)) cpumask_set_cpu(cpu, &cpu_mask); return 0; @@ -454,7 +441,6 @@ static int __init amd_power_pmu_init(void) { int i, ret; u64 tmp; - cpumask_var_t tmp_mask, res_mask; if (!x86_match_cpu(cpu_match)) return 0; @@ -476,27 +462,16 @@ static int __init amd_power_pmu_init(void) } max_cu_acc_power = tmp; - if (!zalloc_cpumask_var(&tmp_mask, GFP_KERNEL)) - return -ENOMEM; - - if (!zalloc_cpumask_var(&res_mask, GFP_KERNEL)) { - ret = -ENOMEM; - goto out; - } - - for (i = 0; i < cores_per_cu; i++) - cpumask_set_cpu(i, tmp_mask); - cpu_notifier_register_begin(); /* * Choose the one online core of each compute unit */ - for (i = 0; i < cu_num; i++) { + for (i = 0; i < boot_cpu_data.x86_max_cores; i += cores_per_cu) { /* WARN_ON for empty CU masks */ - WARN_ON(!cpumask_and(res_mask, tmp_mask, cpu_online_mask)); - cpumask_set_cpu(cpumask_any(res_mask), &cpu_mask); - cpumask_shift_left(tmp_mask, tmp_mask, cores_per_cu); + WARN_ON(cpumask_empty(topology_sibling_cpumask(i))); + cpumask_set_cpu(cpumask_any(topology_sibling_cpumask(i)), + &cpu_mask); } for_each_present_cpu(i) { @@ -505,14 +480,14 @@ static int __init amd_power_pmu_init(void) /* unwind on [0 ... i-1] CPUs */ while (i--) power_cpu_kfree(i); - goto out1; + goto out; } ret = power_cpu_init(i); if (ret) { /* unwind on [0 ... i] CPUs */ while (i >= 0) power_cpu_kfree(i--); - goto out1; + goto out; } } @@ -521,17 +496,13 @@ static int __init amd_power_pmu_init(void) ret = perf_pmu_register(&pmu_class, "power", -1); if (WARN_ON(ret)) { pr_warn("AMD Power PMU registration failed\n"); - goto out1; + goto out; } pr_info("AMD Power PMU detected, %d compute units\n", cu_num); -out1: - cpu_notifier_register_done(); - - free_cpumask_var(res_mask); out: - free_cpumask_var(tmp_mask); + cpu_notifier_register_done(); return ret; } 8<-------------------------------------------------------------------------- BTW, "smp_num_siblings = ((ebx >> 8) & 3) + 1" should not put under init_amd(), we would better move it to bsp_init_amd(). Because the AMD "smp_num_siblings" number must be constant. Thanks, Rui