From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753107AbdFUFhl (ORCPT ); Wed, 21 Jun 2017 01:37:41 -0400 Received: from mail-pg0-f50.google.com ([74.125.83.50]:33338 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752582AbdFUFhj (ORCPT ); Wed, 21 Jun 2017 01:37:39 -0400 Date: Wed, 21 Jun 2017 11:07:35 +0530 From: Viresh Kumar To: Saravana Kannan Cc: Dietmar Eggemann , "linux-kernel@vger.kernel.org" , Linux PM list , Russell King , "linux-arm-kernel@lists.infradead.org" , Greg Kroah-Hartman , Russell King , Catalin Marinas , Will Deacon , Juri Lelli , Vincent Guittot , Peter Zijlstra , Morten Rasmussen Subject: Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support Message-ID: <20170621053735.GR3942@vireshk-i7> References: <20170608075513.12475-1-dietmar.eggemann@arm.com> <20170608075513.12475-3-dietmar.eggemann@arm.com> <5949BE5F.4020502@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5949BE5F.4020502@codeaurora.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20-06-17, 17:31, Saravana Kannan wrote: > On 06/19/2017 11:17 PM, Viresh Kumar wrote: > >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann > > wrote: > > > >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > > > >> 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 > >>@@ -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); > > > >Wanted to make sure that we all understand the constraints this is going to add > >for the ARM64 platforms. > > > >With the introduction of this transition notifier, we would not be able to use > >the fast-switch path in the schedutil governor. I am not sure if there are any > >ARM platforms that can actually use the fast-switch path in future or not > >though. The requirement of fast-switch path is that the freq can be changed > >without sleeping in the hot-path. > > > >So, will we ever want fast-switching for ARM platforms ? > > > > I don't think we should go down a path that'll prevent ARM platform from > switching over to fast-switching in the future. Yeah, that's why brought attention to this stuff. I think this patch doesn't really need to go down the notifiers way. We can do something like this in the implementation of topology_get_freq_scale(): return (policy->cur << SCHED_CAPACITY_SHIFT) / max; Though, we would be required to take care of policy structure in this case somehow. > Having said that, I'm not sure I fully agree with the decision to completely > disable notifiers in the fast-switching case. How many of the current users > of notifiers truly need support for sleeping in the notifier? Its not just about sleeping here. We do not wish to call too much stuff from scheduler hot path. Even if it doesn't sleep. > Why not make > all the transition notifiers atomic? Or at least add atomic transition > notifiers that can be registered for separately if the client doesn't need > the ability to sleep? > > Most of the clients don't seem like ones that'll need to sleep. Only if the scheduler maintainers agree to getting these notifiers called from hot path, which I don't think is going to happen. > There are a bunch of generic off-tree drivers (can't upstream them yet > because it depends on the bus scaling framework) that also depend on CPUfreq > transition notifiers that are going to stop working if fast switching > becomes available in the future. So, this decision to disallow transition > notifiers is painful for other reasons too. I think its kind of fine to work without fast switch in those cases, as we are anyway ready to call notifiers which may end up taking any amount of time. This case was special as it is affecting entire arch here and so I pointed it out. -- viresh From mboxrd@z Thu Jan 1 00:00:00 1970 From: Viresh Kumar Subject: Re: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support Date: Wed, 21 Jun 2017 11:07:35 +0530 Message-ID: <20170621053735.GR3942@vireshk-i7> References: <20170608075513.12475-1-dietmar.eggemann@arm.com> <20170608075513.12475-3-dietmar.eggemann@arm.com> <5949BE5F.4020502@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pg0-f50.google.com ([74.125.83.50]:36224 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751069AbdFUFhj (ORCPT ); Wed, 21 Jun 2017 01:37:39 -0400 Received: by mail-pg0-f50.google.com with SMTP id u62so53408027pgb.3 for ; Tue, 20 Jun 2017 22:37:39 -0700 (PDT) Content-Disposition: inline In-Reply-To: <5949BE5F.4020502@codeaurora.org> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Saravana Kannan Cc: Dietmar Eggemann , "linux-kernel@vger.kernel.org" , Linux PM list , Russell King , "linux-arm-kernel@lists.infradead.org" , Greg Kroah-Hartman , Russell King , Catalin Marinas , Will Deacon , Juri Lelli , Vincent Guittot , Peter Zijlstra , Morten Rasmussen On 20-06-17, 17:31, Saravana Kannan wrote: > On 06/19/2017 11:17 PM, Viresh Kumar wrote: > >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann > > wrote: > > > >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > > > >> 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 > >>@@ -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); > > > >Wanted to make sure that we all understand the constraints this is going to add > >for the ARM64 platforms. > > > >With the introduction of this transition notifier, we would not be able to use > >the fast-switch path in the schedutil governor. I am not sure if there are any > >ARM platforms that can actually use the fast-switch path in future or not > >though. The requirement of fast-switch path is that the freq can be changed > >without sleeping in the hot-path. > > > >So, will we ever want fast-switching for ARM platforms ? > > > > I don't think we should go down a path that'll prevent ARM platform from > switching over to fast-switching in the future. Yeah, that's why brought attention to this stuff. I think this patch doesn't really need to go down the notifiers way. We can do something like this in the implementation of topology_get_freq_scale(): return (policy->cur << SCHED_CAPACITY_SHIFT) / max; Though, we would be required to take care of policy structure in this case somehow. > Having said that, I'm not sure I fully agree with the decision to completely > disable notifiers in the fast-switching case. How many of the current users > of notifiers truly need support for sleeping in the notifier? Its not just about sleeping here. We do not wish to call too much stuff from scheduler hot path. Even if it doesn't sleep. > Why not make > all the transition notifiers atomic? Or at least add atomic transition > notifiers that can be registered for separately if the client doesn't need > the ability to sleep? > > Most of the clients don't seem like ones that'll need to sleep. Only if the scheduler maintainers agree to getting these notifiers called from hot path, which I don't think is going to happen. > There are a bunch of generic off-tree drivers (can't upstream them yet > because it depends on the bus scaling framework) that also depend on CPUfreq > transition notifiers that are going to stop working if fast switching > becomes available in the future. So, this decision to disallow transition > notifiers is painful for other reasons too. I think its kind of fine to work without fast switch in those cases, as we are anyway ready to call notifiers which may end up taking any amount of time. This case was special as it is affecting entire arch here and so I pointed it out. -- viresh From mboxrd@z Thu Jan 1 00:00:00 1970 From: viresh.kumar@linaro.org (Viresh Kumar) Date: Wed, 21 Jun 2017 11:07:35 +0530 Subject: [PATCH 2/6] drivers base/arch_topology: frequency-invariant load-tracking support In-Reply-To: <5949BE5F.4020502@codeaurora.org> References: <20170608075513.12475-1-dietmar.eggemann@arm.com> <20170608075513.12475-3-dietmar.eggemann@arm.com> <5949BE5F.4020502@codeaurora.org> Message-ID: <20170621053735.GR3942@vireshk-i7> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 20-06-17, 17:31, Saravana Kannan wrote: > On 06/19/2017 11:17 PM, Viresh Kumar wrote: > >On Thu, Jun 8, 2017 at 1:25 PM, Dietmar Eggemann > > wrote: > > > >>diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > > > >> 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 > >>@@ -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); > > > >Wanted to make sure that we all understand the constraints this is going to add > >for the ARM64 platforms. > > > >With the introduction of this transition notifier, we would not be able to use > >the fast-switch path in the schedutil governor. I am not sure if there are any > >ARM platforms that can actually use the fast-switch path in future or not > >though. The requirement of fast-switch path is that the freq can be changed > >without sleeping in the hot-path. > > > >So, will we ever want fast-switching for ARM platforms ? > > > > I don't think we should go down a path that'll prevent ARM platform from > switching over to fast-switching in the future. Yeah, that's why brought attention to this stuff. I think this patch doesn't really need to go down the notifiers way. We can do something like this in the implementation of topology_get_freq_scale(): return (policy->cur << SCHED_CAPACITY_SHIFT) / max; Though, we would be required to take care of policy structure in this case somehow. > Having said that, I'm not sure I fully agree with the decision to completely > disable notifiers in the fast-switching case. How many of the current users > of notifiers truly need support for sleeping in the notifier? Its not just about sleeping here. We do not wish to call too much stuff from scheduler hot path. Even if it doesn't sleep. > Why not make > all the transition notifiers atomic? Or at least add atomic transition > notifiers that can be registered for separately if the client doesn't need > the ability to sleep? > > Most of the clients don't seem like ones that'll need to sleep. Only if the scheduler maintainers agree to getting these notifiers called from hot path, which I don't think is going to happen. > There are a bunch of generic off-tree drivers (can't upstream them yet > because it depends on the bus scaling framework) that also depend on CPUfreq > transition notifiers that are going to stop working if fast switching > becomes available in the future. So, this decision to disallow transition > notifiers is painful for other reasons too. I think its kind of fine to work without fast switch in those cases, as we are anyway ready to call notifiers which may end up taking any amount of time. This case was special as it is affecting entire arch here and so I pointed it out. -- viresh