Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1403352 > unrolled thread

Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-19 01:40 +0200
    Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-19 14:10 +0200
      Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs Steve Muckle <steve.muckle@linaro.org> - 2016-05-19 21:20 +0200
        Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-19 23:10 +0200
          Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs Steve Muckle <steve.muckle@linaro.org> - 2016-05-20 01:10 +0200
            Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs Steve Muckle <steve.muckle@linaro.org> - 2016-05-21 21:50 +0200

#1403352 — Re: [PATCH 3/5] sched: cpufreq: call cpufreq hook from remote CPUs

On Mon, May 9, 2016 at 11:20 PM, Steve Muckle <steve.muckle@linaro.org> wrote:
> Without calling the cpufreq hook for a remote wakeup it is possible
> for such a wakeup to go unnoticed by cpufreq on the target CPU for up
> to a full tick. This can occur if the target CPU is running a
> CPU-bound task and preemption does not occur. If preemption does occur
> then the scheduler is expected to run soon on the target CPU anyway so
> invoking the cpufreq hook on the remote wakeup is not required.
>
> In order to avoid unnecessary interprocessor communication in the
> governor for the preemption case, the existing hook (which happens
> before preemption is decided) is only called when the target CPU
> is within the current CPU's cpufreq policy. A new hook is added in
> check_preempt_curr() to handle events outside the current CPU's
> cpufreq policy where preemption does not happen.
>
> Some governors may opt to not receive remote CPU callbacks. This
> behavior is possible by providing NULL as the new policy_cpus
> parameter to cpufreq_add_update_util_hook(). Callbacks will only be
> issued in this case when the target CPU and the current CPU are the
> same. Otherwise policy_cpus is used to determine what is a local
> vs. remote callback.

I don't really like the extra complexity added by this patch.

It makes the code look fragile at some places and it only really
matters for schedutil and for the fast switch case in there.

Overall, it looks like a premature optimization to me.

[toc] | [next] | [standalone]

#1403696

On Thu, May 19, 2016 at 1:33 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, May 9, 2016 at 11:20 PM, Steve Muckle <steve.muckle@linaro.org> wrote:
>> Without calling the cpufreq hook for a remote wakeup it is possible
>> for such a wakeup to go unnoticed by cpufreq on the target CPU for up
>> to a full tick. This can occur if the target CPU is running a
>> CPU-bound task and preemption does not occur. If preemption does occur
>> then the scheduler is expected to run soon on the target CPU anyway so
>> invoking the cpufreq hook on the remote wakeup is not required.
>>
>> In order to avoid unnecessary interprocessor communication in the
>> governor for the preemption case, the existing hook (which happens
>> before preemption is decided) is only called when the target CPU
>> is within the current CPU's cpufreq policy. A new hook is added in
>> check_preempt_curr() to handle events outside the current CPU's
>> cpufreq policy where preemption does not happen.
>>
>> Some governors may opt to not receive remote CPU callbacks. This
>> behavior is possible by providing NULL as the new policy_cpus
>> parameter to cpufreq_add_update_util_hook(). Callbacks will only be
>> issued in this case when the target CPU and the current CPU are the
>> same. Otherwise policy_cpus is used to determine what is a local
>> vs. remote callback.
>
> I don't really like the extra complexity added by this patch.
>
> It makes the code look fragile at some places and it only really
> matters for schedutil and for the fast switch case in there.
>
> Overall, it looks like a premature optimization to me.

