Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Viresh Kumar]

On 28-07-17, 14:05, Saravana Kannan wrote:
> 1. I'm not saying that. I'm saying assuming CPUs can change the freq only on
> behalf of all the CPUs in the same policy is wrong. Again, the scheduler or
> governor shouldn't even be making any of that assumption. That's a CPUfreq
> driver problem.
> 
> 2. No, that is not the basis of the entire cpufreq core design. None of the
> existing CPUfreq code has any assumptions that only CPUs in a policy can
> change their frequency. It doesn't break in any way in system where any CPU
> can change any other CPU's frequency -- all Qualcomm chips are like that.
> It's only the recent scheduler notifier changes that are adding this
> additional limitation and breaking stuff for systems where any CPU can
> change any other CPU's frequency.

Can you please have a look at V5 and see f the solution proposed there would be
fine ?

-- 
viresh

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Viresh Kumar]

On 28-07-17, 14:05, Saravana Kannan wrote:
> 1. I'm not saying that. I'm saying assuming CPUs can change the freq only on
> behalf of all the CPUs in the same policy is wrong. Again, the scheduler or
> governor shouldn't even be making any of that assumption. That's a CPUfreq
> driver problem.
> 
> 2. No, that is not the basis of the entire cpufreq core design. None of the
> existing CPUfreq code has any assumptions that only CPUs in a policy can
> change their frequency. It doesn't break in any way in system where any CPU
> can change any other CPU's frequency -- all Qualcomm chips are like that.
> It's only the recent scheduler notifier changes that are adding this
> additional limitation and breaking stuff for systems where any CPU can
> change any other CPU's frequency.

Can you please have a look at V5 and see f the solution proposed there would be
fine ?

-- 
viresh

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Saravana Kannan]

On 07/27/2017 11:00 PM, Viresh Kumar wrote:

On 27-07-17, 12:55, Saravana Kannan wrote:

Yes. Simplifying isn't always about number of lines of code. It's also about
abstraction. Having generic scheduler code care about HW details doesn't
seem nice.


I can argue that even the policy->cpus field is also hardware
specific, isn't it ?


Yes.

And we are using that in the schedutil governor
anyway.

Yes


What's wrong with having another field (in a generic way) in
the same structure that tells us more about hardware ?


Nothing wrong. I'm not saying you shouldn't have the cpu field in the 
data or as a parameter to the hook. You'll definitely need that.



And then schedutil isn't really scheduler, but a cpufreq governor.
Just like ondemand/conservative, which are also called from the same
scheduler path.


Exactly. I never debated anything about schedutil. I'm just saying don't 
have any CPU limitations or check on the scheduler side when sending 
notification. Scheduler shouldn't have to know/care of the driver can 
only set the freq on that CPU or across CPUs in a cluster or across the 
entire system.



It'll literally one simple check (cpu == smp_processor_id()) or (cpu "in"
policy->cpus).

Also, this is only for drivers that currently support fast switching. How
many of those do you have?


Why? Why shouldn't we do that for the other drivers? I think it should
be done across everyone.


Because if I remember it right, the "don't send the notification if it's 
not the same CPU" limitation is only for the fast switching case?  I 
might be mistaken about this part though.



The core already has most of the data required and I believe that we
need to handle it in the governor's code as is handled in this series.


Clearly, it doesn't. You are just making assumptions about HW.


So assuming that any CPU from a policy can change freq on behalf of
all the CPUs of the same policy is wrong? That is the basis of how the
cpufreq core is designed.


1. I'm not saying that. I'm saying assuming CPUs can change the freq 
only on behalf of all the CPUs in the same policy is wrong. Again, the 
scheduler or governor shouldn't even be making any of that assumption. 
That's a CPUfreq driver problem.


2. No, that is not the basis of the entire cpufreq core design. None of 
the existing CPUfreq code has any assumptions that only CPUs in a policy 
can change their frequency. It doesn't break in any way in system where 
any CPU can change any other CPU's frequency -- all Qualcomm chips are 
like that. It's only the recent scheduler notifier changes that are 
adding this additional limitation and breaking stuff for systems where 
any CPU can change any other CPU's frequency.


