From: Vincent Guittot <vincent.guittot@linaro.org> To: Dietmar Eggemann <dietmar.eggemann@arm.com> Cc: linux-kernel <linux-kernel@vger.kernel.org>, "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, Russell King - ARM Linux <linux@arm.linux.org.uk>, LAK <linux-arm-kernel@lists.infradead.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Russell King <rmk+kernel@armlinux.org.uk>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Juri Lelli <juri.lelli@arm.com>, Peter Zijlstra <peterz@infradead.org>, Morten Rasmussen <morten.rasmussen@arm.com> Subject: Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support Date: Wed, 14 Jun 2017 15:08:48 +0200 [thread overview] Message-ID: <CAKfTPtBnUWyAWTMJ2H8C+SPT+fVj-NkAs_pdOu+_a1zJ=VXZdA@mail.gmail.com> (raw) In-Reply-To: <9716a48d-8198-a1d8-9450-de6386338665@arm.com> On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 06/12/2017 04:27 PM, Vincent Guittot wrote: > > On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com> > 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 <vincent.guittot@linaro.org> > Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> your proposal for freeing cpus_to_visit looks good for me Acked-by: Vincent Guittot <vincent.guittot@linaro.org> > > 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. Ok so IIUC, the transition notifier is somehow optional and we still have the cpu invariance. In this case, you should not return the error code of cpufreq_register_notifier(&set_freq_scale_notifier, CPUFREQ_TRANSITION_NOTIFIER) as the error code of the register_cpufreq_notifier function. you should better print a warning like " failed to init frequency invariance" and return 0 for register_cpufreq_notifier() > > [...]
WARNING: multiple messages have this Message-ID (diff)
From: vincent.guittot@linaro.org (Vincent Guittot) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support Date: Wed, 14 Jun 2017 15:08:48 +0200 [thread overview] Message-ID: <CAKfTPtBnUWyAWTMJ2H8C+SPT+fVj-NkAs_pdOu+_a1zJ=VXZdA@mail.gmail.com> (raw) In-Reply-To: <9716a48d-8198-a1d8-9450-de6386338665@arm.com> On 14 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > On 06/12/2017 04:27 PM, Vincent Guittot wrote: > > On 8 June 2017 at 09:55, Dietmar Eggemann <dietmar.eggemann@arm.com> 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 <dietmar.eggemann@arm.com> > 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 <vincent.guittot@linaro.org> > Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com> your proposal for freeing cpus_to_visit looks good for me Acked-by: Vincent Guittot <vincent.guittot@linaro.org> > > 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. Ok so IIUC, the transition notifier is somehow optional and we still have the cpu invariance. In this case, you should not return the error code of cpufreq_register_notifier(&set_freq_scale_notifier, CPUFREQ_TRANSITION_NOTIFIER) as the error code of the register_cpufreq_notifier function. you should better print a warning like " failed to init frequency invariance" and return 0 for register_cpufreq_notifier() > > [...]
next prev parent reply other threads:[~2017-06-14 13:09 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-08 7:55 [PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for task scheduler Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-08 7:55 ` [PATCH 1/6] drivers base/arch_topology: prepare cpufreq policy notifier for frequency-invariant load-tracking support Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-12 14:45 ` Vincent Guittot 2017-06-12 14:45 ` Vincent Guittot 2017-06-08 7:55 ` [PATCH 2/6] drivers base/arch_topology: " Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-12 14:27 ` Vincent Guittot 2017-06-12 14:27 ` Vincent Guittot 2017-06-14 7:55 ` Dietmar Eggemann 2017-06-14 7:55 ` Dietmar Eggemann 2017-06-14 7:55 ` Dietmar Eggemann 2017-06-14 13:08 ` Vincent Guittot [this message] 2017-06-14 13:08 ` Vincent Guittot 2017-06-15 8:28 ` Juri Lelli 2017-06-15 8:28 ` Juri Lelli 2017-06-21 16:40 ` Dietmar Eggemann 2017-06-21 16:40 ` Dietmar Eggemann 2017-06-20 6:17 ` Viresh Kumar 2017-06-20 6:17 ` Viresh Kumar 2017-06-20 6:17 ` Viresh Kumar 2017-06-21 0:31 ` Saravana Kannan 2017-06-21 0:31 ` Saravana Kannan 2017-06-21 0:31 ` Saravana Kannan 2017-06-21 5:37 ` Viresh Kumar 2017-06-21 5:37 ` Viresh Kumar 2017-06-21 5:37 ` Viresh Kumar 2017-06-21 16:57 ` Morten Rasmussen 2017-06-21 16:57 ` Morten Rasmussen 2017-06-21 16:57 ` Morten Rasmussen 2017-06-22 4:06 ` Viresh Kumar 2017-06-22 4:06 ` Viresh Kumar 2017-06-22 4:06 ` Viresh Kumar 2017-06-22 9:59 ` Morten Rasmussen 2017-06-22 9:59 ` Morten Rasmussen 2017-06-22 9:59 ` Morten Rasmussen 2017-06-21 17:08 ` Dietmar Eggemann 2017-06-21 17:08 ` Dietmar Eggemann 2017-06-21 17:08 ` Dietmar Eggemann 2017-06-21 16:38 ` Dietmar Eggemann 2017-06-21 16:38 ` Dietmar Eggemann 2017-06-21 16:38 ` Dietmar Eggemann 2017-06-22 3:55 ` Viresh Kumar 2017-06-22 3:55 ` Viresh Kumar 2017-06-22 3:55 ` Viresh Kumar 2017-06-26 8:28 ` Dietmar Eggemann 2017-06-26 8:28 ` Dietmar Eggemann 2017-06-08 7:55 ` [PATCH 3/6] arm: wire frequency-invariant accounting support up to the task scheduler Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-12 14:30 ` Vincent Guittot 2017-06-12 14:30 ` Vincent Guittot 2017-06-08 7:55 ` [PATCH 4/6] arm: wire cpu-invariant " Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-12 14:31 ` Vincent Guittot 2017-06-12 14:31 ` Vincent Guittot 2017-06-08 7:55 ` [PATCH 5/6] arm64: wire frequency-invariant " Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-12 13:06 ` Catalin Marinas 2017-06-12 13:06 ` Catalin Marinas 2017-06-12 14:32 ` Vincent Guittot 2017-06-12 14:32 ` Vincent Guittot 2017-06-08 7:55 ` [PATCH 6/6] arm64: wire cpu-invariant " Dietmar Eggemann 2017-06-08 7:55 ` Dietmar Eggemann 2017-06-12 13:07 ` Catalin Marinas 2017-06-12 13:07 ` Catalin Marinas 2017-06-12 14:33 ` Vincent Guittot 2017-06-12 14:33 ` Vincent Guittot 2017-06-12 13:00 ` [PATCH 0/6] arm, arm64: frequency- and cpu-invariant accounting support for " Juri Lelli 2017-06-12 13:00 ` Juri Lelli 2017-06-12 13:04 ` Juri Lelli 2017-06-12 13:04 ` Juri Lelli
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKfTPtBnUWyAWTMJ2H8C+SPT+fVj-NkAs_pdOu+_a1zJ=VXZdA@mail.gmail.com' \ --to=vincent.guittot@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=dietmar.eggemann@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=juri.lelli@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=morten.rasmussen@arm.com \ --cc=peterz@infradead.org \ --cc=rmk+kernel@armlinux.org.uk \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.