In particular, the dance with checking the policy CPUs from the
scheduler is questionable (the scheduler might be interested in this
information for other purposes too and hooking it up in an ad-hoc way
just for cpufreq doesn't seem to be appropriate from that perspective)
and also doesn't seem to be necessary.

You can check if the current CPU is a policy one from the governor and
if that is the case, simply run the frequency update on it directly
without any IPI (because if both the target CPU and the current CPU
belong to the same policy, it doesn't matter which one of them will
run the update).

[toc] | [prev] | [next] | [standalone]

#1403932

On Thu, May 19, 2016 at 02:00:54PM +0200, Rafael J. Wysocki wrote:
> On Thu, May 19, 2016 at 1:33 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Mon, May 9, 2016 at 11:20 PM, Steve Muckle <steve.muckle@linaro.org> wrote:
> >> Without calling the cpufreq hook for a remote wakeup it is possible
> >> for such a wakeup to go unnoticed by cpufreq on the target CPU for up
> >> to a full tick. This can occur if the target CPU is running a
> >> CPU-bound task and preemption does not occur. If preemption does occur
> >> then the scheduler is expected to run soon on the target CPU anyway so
> >> invoking the cpufreq hook on the remote wakeup is not required.
> >>
> >> In order to avoid unnecessary interprocessor communication in the
> >> governor for the preemption case, the existing hook (which happens
> >> before preemption is decided) is only called when the target CPU
> >> is within the current CPU's cpufreq policy. A new hook is added in
> >> check_preempt_curr() to handle events outside the current CPU's
> >> cpufreq policy where preemption does not happen.
> >>
> >> Some governors may opt to not receive remote CPU callbacks. This
> >> behavior is possible by providing NULL as the new policy_cpus
> >> parameter to cpufreq_add_update_util_hook(). Callbacks will only be
> >> issued in this case when the target CPU and the current CPU are the
> >> same. Otherwise policy_cpus is used to determine what is a local
> >> vs. remote callback.
> >
> > I don't really like the extra complexity added by this patch.
> >
> > It makes the code look fragile at some places 

Perhaps I can improve these, can you point them out?

> > and it only really matters for schedutil 

True. As we are trying to create an integrated scheduler+CPU frequency
control solution I think some of this is to be expected, and should be
worthwhile since this is hopefully the future and will eventually
replace the need for the other governors.

> > and for the fast switch case in there.

Once there is a high priority context for the slow path I expect it to
benefit from this as well.

> >
> > Overall, it looks like a premature optimization to me.

Are you referring to this new approach of avoiding duplicate IPIs, or
supporting updates on remote wakeups overall?

The duplicate IPI thing is admittedly not required for the problem I'm
looking to solve but I figured at least some people would be concerned
about it.  If folks can live with it for now then I can go back to the
simpler approach I had in my first posting.
 
> In particular, the dance with checking the policy CPUs from the
> scheduler is questionable (the scheduler might be interested in this
> information for other purposes too and hooking it up in an ad-hoc way
> just for cpufreq doesn't seem to be appropriate from that perspective)
> and also doesn't seem to be necessary.
> 
> You can check if the current CPU is a policy one from the governor and
> if that is the case, simply run the frequency update on it directly
> without any IPI (because if both the target CPU and the current CPU
> belong to the same policy, it doesn't matter which one of them will
> run the update).

The checking of policy CPUs from the scheduler was done so as to
minimize the number of calls to the hook, given their expense.

In the case of a remote update the hook has to run (or not) after it is
known whether preemption will occur so we don't do needless work or
IPIs. If the policy CPUs aren't known in the scheduler then the early
hook will always need to be called along with an indication that it is
the early hook being called. If it turns out to be a remote update it
could then be deferred to the later hook, which would only be called
when a remote update has been deferred and preemption has not occurred.

This means two hook inovcations for a remote non-preempting wakeup
though instead of one.  Perhaps this is a good middle ground on code
churn vs. optimization though.

thanks,
Steve

[toc] | [prev] | [next] | [standalone]

#1403964

On Thu, May 19, 2016 at 9:19 PM, Steve Muckle <steve.muckle@linaro.org> wrote:
> On Thu, May 19, 2016 at 02:00:54PM +0200, Rafael J. Wysocki wrote:
>> On Thu, May 19, 2016 at 1:33 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Mon, May 9, 2016 at 11:20 PM, Steve Muckle <steve.muckle@linaro.org> wrote:
>> >> Without calling the cpufreq hook for a remote wakeup it is possible
>> >> for such a wakeup to go unnoticed by cpufreq on the target CPU for up
>> >> to a full tick. This can occur if the target CPU is running a
>> >> CPU-bound task and preemption does not occur. If preemption does occur
>> >> then the scheduler is expected to run soon on the target CPU anyway so
>> >> invoking the cpufreq hook on the remote wakeup is not required.
>> >>
>> >> In order to avoid unnecessary interprocessor communication in the
>> >> governor for the preemption case, the existing hook (which happens
>> >> before preemption is decided) is only called when the target CPU
>> >> is within the current CPU's cpufreq policy. A new hook is added in
>> >> check_preempt_curr() to handle events outside the current CPU's
>> >> cpufreq policy where preemption does not happen.
>> >>
>> >> Some governors may opt to not receive remote CPU callbacks. This
>> >> behavior is possible by providing NULL as the new policy_cpus
>> >> parameter to cpufreq_add_update_util_hook(). Callbacks will only be
>> >> issued in this case when the target CPU and the current CPU are the
>> >> same. Otherwise policy_cpus is used to determine what is a local
>> >> vs. remote callback.
>> >
>> > I don't really like the extra complexity added by this patch.
>> >
>> > It makes the code look fragile at some places
>
> Perhaps I can improve these, can you point them out?
>
>> > and it only really matters for schedutil
>
> True. As we are trying to create an integrated scheduler+CPU frequency
> control solution I think some of this is to be expected, and should be
> worthwhile since this is hopefully the future and will eventually
> replace the need for the other governors.
>
>> > and for the fast switch case in there.
>
> Once there is a high priority context for the slow path I expect it to
> benefit from this as well.

I don't think that saving an occasional IPI would matter for that case
overall, though.

>> >
>> > Overall, it looks like a premature optimization to me.
>
> Are you referring to this new approach of avoiding duplicate IPIs,

Yes.

> or supporting updates on remote wakeups overall?

No.  I already said I would be fine with that if done properly.

> The duplicate IPI thing is admittedly not required for the problem I'm
> looking to solve but I figured at least some people would be concerned
> about it.

Avoiding IPIs that aren't necessary is fine by me in general, but
doing that at the scheduler level doesn't seem to be necessary as I
said.

> If folks can live with it for now then I can go back to the
> simpler approach I had in my first posting.

Please at least avoid introducing internal cpufreq concepts into the
scheduler uncleanly.

>> In particular, the dance with checking the policy CPUs from the
>> scheduler is questionable (the scheduler might be interested in this
>> information for other purposes too and hooking it up in an ad-hoc way
>> just for cpufreq doesn't seem to be appropriate from that perspective)
>> and also doesn't seem to be necessary.
>>
>> You can check if the current CPU is a policy one from the governor and
>> if that is the case, simply run the frequency update on it directly
>> without any IPI (because if both the target CPU and the current CPU
>> belong to the same policy, it doesn't matter which one of them will
>> run the update).
>
> The checking of policy CPUs from the scheduler was done so as to
> minimize the number of calls to the hook, given their expense.

But policy CPUs is entirely an internal cpufreq concept and adding
that to the scheduler kind of via a kitchen door doesn't look good to
me.

> In the case of a remote update the hook has to run (or not) after it is
> known whether preemption will occur so we don't do needless work or
> IPIs. If the policy CPUs aren't known in the scheduler then the early
> hook will always need to be called along with an indication that it is
> the early hook being called. If it turns out to be a remote update it
> could then be deferred to the later hook, which would only be called
> when a remote update has been deferred and preemption has not occurred.
>
> This means two hook inovcations for a remote non-preempting wakeup
> though instead of one.  Perhaps this is a good middle ground on code
> churn vs. optimization though.

I would think so.

[toc] | [prev] | [next] | [standalone]

#1404009

On Thu, May 19, 2016 at 11:06:14PM +0200, Rafael J. Wysocki wrote:
> > In the case of a remote update the hook has to run (or not) after it is
> > known whether preemption will occur so we don't do needless work or
> > IPIs. If the policy CPUs aren't known in the scheduler then the early
> > hook will always need to be called along with an indication that it is
> > the early hook being called. If it turns out to be a remote update it
> > could then be deferred to the later hook, which would only be called
> > when a remote update has been deferred and preemption has not occurred.
> >
> > This means two hook inovcations for a remote non-preempting wakeup
> > though instead of one.  Perhaps this is a good middle ground on code
> > churn vs. optimization though.
> 
> I would think so.

Ok, I will pursue this approach.

thanks,
Steve

[toc] | [prev] | [next] | [standalone]

#1404894

Hi Peter, Ingo,

On Thu, May 19, 2016 at 04:04:19PM -0700, Steve Muckle wrote:
> On Thu, May 19, 2016 at 11:06:14PM +0200, Rafael J. Wysocki wrote:
> > > In the case of a remote update the hook has to run (or not) after it is
> > > known whether preemption will occur so we don't do needless work or
> > > IPIs. If the policy CPUs aren't known in the scheduler then the early
> > > hook will always need to be called along with an indication that it is
> > > the early hook being called. If it turns out to be a remote update it
> > > could then be deferred to the later hook, which would only be called
> > > when a remote update has been deferred and preemption has not occurred.
> > >
> > > This means two hook inovcations for a remote non-preempting wakeup
> > > though instead of one.  Perhaps this is a good middle ground on code
> > > churn vs. optimization though.
> > 
> > I would think so.
> 
> Ok, I will pursue this approach.

I'd like to get your opinion here before proceeding further...

To catch you up and summarize, I'm trying to implement support for
calling the scheduler cpufreq callback during remote wakeups.  Currently
the scheduler cpufreq callback is only called when the target CPU is the
current CPU. If a remote wakeup does not result in preemption, the CPU
frequency may currently not be adjusted appropriately for up to a tick,
when we are guaranteed to call the hook again.

Invoking schedutil promptly for the target CPU in this situation means
sending an IPI if the current CPU is not in the target CPU's frequency
domain. This is because often a cpufreq driver must run on a CPU within
the frequency domain it is bound to.  But the catch is that we should
not do this and incur the overhead of an IPI if preemption will occur,
as in that case the scheduler (and schedutil) will run soon on the
target CPU anyway, potentially as a result of the scheduler sending its
own IPI.

I figured this unnecessary overhead would be unacceptable and so have
been working on an approach to avoid it. Unfortunately the current hooks
happen before the preemption decision is made. My current implementation
sets a flag if schedutil sees a remote wakeup and then bails. There's a
test to call the hook again at the end of check_preempt_curr() if the flag
is set.  The flag is cleared by resched_curr() as that means preemption
will happen on the target CPU. The flag currently lives at the end of
the rq struct. I could move it into the update_util_data hook structure
or elsewhere, but that would mean accessing another per-cpu item in
hot scheduler paths.

Thoughts? Note the current implementation described above differs a bit
from the last posting in this thread, per discussion with Rafael.

thanks,
Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web