-Saravana

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Saravana Kannan]

On 07/27/2017 11:00 PM, Viresh Kumar wrote:

On 27-07-17, 12:55, Saravana Kannan wrote:

Yes. Simplifying isn't always about number of lines of code. It's also about
abstraction. Having generic scheduler code care about HW details doesn't
seem nice.


I can argue that even the policy->cpus field is also hardware
specific, isn't it ?


Yes.

And we are using that in the schedutil governor
anyway.

Yes


What's wrong with having another field (in a generic way) in
the same structure that tells us more about hardware ?


Nothing wrong. I'm not saying you shouldn't have the cpu field in the 
data or as a parameter to the hook. You'll definitely need that.



And then schedutil isn't really scheduler, but a cpufreq governor.
Just like ondemand/conservative, which are also called from the same
scheduler path.


Exactly. I never debated anything about schedutil. I'm just saying don't 
have any CPU limitations or check on the scheduler side when sending 
notification. Scheduler shouldn't have to know/care of the driver can 
only set the freq on that CPU or across CPUs in a cluster or across the 
entire system.



It'll literally one simple check (cpu == smp_processor_id()) or (cpu "in"
policy->cpus).

Also, this is only for drivers that currently support fast switching. How
many of those do you have?


Why? Why shouldn't we do that for the other drivers? I think it should
be done across everyone.


Because if I remember it right, the "don't send the notification if it's 
not the same CPU" limitation is only for the fast switching case?  I 
might be mistaken about this part though.



The core already has most of the data required and I believe that we
need to handle it in the governor's code as is handled in this series.


Clearly, it doesn't. You are just making assumptions about HW.


So assuming that any CPU from a policy can change freq on behalf of
all the CPUs of the same policy is wrong? That is the basis of how the
cpufreq core is designed.


1. I'm not saying that. I'm saying assuming CPUs can change the freq 
only on behalf of all the CPUs in the same policy is wrong. Again, the 
scheduler or governor shouldn't even be making any of that assumption. 
That's a CPUfreq driver problem.


2. No, that is not the basis of the entire cpufreq core design. None of 
the existing CPUfreq code has any assumptions that only CPUs in a policy 
can change their frequency. It doesn't break in any way in system where 
any CPU can change any other CPU's frequency -- all Qualcomm chips are 
like that. It's only the recent scheduler notifier changes that are 
adding this additional limitation and breaking stuff for systems where 
any CPU can change any other CPU's frequency.


-Saravana

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Viresh Kumar]

On 27-07-17, 12:55, Saravana Kannan wrote:
> Yes. Simplifying isn't always about number of lines of code. It's also about
> abstraction. Having generic scheduler code care about HW details doesn't
> seem nice.

I can argue that even the policy->cpus field is also hardware
specific, isn't it ? And we are using that in the schedutil governor
anyway. What's wrong with having another field (in a generic way) in
the same structure that tells us more about hardware ?

And then schedutil isn't really scheduler, but a cpufreq governor.
Just like ondemand/conservative, which are also called from the same
scheduler path.

> It'll literally one simple check (cpu == smp_processor_id()) or (cpu "in"
> policy->cpus).
> 
> Also, this is only for drivers that currently support fast switching. How
> many of those do you have?

Why? Why shouldn't we do that for the other drivers? I think it should
be done across everyone.

> >The core already has most of the data required and I believe that we
> >need to handle it in the governor's code as is handled in this series.
> 
> Clearly, it doesn't. You are just making assumptions about HW.

So assuming that any CPU from a policy can change freq on behalf of
all the CPUs of the same policy is wrong? That is the basis of how the
cpufreq core is designed.

-- 
viresh

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Viresh Kumar]

On 27-07-17, 12:55, Saravana Kannan wrote:
> Yes. Simplifying isn't always about number of lines of code. It's also about
> abstraction. Having generic scheduler code care about HW details doesn't
> seem nice.

I can argue that even the policy->cpus field is also hardware
specific, isn't it ? And we are using that in the schedutil governor
anyway. What's wrong with having another field (in a generic way) in
the same structure that tells us more about hardware ?

