Re: [RFC PATCH 1/4] cpufreq: governor: support scheduler cpufreq callbacks on remote CPUs

[Rafael J. Wysocki]

On Mon, Apr 25, 2016 at 9:17 PM, Steve Muckle <steve.muckle@xxxxxxxxxx> wrote:
> On Wed, Apr 20, 2016 at 02:26:06PM +0200, Rafael J. Wysocki wrote:
>> You could have added a cover [0/4] message which would have made it easier
>> to reply to the entire series in general.  Let me do it here.
>
> Will add that next time.
>
>> Doing it the way it is done in this series would be fine by me in general
>> (up to a few more or less minor comments), but it is still unclear to me
>> how much of a difference these changes would make in terms of improved
>> response times etc.
>
> I spent some time last week constructing a test case where the
> benefits could be seen. A task which was previously low utilization
> wakes on CPU0 and becomes CPU bound. Just after that, a new task is
> spawned on CPU0. The initial task utilization is high so ideally we
> would like to see the frequency immediately rise, but in my test it
> does not occur until the next tick. There is 7ms of delay in the trace
> I've saved.
>
> Unfortunately these patches alone will not address it. There are a
> couple other issues which get in the way (which is why I didn't
> respond here right away). Let me spend some more time on those and see
> how it goes.

I see.

>> > In preparation for the scheduler cpufreq callback happening
>> > on remote CPUs, add support for this in the dbs governors.
>> > The dbs governors make assumptions about the callback occurring
>> > on the CPU being updated.
>>
>> While the above is generally correct, it would be nice to say more about what
>> happens in the patch.  Like:
>>
>> "To that end, add a CPU number field to struct cpu_dbs_info and modify
>> dbs_update_util_handler() to schedule IRQ works on target CPUs rather than on
>> the local one only."
>
> I'm happy to do that if it is what you'd like to see, but just
> curious, isn't it really just restating the patch contents?

It is somewhat, but that's for the benefit of whoever reads the git
history without necessarily looking and the changes themselves
upfront.

> ...
>> > diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
>> > index 3e0eb7c54903..1d5f4857ff80 100644
>> > --- a/drivers/cpufreq/cpufreq_governor.h
>> > +++ b/drivers/cpufreq/cpufreq_governor.h
>> > @@ -122,6 +122,7 @@ struct cpu_dbs_info {
>> >     unsigned int prev_load;
>> >     struct update_util_data update_util;
>> >     struct policy_dbs_info *policy_dbs;
>> > +   int cpu;
>> >  };
>>
>> Wouldn't it be better to add the cpu field to struct update_util_data and set
>> it from cpufreq_add_update_util_hook()?
>>
>> That would allow you to avoid adding the cpu field to struct sugov_cpu in the
>> second patch at least.
>
> Sure, will do.

Thanks!