On 18-02-21, 16:25, Yue Hu wrote:
> From: Yue Hu <[email protected]>
>
> For busy CPU case, we do not need to avoid freq reduction if limits
> change since commit 600f5badb78c ("cpufreq: schedutil: Don't skip
> freq update when limits change").
>
> Later, commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq
> update if need_freq_update is set") discarded the need_freq_update
> check for special case of busy CPU because we won't abort a frequency
> update anymore if need_freq_update is set.
>
> That is nonlogical since we will not reduce the freq for busy CPU
> if the computed next_f is really reduced when limits change.
Schedutil governor will probably ask for a higher frequency than
allowed, but cpufreq core will clamp the request between policy
min/max before updating the frequency here.
We added the check in 600f5badb78c here earlier as there were chances
that we will abort the operation without reaching to cpufreq core,
which won't happen now.
--
viresh
On Thu, 18 Feb 2021 15:50:29 +0530
Viresh Kumar <[email protected]> wrote:
> On 18-02-21, 16:25, Yue Hu wrote:
> > From: Yue Hu <[email protected]>
> >
> > For busy CPU case, we do not need to avoid freq reduction if limits
> > change since commit 600f5badb78c ("cpufreq: schedutil: Don't skip
> > freq update when limits change").
> >
> > Later, commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq
> > update if need_freq_update is set") discarded the need_freq_update
> > check for special case of busy CPU because we won't abort a
> > frequency update anymore if need_freq_update is set.
> >
> > That is nonlogical since we will not reduce the freq for busy CPU
> > if the computed next_f is really reduced when limits change.
>
> Schedutil governor will probably ask for a higher frequency than
> allowed, but cpufreq core will clamp the request between policy
> min/max before updating the frequency here.
>
> We added the check in 600f5badb78c here earlier as there were chances
> that we will abort the operation without reaching to cpufreq core,
> which won't happen now.
>
There's a possibility: we will use the previous freq to update if next_f
is reduced for busy CPU if need_freq_update is set in
sugov_update_next_freq(). This possibility would happen now? And this
update is what we want if it happens? This is related to another
possible patch ready to send.
Thank you.
On 19-02-21, 11:38, Yue Hu wrote:
> There's a possibility: we will use the previous freq to update if next_f
> is reduced for busy CPU if need_freq_update is set in
> sugov_update_next_freq().
Right.
> This possibility would happen now? And this
> update is what we want if it happens?
This is exactly what we want here, don't reduce speed for busy CPU, but we also
need to make sure we are in the policy's valid range which cpufreq core will
take care of.
> This is related to another possible patch ready to send.
I am not sure what's there to send now.
--
viresh
On Fri, 19 Feb 2021 09:39:33 +0530
Viresh Kumar <[email protected]> wrote:
> On 19-02-21, 11:38, Yue Hu wrote:
> > There's a possibility: we will use the previous freq to update if
> > next_f is reduced for busy CPU if need_freq_update is set in
> > sugov_update_next_freq().
>
> Right.
>
> > This possibility would happen now? And this
> > update is what we want if it happens?
>
> This is exactly what we want here, don't reduce speed for busy CPU,
I understand it should not skip this update but set the same freq as
previous one again for the specail case if need_freq_update is set. Am
i rt?
> but we also need to make sure we are in the policy's valid range
> which cpufreq core will take care of.
>
> > This is related to another possible patch ready to send.
>
> I am not sure what's there to send now.
I will send later after figure out the doubt above.
>
On 19-02-21, 14:41, Yue Hu wrote:
> On Fri, 19 Feb 2021 09:39:33 +0530
> Viresh Kumar <[email protected]> wrote:
>
> > On 19-02-21, 11:38, Yue Hu wrote:
> > > There's a possibility: we will use the previous freq to update if
> > > next_f is reduced for busy CPU if need_freq_update is set in
> > > sugov_update_next_freq().
> >
> > Right.
> >
> > > This possibility would happen now? And this
> > > update is what we want if it happens?
> >
> > This is exactly what we want here, don't reduce speed for busy CPU,
>
> I understand it should not skip this update but set the same freq as
> previous one again for the special case if need_freq_update is set. Am
> i rt?
The special check, about not reducing freq if CPU had been busy
recently, doesn't have anything to do with need_freq_update.
Though previously we added the need_freq_update check there to make
sure we account for any recent policy min/max change and don't skip
freq update anymore. That won't happen anymore and so we don't need
any check here related to need_freq_update.
If you still have doubt, please explain your concern in detail with an
example as I am failing to understand it.
--
viresh
On Fri, 19 Feb 2021 13:12:49 +0530
Viresh Kumar <[email protected]> wrote:
> On 19-02-21, 14:41, Yue Hu wrote:
> > On Fri, 19 Feb 2021 09:39:33 +0530
> > Viresh Kumar <[email protected]> wrote:
> >
> > > On 19-02-21, 11:38, Yue Hu wrote:
> > > > There's a possibility: we will use the previous freq to update
> > > > if next_f is reduced for busy CPU if need_freq_update is set in
> > > > sugov_update_next_freq().
> > >
> > > Right.
> > >
> > > > This possibility would happen now? And this
> > > > update is what we want if it happens?
> > >
> > > This is exactly what we want here, don't reduce speed for busy
> > > CPU,
> >
> > I understand it should not skip this update but set the same freq as
> > previous one again for the special case if need_freq_update is set.
> > Am i rt?
>
> The special check, about not reducing freq if CPU had been busy
> recently, doesn't have anything to do with need_freq_update.
However, we will skip the update if need_freq_update is not set. And do
the update if need_freq_update is set.
Note that there are unnecessary fast switch check and spin lock/unlock
operations in freq skip path.
If we consider unnecessary behaviors above, then we should return right
away rather than continue to execute following code.
Consequently, we also need to consider need_update_freq flag since we
need to update freq currently if it is set.
>
> Though previously we added the need_freq_update check there to make
> sure we account for any recent policy min/max change and don't skip
> freq update anymore. That won't happen anymore and so we don't need
> any check here related to need_freq_update.
>
> If you still have doubt, please explain your concern in detail with an
> example as I am failing to understand it.
>
On 19-02-21, 16:20, Yue Hu wrote:
> However, we will skip the update if need_freq_update is not set.
Not really, we will update freq periodically nevertheless, around
every 10ms or something..
> And do the update if need_freq_update is set.
Yeah, that breaks the periodic cycle to attend to some urgent request.
> Note that there are unnecessary fast switch check and spin lock/unlock
> operations in freq skip path.
Maybe, I am not sure. We are all up for optimizations if there are
any.
> If we consider unnecessary behaviors above, then we should return right
> away rather than continue to execute following code.
As I said earlier, we may end up updating the frequency even if
need_freq_update is unset.
--
viresh
On Fri, 19 Feb 2021 15:05:51 +0530
Viresh Kumar <[email protected]> wrote:
> On 19-02-21, 16:20, Yue Hu wrote:
> > However, we will skip the update if need_freq_update is not set.
>
> Not really, we will update freq periodically nevertheless, around
> every 10ms or something..
>
> > And do the update if need_freq_update is set.
>
> Yeah, that breaks the periodic cycle to attend to some urgent request.
>
> > Note that there are unnecessary fast switch check and spin
> > lock/unlock operations in freq skip path.
>
> Maybe, I am not sure. We are all up for optimizations if there are
> any.
We will set next_f to next_freq(previous freq) if next_f is
reduced for busy CPU. Then the next sugov_update_next_freq() will check
if next_freq matches next_f if need_freq_update is not set.
Obviously, we will do nothing for the case. And The related check to
fast_switch_enabled and raw_spin_{lock,unlock} operations are
unnecessary.
>
> > If we consider unnecessary behaviors above, then we should return
> > right away rather than continue to execute following code.
>
> As I said earlier, we may end up updating the frequency even if
> need_freq_update is unset.
>
On 19-02-21, 19:45, Yue Hu wrote:
> We will set next_f to next_freq(previous freq) if next_f is
> reduced for busy CPU. Then the next sugov_update_next_freq() will check
> if next_freq matches next_f if need_freq_update is not set.
> Obviously, we will do nothing for the case. And The related check to
> fast_switch_enabled and raw_spin_{lock,unlock} operations are
> unnecessary.
Right, but we will still need sugov_update_next_freq() to have the
same implementation regardless and so I am not sure if we should add
this change:
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 41e498b0008a..7289e1adab73 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
* recently, as the reduction is likely to be premature then.
*/
if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
+ if (!sg_policy->need_freq_update)
+ return;
+
next_f = sg_policy->next_freq;
/* Restore cached freq as next_freq has changed */
--
viresh
On Mon, 22 Feb 2021 11:00:14 +0530
Viresh Kumar <[email protected]> wrote:
> On 19-02-21, 19:45, Yue Hu wrote:
> > We will set next_f to next_freq(previous freq) if next_f is
> > reduced for busy CPU. Then the next sugov_update_next_freq() will check
> > if next_freq matches next_f if need_freq_update is not set.
> > Obviously, we will do nothing for the case. And The related check to
> > fast_switch_enabled and raw_spin_{lock,unlock} operations are
> > unnecessary.
>
> Right, but we will still need sugov_update_next_freq() to have the
> same implementation regardless and so I am not sure if we should add
Yes, sugov_update_next_freq() should be keeping current logic for corner case.
> this change:
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 41e498b0008a..7289e1adab73 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> * recently, as the reduction is likely to be premature then.
> */
> if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> + if (!sg_policy->need_freq_update)
The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
case) should be skipping the freq update.
Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
we add the check to busy CPU for not skipping the update, we need to update the freq using
computed one because limits change.
After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
even though current freq == sg_policy->next_freq because of corner case issue. But that is
conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
busy CPU.
> + return;
> +
Yes, it's what i want ot add for unnecessary behaviors i mentioned above. Add return here
should just be for skipping update(different from corner case in commit23a881852f3e).
> next_f = sg_policy->next_freq;
>
> /* Restore cached freq as next_freq has changed */
>
>
On Mon, Feb 22, 2021 at 2:57 PM Yue Hu <[email protected]> wrote:
>
> On Mon, 22 Feb 2021 11:00:14 +0530
> Viresh Kumar <[email protected]> wrote:
>
> > On 19-02-21, 19:45, Yue Hu wrote:
> > > We will set next_f to next_freq(previous freq) if next_f is
> > > reduced for busy CPU. Then the next sugov_update_next_freq() will check
> > > if next_freq matches next_f if need_freq_update is not set.
> > > Obviously, we will do nothing for the case. And The related check to
> > > fast_switch_enabled and raw_spin_{lock,unlock} operations are
> > > unnecessary.
> >
> > Right, but we will still need sugov_update_next_freq() to have the
> > same implementation regardless and so I am not sure if we should add
>
> Yes, sugov_update_next_freq() should be keeping current logic for corner case.
>
> > this change:
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 41e498b0008a..7289e1adab73 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > * recently, as the reduction is likely to be premature then.
> > */
> > if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > + if (!sg_policy->need_freq_update)
>
> The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
> case) should be skipping the freq update.
>
> Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
> we add the check to busy CPU for not skipping the update, we need to update the freq using
> computed one because limits change.
>
> After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
> is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
> and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
> even though current freq == sg_policy->next_freq because of corner case issue. But that is
> conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
> busy CPU.
That's because we realized that it was not always a good idea to skip
the update even if next_f == sg_policy->next_freq.
That's why CPUFREQ_NEED_UPDATE_LIMITS has been introduced and the
current flow is a result of subsequent code rearrangements.
On Mon, 22 Feb 2021 15:30:34 +0100
"Rafael J. Wysocki" <[email protected]> wrote:
> On Mon, Feb 22, 2021 at 2:57 PM Yue Hu <[email protected]> wrote:
> >
> > On Mon, 22 Feb 2021 11:00:14 +0530
> > Viresh Kumar <[email protected]> wrote:
> >
> > > On 19-02-21, 19:45, Yue Hu wrote:
> > > > We will set next_f to next_freq(previous freq) if next_f is
> > > > reduced for busy CPU. Then the next sugov_update_next_freq() will check
> > > > if next_freq matches next_f if need_freq_update is not set.
> > > > Obviously, we will do nothing for the case. And The related check to
> > > > fast_switch_enabled and raw_spin_{lock,unlock} operations are
> > > > unnecessary.
> > >
> > > Right, but we will still need sugov_update_next_freq() to have the
> > > same implementation regardless and so I am not sure if we should add
> >
> > Yes, sugov_update_next_freq() should be keeping current logic for corner case.
> >
> > > this change:
> > >
> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > index 41e498b0008a..7289e1adab73 100644
> > > --- a/kernel/sched/cpufreq_schedutil.c
> > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > @@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > > * recently, as the reduction is likely to be premature then.
> > > */
> > > if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > > + if (!sg_policy->need_freq_update)
> >
> > The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
> > case) should be skipping the freq update.
> >
> > Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
> > we add the check to busy CPU for not skipping the update, we need to update the freq using
> > computed one because limits change.
> >
> > After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
> > is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
> > and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
> > even though current freq == sg_policy->next_freq because of corner case issue. But that is
> > conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
> > busy CPU.
>
> That's because we realized that it was not always a good idea to skip
> the update even if next_f == sg_policy->next_freq.
>
> That's why CPUFREQ_NEED_UPDATE_LIMITS has been introduced and the
> current flow is a result of subsequent code rearrangements.
ok, care about unnecessary(should be) behaviors(fast_switch_enabled and raw_spin_{lock,unlock})
if need_freq_update is unset?
If we care, i will send another patch (which is different from above change for busy CPU).
Thank you.
On Wed, Feb 24, 2021 at 3:24 AM Yue Hu <[email protected]> wrote:
>
> On Mon, 22 Feb 2021 15:30:34 +0100
> "Rafael J. Wysocki" <[email protected]> wrote:
>
> > On Mon, Feb 22, 2021 at 2:57 PM Yue Hu <[email protected]> wrote:
> > >
> > > On Mon, 22 Feb 2021 11:00:14 +0530
> > > Viresh Kumar <[email protected]> wrote:
> > >
> > > > On 19-02-21, 19:45, Yue Hu wrote:
> > > > > We will set next_f to next_freq(previous freq) if next_f is
> > > > > reduced for busy CPU. Then the next sugov_update_next_freq() will check
> > > > > if next_freq matches next_f if need_freq_update is not set.
> > > > > Obviously, we will do nothing for the case. And The related check to
> > > > > fast_switch_enabled and raw_spin_{lock,unlock} operations are
> > > > > unnecessary.
> > > >
> > > > Right, but we will still need sugov_update_next_freq() to have the
> > > > same implementation regardless and so I am not sure if we should add
> > >
> > > Yes, sugov_update_next_freq() should be keeping current logic for corner case.
> > >
> > > > this change:
> > > >
> > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > index 41e498b0008a..7289e1adab73 100644
> > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > @@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > > > * recently, as the reduction is likely to be premature then.
> > > > */
> > > > if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > > > + if (!sg_policy->need_freq_update)
> > >
> > > The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
> > > case) should be skipping the freq update.
> > >
> > > Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
> > > we add the check to busy CPU for not skipping the update, we need to update the freq using
> > > computed one because limits change.
> > >
> > > After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
> > > is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
> > > and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
> > > even though current freq == sg_policy->next_freq because of corner case issue. But that is
> > > conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
> > > busy CPU.
> >
> > That's because we realized that it was not always a good idea to skip
> > the update even if next_f == sg_policy->next_freq.
> >
> > That's why CPUFREQ_NEED_UPDATE_LIMITS has been introduced and the
> > current flow is a result of subsequent code rearrangements.
>
> ok, care about unnecessary(should be) behaviors(fast_switch_enabled and raw_spin_{lock,unlock})
> if need_freq_update is unset?
>
> If we care, i will send another patch (which is different from above change for busy CPU).
Please send a patch and we'll see (this is how things go).
On Wed, 24 Feb 2021 13:46:11 +0100
"Rafael J. Wysocki" <[email protected]> wrote:
> On Wed, Feb 24, 2021 at 3:24 AM Yue Hu <[email protected]> wrote:
> >
> > On Mon, 22 Feb 2021 15:30:34 +0100
> > "Rafael J. Wysocki" <[email protected]> wrote:
> >
> > > On Mon, Feb 22, 2021 at 2:57 PM Yue Hu <[email protected]> wrote:
> > > >
> > > > On Mon, 22 Feb 2021 11:00:14 +0530
> > > > Viresh Kumar <[email protected]> wrote:
> > > >
> > > > > On 19-02-21, 19:45, Yue Hu wrote:
> > > > > > We will set next_f to next_freq(previous freq) if next_f is
> > > > > > reduced for busy CPU. Then the next sugov_update_next_freq() will check
> > > > > > if next_freq matches next_f if need_freq_update is not set.
> > > > > > Obviously, we will do nothing for the case. And The related check to
> > > > > > fast_switch_enabled and raw_spin_{lock,unlock} operations are
> > > > > > unnecessary.
> > > > >
> > > > > Right, but we will still need sugov_update_next_freq() to have the
> > > > > same implementation regardless and so I am not sure if we should add
> > > >
> > > > Yes, sugov_update_next_freq() should be keeping current logic for corner case.
> > > >
> > > > > this change:
> > > > >
> > > > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > > > index 41e498b0008a..7289e1adab73 100644
> > > > > --- a/kernel/sched/cpufreq_schedutil.c
> > > > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > > > @@ -362,6 +362,9 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > > > > * recently, as the reduction is likely to be premature then.
> > > > > */
> > > > > if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
> > > > > + if (!sg_policy->need_freq_update)
> > > >
> > > > The initial purpose about code of `next_f = sg_policy->next_freq` here (for special CPU busy
> > > > case) should be skipping the freq update.
> > > >
> > > > Since commit 600f5badb78c ("cpufreq: schedutil: Don't skip freq update when limits change"),
> > > > we add the check to busy CPU for not skipping the update, we need to update the freq using
> > > > computed one because limits change.
> > > >
> > > > After commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update
> > > > is set"), we removed the need_freq_update check(no issue of commit 600f5badb78c anymore?)
> > > > and introduce to always do an update in sugov_update_next_freq() if need_freq_update is set
> > > > even though current freq == sg_policy->next_freq because of corner case issue. But that is
> > > > conflict with original purpose of the freq skip code (next_f = sg_policy->next_freq) of
> > > > busy CPU.
> > >
> > > That's because we realized that it was not always a good idea to skip
> > > the update even if next_f == sg_policy->next_freq.
> > >
> > > That's why CPUFREQ_NEED_UPDATE_LIMITS has been introduced and the
> > > current flow is a result of subsequent code rearrangements.
> >
> > ok, care about unnecessary(should be) behaviors(fast_switch_enabled and raw_spin_{lock,unlock})
> > if need_freq_update is unset?
> >
> > If we care, i will send another patch (which is different from above change for busy CPU).
>
> Please send a patch and we'll see (this is how things go).
Already sent it("Call sugov_update_next_freq() before check to fast_switch_enabled"). Please review.
Thank you.