And then schedutil isn't really scheduler, but a cpufreq governor.
Just like ondemand/conservative, which are also called from the same
scheduler path.

> It'll literally one simple check (cpu == smp_processor_id()) or (cpu "in"
> policy->cpus).
> 
> Also, this is only for drivers that currently support fast switching. How
> many of those do you have?

Why? Why shouldn't we do that for the other drivers? I think it should
be done across everyone.

> >The core already has most of the data required and I believe that we
> >need to handle it in the governor's code as is handled in this series.
> 
> Clearly, it doesn't. You are just making assumptions about HW.

So assuming that any CPU from a policy can change freq on behalf of
all the CPUs of the same policy is wrong? That is the basis of how the
cpufreq core is designed.

-- 
viresh

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Joel Fernandes (Google)]

On Thu, Jul 27, 2017 at 12:55 PM, Saravana Kannan
 wrote:
> On 07/26/2017 08:30 PM, Viresh Kumar wrote:
>>
>> On 26-07-17, 14:00, Saravana Kannan wrote:
>>>
>>> No, the alternative is to pass it on to the CPU freq driver and let it
>>> decide what it wants to do. That's the whole point if having a CPU freq
>>> driver -- so that the generic code doesn't need to care about HW specific
>>> details. Which is the point I was making in an earlier email to Viresh's
>>> patch -- we shouldn't be doing any CPU check for the call backs at the
>>> scheduler or ever governor level.
>>>
>>> That would simplify this whole thing by deleting a bunch of code. And
>>> having
>>> much simpler checks in those drivers that actually have to deal with
>>> their
>>> HW specific details.
>>
>>
>> So what you are saying is that we go and update (almost) every cpufreq
>> driver we have today and make their ->target() callbacks return early
>> if they don't support switching frequency remotely ? Is that really
>> simplifying anything?
>
>
> Yes. Simplifying isn't always about number of lines of code. It's also about
> abstraction. Having generic scheduler code care about HW details doesn't
> seem nice.
>
> It'll literally one simple check (cpu == smp_processor_id()) or (cpu "in"
> policy->cpus).
>

I think we can have both approaches? So we query the driver some time
around sugov_should_update_freq (with a new driver callback?) and ask
it if it has any say over the default behavior of "can't update remote
CPU if I'm not a part of its policy" and use that over the default if
it hasn't defined it in their struct cpufreq_driver.

I think this will also not have the concern of "updating every
driver", then we can just stick to the sane default of "no" for
drivers that haven't defined it. Probably Viresh has already thought
about this, but I just thought of bringing it up anyway. I also think
its fine to handle this case after this series gets in, but that's
just my opinion.

thanks!

-Joel

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Joel Fernandes (Google)]

On Thu, Jul 27, 2017 at 12:55 PM, Saravana Kannan
 wrote:
> On 07/26/2017 08:30 PM, Viresh Kumar wrote:
>>
>> On 26-07-17, 14:00, Saravana Kannan wrote:
>>>
>>> No, the alternative is to pass it on to the CPU freq driver and let it
>>> decide what it wants to do. That's the whole point if having a CPU freq
>>> driver -- so that the generic code doesn't need to care about HW specific
>>> details. Which is the point I was making in an earlier email to Viresh's
>>> patch -- we shouldn't be doing any CPU check for the call backs at the
>>> scheduler or ever governor level.
>>>
>>> That would simplify this whole thing by deleting a bunch of code. And
>>> having
>>> much simpler checks in those drivers that actually have to deal with
>>> their
>>> HW specific details.
>>
>>
>> So what you are saying is that we go and update (almost) every cpufreq
>> driver we have today and make their ->target() callbacks return early
>> if they don't support switching frequency remotely ? Is that really
>> simplifying anything?
>
>
> Yes. Simplifying isn't always about number of lines of code. It's also about
> abstraction. Having generic scheduler code care about HW details doesn't
> seem nice.
>
> It'll literally one simple check (cpu == smp_processor_id()) or (cpu "in"
> policy->cpus).
>

