From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754369AbdFNHzu (ORCPT ); Wed, 14 Jun 2017 03:55:50 -0400 Received: from foss.arm.com ([217.140.101.70]:57514 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750828AbdFNHzt (ORCPT ); Wed, 14 Jun 2017 03:55:49 -0400 Subject: Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support To: Vincent Guittot Cc: linux-kernel , "linux-pm@vger.kernel.org" , Russell King - ARM Linux , LAK , Greg Kroah-Hartman , Russell King , Catalin Marinas , Will Deacon , Juri Lelli , Vincent Guittot , Peter Zijlstra , Morten Rasmussen References: <20170608075513.12475-1-dietmar.eggemann@arm.com> <20170608075513.12475-3-dietmar.eggemann@arm.com> From: Dietmar Eggemann Message-ID: <9716a48d-8198-a1d8-9450-de6386338665@arm.com> Date: Wed, 14 Jun 2017 09:55:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/12/2017 04:27 PM, Vincent Guittot wrote: > On 8 June 2017 at 09:55, Dietmar Eggemann wrote: Hi Vincent, Thanks for the review! [...] >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void) >> >> cpumask_copy(cpus_to_visit, cpu_possible_mask); >> >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier, >> - CPUFREQ_POLICY_NOTIFIER); >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, >> + CPUFREQ_POLICY_NOTIFIER); >> + >> + if (ret) > > Don't you have to free memory allocated for cpus_to_visit in case of > errot ? it was not done before your patch as well Yes, we should free cpus_to_visit if the policy notifier registration fails. But IMHO also, once the parsing of the capacity-dmips-mhz property is done. free cpus_to_visit is only used in the notifier call init_cpu_capacity_callback() after being allocated and initialized in register_cpufreq_notifier(). We could add something like this as the first patch of this set. Only mildly tested on Juno. Juri, what do you think? Author: Dietmar Eggemann Date: Tue Jun 13 23:21:59 2017 +0100 drivers base/arch_topology: free cpumask cpus_to_visit Free cpumask cpus_to_visit in case registering init_cpu_capacity_notifier has failed or the parsing of the cpu capacity-dmips-mhz property is done. The cpumask cpus_to_visit is only used inside the notifier call init_cpu_capacity_callback. Reported-by: Vincent Guittot Signed-off-by: Dietmar Eggemann diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index d1c33a85059e..f4832c662762 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier = { static int __init register_cpufreq_notifier(void) { + int ret; + /* * on ACPI-based systems we need to use the default cpu capacity * until we have the necessary code to parse the cpu capacity, so @@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void) cpumask_copy(cpus_to_visit, cpu_possible_mask); - return cpufreq_register_notifier(&init_cpu_capacity_notifier, - CPUFREQ_POLICY_NOTIFIER); + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, + CPUFREQ_POLICY_NOTIFIER); + + if (ret) + free_cpumask_var(cpus_to_visit); + + return ret; } core_initcall(register_cpufreq_notifier); static void parsing_done_workfn(struct work_struct *work) { + free_cpumask_var(cpus_to_visit); cpufreq_unregister_notifier(&init_cpu_capacity_notifier, CPUFREQ_POLICY_NOTIFIER); } >> + return ret; >> + >> + return cpufreq_register_notifier(&set_freq_scale_notifier, >> + CPUFREQ_TRANSITION_NOTIFIER); > > Don't you have to unregister the other cpufreq notifier if an error is > returned and free the mem allocated for cpus_to_visit ? IMHO, that's not necessary. The transition notifier works completely independent from the policy notifier. In case the latter gets registered correctly and the registration of the former fails, the notifier call of the policy notifier still parses the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu). The notifier call set_freq_scale_callback() of the transition notifier will not be called so that frequency invariance always returns SCHED_CAPACITY_SCALE. After the policy notifier has finished its work, it schedules parsing_done_work() in which it gets unregistered. [...] From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dietmar Eggemann Subject: Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support Date: Wed, 14 Jun 2017 09:55:44 +0200 Message-ID: <9716a48d-8198-a1d8-9450-de6386338665@arm.com> References: <20170608075513.12475-1-dietmar.eggemann@arm.com> <20170608075513.12475-3-dietmar.eggemann@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org Cc: linux-kernel , "linux-pm@vger.kernel.org" , Russell King - ARM Linux , LAK , Greg Kroah-Hartman , Russell King , Catalin Marinas , Will Deacon , Juri Lelli , Vincent Guittot , Peter Zijlstra , Morten Rasmussen List-Id: linux-pm@vger.kernel.org On 06/12/2017 04:27 PM, Vincent Guittot wrote: > On 8 June 2017 at 09:55, Dietmar Eggemann wrote: Hi Vincent, Thanks for the review! [...] >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void) >> >> cpumask_copy(cpus_to_visit, cpu_possible_mask); >> >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier, >> - CPUFREQ_POLICY_NOTIFIER); >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, >> + CPUFREQ_POLICY_NOTIFIER); >> + >> + if (ret) > > Don't you have to free memory allocated for cpus_to_visit in case of > errot ? it was not done before your patch as well Yes, we should free cpus_to_visit if the policy notifier registration fails. But IMHO also, once the parsing of the capacity-dmips-mhz property is done. free cpus_to_visit is only used in the notifier call init_cpu_capacity_callback() after being allocated and initialized in register_cpufreq_notifier(). We could add something like this as the first patch of this set. Only mildly tested on Juno. Juri, what do you think? Author: Dietmar Eggemann Date: Tue Jun 13 23:21:59 2017 +0100 drivers base/arch_topology: free cpumask cpus_to_visit Free cpumask cpus_to_visit in case registering init_cpu_capacity_notifier has failed or the parsing of the cpu capacity-dmips-mhz property is done. The cpumask cpus_to_visit is only used inside the notifier call init_cpu_capacity_callback. Reported-by: Vincent Guittot Signed-off-by: Dietmar Eggemann diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index d1c33a85059e..f4832c662762 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier = { static int __init register_cpufreq_notifier(void) { + int ret; + /* * on ACPI-based systems we need to use the default cpu capacity * until we have the necessary code to parse the cpu capacity, so @@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void) cpumask_copy(cpus_to_visit, cpu_possible_mask); - return cpufreq_register_notifier(&init_cpu_capacity_notifier, - CPUFREQ_POLICY_NOTIFIER); + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, + CPUFREQ_POLICY_NOTIFIER); + + if (ret) + free_cpumask_var(cpus_to_visit); + + return ret; } core_initcall(register_cpufreq_notifier); static void parsing_done_workfn(struct work_struct *work) { + free_cpumask_var(cpus_to_visit); cpufreq_unregister_notifier(&init_cpu_capacity_notifier, CPUFREQ_POLICY_NOTIFIER); } >> + return ret; >> + >> + return cpufreq_register_notifier(&set_freq_scale_notifier, >> + CPUFREQ_TRANSITION_NOTIFIER); > > Don't you have to unregister the other cpufreq notifier if an error is > returned and free the mem allocated for cpus_to_visit ? IMHO, that's not necessary. The transition notifier works completely independent from the policy notifier. In case the latter gets registered correctly and the registration of the former fails, the notifier call of the policy notifier still parses the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu). The notifier call set_freq_scale_callback() of the transition notifier will not be called so that frequency invariance always returns SCHED_CAPACITY_SCALE. After the policy notifier has finished its work, it schedules parsing_done_work() in which it gets unregistered. [...] From mboxrd@z Thu Jan 1 00:00:00 1970 From: dietmar.eggemann@arm.com (Dietmar Eggemann) Date: Wed, 14 Jun 2017 09:55:44 +0200 Subject: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support In-Reply-To: References: <20170608075513.12475-1-dietmar.eggemann@arm.com> <20170608075513.12475-3-dietmar.eggemann@arm.com> Message-ID: <9716a48d-8198-a1d8-9450-de6386338665@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 06/12/2017 04:27 PM, Vincent Guittot wrote: > On 8 June 2017 at 09:55, Dietmar Eggemann wrote: Hi Vincent, Thanks for the review! [...] >> @@ -225,8 +265,14 @@ static int __init register_cpufreq_notifier(void) >> >> cpumask_copy(cpus_to_visit, cpu_possible_mask); >> >> - return cpufreq_register_notifier(&init_cpu_capacity_notifier, >> - CPUFREQ_POLICY_NOTIFIER); >> + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, >> + CPUFREQ_POLICY_NOTIFIER); >> + >> + if (ret) > > Don't you have to free memory allocated for cpus_to_visit in case of > errot ? it was not done before your patch as well Yes, we should free cpus_to_visit if the policy notifier registration fails. But IMHO also, once the parsing of the capacity-dmips-mhz property is done. free cpus_to_visit is only used in the notifier call init_cpu_capacity_callback() after being allocated and initialized in register_cpufreq_notifier(). We could add something like this as the first patch of this set. Only mildly tested on Juno. Juri, what do you think? Author: Dietmar Eggemann Date: Tue Jun 13 23:21:59 2017 +0100 drivers base/arch_topology: free cpumask cpus_to_visit Free cpumask cpus_to_visit in case registering init_cpu_capacity_notifier has failed or the parsing of the cpu capacity-dmips-mhz property is done. The cpumask cpus_to_visit is only used inside the notifier call init_cpu_capacity_callback. Reported-by: Vincent Guittot Signed-off-by: Dietmar Eggemann diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index d1c33a85059e..f4832c662762 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -206,6 +206,8 @@ static struct notifier_block init_cpu_capacity_notifier = { static int __init register_cpufreq_notifier(void) { + int ret; + /* * on ACPI-based systems we need to use the default cpu capacity * until we have the necessary code to parse the cpu capacity, so @@ -221,13 +223,19 @@ static int __init register_cpufreq_notifier(void) cpumask_copy(cpus_to_visit, cpu_possible_mask); - return cpufreq_register_notifier(&init_cpu_capacity_notifier, - CPUFREQ_POLICY_NOTIFIER); + ret = cpufreq_register_notifier(&init_cpu_capacity_notifier, + CPUFREQ_POLICY_NOTIFIER); + + if (ret) + free_cpumask_var(cpus_to_visit); + + return ret; } core_initcall(register_cpufreq_notifier); static void parsing_done_workfn(struct work_struct *work) { + free_cpumask_var(cpus_to_visit); cpufreq_unregister_notifier(&init_cpu_capacity_notifier, CPUFREQ_POLICY_NOTIFIER); } >> + return ret; >> + >> + return cpufreq_register_notifier(&set_freq_scale_notifier, >> + CPUFREQ_TRANSITION_NOTIFIER); > > Don't you have to unregister the other cpufreq notifier if an error is > returned and free the mem allocated for cpus_to_visit ? IMHO, that's not necessary. The transition notifier works completely independent from the policy notifier. In case the latter gets registered correctly and the registration of the former fails, the notifier call of the policy notifier still parses the capacity-dmips-mhz property information and sets per_cpu(max_freq, cpu). The notifier call set_freq_scale_callback() of the transition notifier will not be called so that frequency invariance always returns SCHED_CAPACITY_SCALE. After the policy notifier has finished its work, it schedules parsing_done_work() in which it gets unregistered. [...]