From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756962AbcCCIvm (ORCPT ); Thu, 3 Mar 2016 03:51:42 -0500 Received: from www.linutronix.de ([62.245.132.108]:56626 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754148AbcCCIvl (ORCPT ); Thu, 3 Mar 2016 03:51:41 -0500 Date: Thu, 3 Mar 2016 09:50:11 +0100 (CET) From: Thomas Gleixner To: Huang Rui cc: Borislav Petkov , Peter Zijlstra , Ingo Molnar , Andy Lutomirski , Robert Richter , Jacob Shin , Arnaldo Carvalho de Melo , Kan Liang , linux-kernel@vger.kernel.org, spg_linux_kernel@amd.com, x86@kernel.org, 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 In-Reply-To: <1456992284-4808-3-git-send-email-ray.huang@amd.com> Message-ID: References: <1456992284-4808-1-git-send-email-ray.huang@amd.com> <1456992284-4808-3-git-send-email-ray.huang@amd.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 3 Mar 2016, Huang Rui wrote: > +/* > + * The ratio of compute unit power accumulator sample period to the > + * PTSC period. > + */ > +static unsigned int cpu_pwr_sample_ratio; > +static unsigned int cu_num; Why do you need static storage for that information when the only purpose is to printk it in init? > +static void power_cpu_exit(int cpu) > +{ > + int target = nr_cpumask_bits; What's that initialization for? > + > + if (!cpumask_test_and_clear_cpu(cpu, &cpu_mask)) > + return; > + > + /* > + * Find a new CPU on the same compute unit, if was set in cpumask > + * and still some CPUs on compute unit. Then migrate event and > + * context to new CPU. > + */ > + target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu); > + if (target < nr_cpumask_bits) { > + cpumask_set_cpu(target, &cpu_mask); > + perf_pmu_migrate_context(&pmu_class, cpu, target); > + } > +} > + > +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? > +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 .... > +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? > + 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. > + 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? > + } > + > + 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. ..... > + goto out; > + } > + > + pr_info("AMD Power PMU detected, %d compute units\n", cu_num); Why is the number of compute units interesting at all? Thanks, tglx