I think we can have both approaches? So we query the driver some time
around sugov_should_update_freq (with a new driver callback?) and ask
it if it has any say over the default behavior of "can't update remote
CPU if I'm not a part of its policy" and use that over the default if
it hasn't defined it in their struct cpufreq_driver.

I think this will also not have the concern of "updating every
driver", then we can just stick to the sane default of "no" for
drivers that haven't defined it. Probably Viresh has already thought
about this, but I just thought of bringing it up anyway. I also think
its fine to handle this case after this series gets in, but that's
just my opinion.

thanks!

-Joel

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Saravana Kannan]

On 07/26/2017 08:30 PM, Viresh Kumar wrote:

On 26-07-17, 14:00, Saravana Kannan wrote:

No, the alternative is to pass it on to the CPU freq driver and let it
decide what it wants to do. That's the whole point if having a CPU freq
driver -- so that the generic code doesn't need to care about HW specific
details. Which is the point I was making in an earlier email to Viresh's
patch -- we shouldn't be doing any CPU check for the call backs at the
scheduler or ever governor level.

That would simplify this whole thing by deleting a bunch of code. And having
much simpler checks in those drivers that actually have to deal with their
HW specific details.


So what you are saying is that we go and update (almost) every cpufreq
driver we have today and make their ->target() callbacks return early
if they don't support switching frequency remotely ? Is that really
simplifying anything?


Yes. Simplifying isn't always about number of lines of code. It's also 
about abstraction. Having generic scheduler code care about HW details 
doesn't seem nice.


It'll literally one simple check (cpu == smp_processor_id()) or (cpu 
"in" policy->cpus).


Also, this is only for drivers that currently support fast switching. 
How many of those do you have?



The core already has most of the data required and I believe that we
need to handle it in the governor's code as is handled in this series.


Clearly, it doesn't. You are just making assumptions about HW.


To solve the problem that you have been reporting (update from any
CPU), we need something like this which I earlier suggested and I
will come back to it after this series is gone. Don't want to
complicate things here unnecessarily.

https://marc.info/?l=linux-kernel=148906012827786=2


I'm okay with handling it later. I'm just saying that if we are going to 
go back and debate the CPU check, then maybe it's better do it in one 
series.


-Saravana

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Saravana Kannan]

On 07/26/2017 08:30 PM, Viresh Kumar wrote:

On 26-07-17, 14:00, Saravana Kannan wrote:

No, the alternative is to pass it on to the CPU freq driver and let it
decide what it wants to do. That's the whole point if having a CPU freq
driver -- so that the generic code doesn't need to care about HW specific
details. Which is the point I was making in an earlier email to Viresh's
patch -- we shouldn't be doing any CPU check for the call backs at the
scheduler or ever governor level.

That would simplify this whole thing by deleting a bunch of code. And having
much simpler checks in those drivers that actually have to deal with their
HW specific details.


So what you are saying is that we go and update (almost) every cpufreq
driver we have today and make their ->target() callbacks return early
if they don't support switching frequency remotely ? Is that really
simplifying anything?


Yes. Simplifying isn't always about number of lines of code. It's also 
about abstraction. Having generic scheduler code care about HW details 
doesn't seem nice.


It'll literally one simple check (cpu == smp_processor_id()) or (cpu 
"in" policy->cpus).


Also, this is only for drivers that currently support fast switching. 
How many of those do you have?



The core already has most of the data required and I believe that we
need to handle it in the governor's code as is handled in this series.


Clearly, it doesn't. You are just making assumptions about HW.


To solve the problem that you have been reporting (update from any
CPU), we need something like this which I earlier suggested and I
will come back to it after this series is gone. Don't want to
complicate things here unnecessarily.

https://marc.info/?l=linux-kernel=148906012827786=2


I'm okay with handling it later. I'm just saying that if we are going to 
go back and debate the CPU check, then maybe it's better do it in one 
series.


-Saravana

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Viresh Kumar]

On 26-07-17, 14:00, Saravana Kannan wrote:
> No, the alternative is to pass it on to the CPU freq driver and let it
> decide what it wants to do. That's the whole point if having a CPU freq
> driver -- so that the generic code doesn't need to care about HW specific
> details. Which is the point I was making in an earlier email to Viresh's
> patch -- we shouldn't be doing any CPU check for the call backs at the
> scheduler or ever governor level.
> 
> That would simplify this whole thing by deleting a bunch of code. And having
> much simpler checks in those drivers that actually have to deal with their
> HW specific details.

