[RFC,7/9] cpufreq: governor: support scheduler cpufreq callbacks on remote CPUs

Message ID 49216ebaad6b26a1d5916350d07654181662b15b.1489058244.git.viresh.kumar@linaro.org
State New
Headers show
Series
  • Untitled series #691
Related show

Commit Message

Viresh Kumar March 9, 2017, 11:45 a.m.
From: Steve Muckle <smuckle.linux@gmail.com>


In preparation for the scheduler cpufreq callback happening on remote
CPUs, add support for this in the legacy (ondemand and conservative)
governors. The legacy governors make assumptions about the callback
occurring on the CPU being updated.

Signed-off-by: Steve Muckle <smuckle.linux@gmail.com>

[ vk: minor updates in commit log ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/cpufreq/cpufreq_governor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.7.1.410.g6faf27b

Comments

Rafael J. Wysocki March 29, 2017, 10:14 p.m. | #1
On Thursday, March 09, 2017 05:15:17 PM Viresh Kumar wrote:
> From: Steve Muckle <smuckle.linux@gmail.com>

> 

> In preparation for the scheduler cpufreq callback happening on remote

> CPUs, add support for this in the legacy (ondemand and conservative)

> governors. The legacy governors make assumptions about the callback

> occurring on the CPU being updated.

> 

> Signed-off-by: Steve Muckle <smuckle.linux@gmail.com>

> [ vk: minor updates in commit log ]

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  drivers/cpufreq/cpufreq_governor.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

> index 47e24b5384b3..c9e786e7ee1f 100644

> --- a/drivers/cpufreq/cpufreq_governor.c

> +++ b/drivers/cpufreq/cpufreq_governor.c

> @@ -315,7 +315,7 @@ static void dbs_update_util_handler(struct update_util_data *data, u64 time,

>  

>  	policy_dbs->last_sample_time = time;

>  	policy_dbs->work_in_progress = true;

> -	irq_work_queue(&policy_dbs->irq_work);

> +	irq_work_queue_on(&policy_dbs->irq_work, data->cpu);


I'm totally unconvinced that this is sufficient.

This function carries out lockless computations with the assumption that it
will always run on the CPU being updated.

For instance, how is it prevented from being run on two CPUs in parallel in
the single-CPU policy case if cross-CPU updates are allowed to happen?

Second, is accessing rq_clock(rq) of a remote runqueue a good idea entirely?

Thanks,
Rafael
Viresh Kumar April 11, 2017, 11:06 a.m. | #2
On 30-03-17, 00:14, Rafael J. Wysocki wrote:
> On Thursday, March 09, 2017 05:15:17 PM Viresh Kumar wrote:

> > From: Steve Muckle <smuckle.linux@gmail.com>

> > 

> > In preparation for the scheduler cpufreq callback happening on remote

> > CPUs, add support for this in the legacy (ondemand and conservative)

> > governors. The legacy governors make assumptions about the callback

> > occurring on the CPU being updated.

> > 

> > Signed-off-by: Steve Muckle <smuckle.linux@gmail.com>

> > [ vk: minor updates in commit log ]

> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> > ---

> >  drivers/cpufreq/cpufreq_governor.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

> > index 47e24b5384b3..c9e786e7ee1f 100644

> > --- a/drivers/cpufreq/cpufreq_governor.c

> > +++ b/drivers/cpufreq/cpufreq_governor.c

> > @@ -315,7 +315,7 @@ static void dbs_update_util_handler(struct update_util_data *data, u64 time,

> >  

> >  	policy_dbs->last_sample_time = time;

> >  	policy_dbs->work_in_progress = true;

> > -	irq_work_queue(&policy_dbs->irq_work);

> > +	irq_work_queue_on(&policy_dbs->irq_work, data->cpu);

> 

> I'm totally unconvinced that this is sufficient.

> 

> This function carries out lockless computations with the assumption that it

> will always run on the CPU being updated.

> 

> For instance, how is it prevented from being run on two CPUs in parallel in

> the single-CPU policy case if cross-CPU updates are allowed to happen?


I am convinced that it is insufficient and yes I too missed the obvious race
here as well for single cpu per policy. Sorry about that.

> Second, is accessing rq_clock(rq) of a remote runqueue a good idea entirely?


I am not sure about how costly that can be.

-- 
viresh

Patch

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 47e24b5384b3..c9e786e7ee1f 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -315,7 +315,7 @@  static void dbs_update_util_handler(struct update_util_data *data, u64 time,
 
 	policy_dbs->last_sample_time = time;
 	policy_dbs->work_in_progress = true;
-	irq_work_queue(&policy_dbs->irq_work);
+	irq_work_queue_on(&policy_dbs->irq_work, data->cpu);
 }
 
 static void gov_set_update_util(struct policy_dbs_info *policy_dbs,