So what you are saying is that we go and update (almost) every cpufreq
driver we have today and make their ->target() callbacks return early
if they don't support switching frequency remotely ? Is that really
simplifying anything?

The core already has most of the data required and I believe that we
need to handle it in the governor's code as is handled in this series.

To solve the problem that you have been reporting (update from any
CPU), we need something like this which I earlier suggested and I
will come back to it after this series is gone. Don't want to
complicate things here unnecessarily.

https://marc.info/?l=linux-kernel=148906012827786=2

-- 
viresh

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Viresh Kumar]

On 26-07-17, 14:00, Saravana Kannan wrote:
> No, the alternative is to pass it on to the CPU freq driver and let it
> decide what it wants to do. That's the whole point if having a CPU freq
> driver -- so that the generic code doesn't need to care about HW specific
> details. Which is the point I was making in an earlier email to Viresh's
> patch -- we shouldn't be doing any CPU check for the call backs at the
> scheduler or ever governor level.
> 
> That would simplify this whole thing by deleting a bunch of code. And having
> much simpler checks in those drivers that actually have to deal with their
> HW specific details.

So what you are saying is that we go and update (almost) every cpufreq
driver we have today and make their ->target() callbacks return early
if they don't support switching frequency remotely ? Is that really
simplifying anything?

The core already has most of the data required and I believe that we
need to handle it in the governor's code as is handled in this series.

To solve the problem that you have been reporting (update from any
CPU), we need something like this which I earlier suggested and I
will come back to it after this series is gone. Don't want to
complicate things here unnecessarily.

https://marc.info/?l=linux-kernel=148906012827786=2

-- 
viresh

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Saravana Kannan]

On 07/21/2017 06:03 AM, Peter Zijlstra wrote:

On Thu, Jul 13, 2017 at 12:14:37PM +0530, Viresh Kumar wrote:

diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index 47e24b5384b3..606b1a37a1af 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -275,6 +275,10 @@ static void dbs_update_util_handler(struct 
update_util_data *data, u64 time,
struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
u64 delta_ns, lst;

+   /* Don't allow remote callbacks */
+   if (smp_processor_id() != data->cpu)
+   return;
+


The alternative is using some of that policy_dbs->policy->*cpus crud I
suppose, because:


No, the alternative is to pass it on to the CPU freq driver and let it 
decide what it wants to do. That's the whole point if having a CPU freq 
driver -- so that the generic code doesn't need to care about HW 
specific details. Which is the point I was making in an earlier email to 
Viresh's patch -- we shouldn't be doing any CPU check for the call backs 
at the scheduler or ever governor level.


That would simplify this whole thing by deleting a bunch of code. And 
having much simpler checks in those drivers that actually have to deal 
with their HW specific details.



--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Eas-dev] [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks

[Saravana Kannan]

On 07/21/2017 06:03 AM, Peter Zijlstra wrote:

On Thu, Jul 13, 2017 at 12:14:37PM +0530, Viresh Kumar wrote:

diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index 47e24b5384b3..606b1a37a1af 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -275,6 +275,10 @@ static void dbs_update_util_handler(struct 
update_util_data *data, u64 time,
struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
u64 delta_ns, lst;

+   /* Don't allow remote callbacks */
+   if (smp_processor_id() != data->cpu)
+   return;
+


The alternative is using some of that policy_dbs->policy->*cpus crud I
suppose, because:


No, the alternative is to pass it on to the CPU freq driver and let it 
decide what it wants to do. That's the whole point if having a CPU freq 
driver -- so that the generic code doesn't need to care about HW 
specific details. Which is the point I was making in an earlier email to 
Viresh's patch -- we shouldn't be doing any CPU check for the call backs 
at the scheduler or ever governor level.


That would simplify this whole thing by deleting a bunch of code. And 
having much simpler checks in those drivers that actually have to deal 
with their HW specific details.



--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project