sched: cpufreq: Track util update flags

Initializing sg_cpu->flags to SCHED_CPUFREQ_RT has no obvious benefit. The flags field wouldn't be used until the utilization update handler is called for the first time, and once that is called we will overwrite flags anyway.

Initialize it to 0.

Change-Id: I028dbb40c5c242cff52fe1b14aeaff37f29a2f8d

Signed-off-by Viresh Kumar <[email protected]>

Hi Viresh,

Quoted 5 lines · Viresh Kumar

Initializing sg_cpu->flags to SCHED_CPUFREQ_RT has no obvious benefit. The flags field wouldn't be used until the utilization update handler is called for the first time, and once that is called we will overwrite flags anyway.

Quoted 3 lines

Initialize it to 0.

Change-Id: I028dbb40c5c242cff52fe1b14aeaff37f29a2f8d

Without ^

Quoted 1 line

Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Juri Lelli <[email protected]>

Best,

- Juri

Currently the schedutil governor overwrites the sg_cpu->flags field on every call to the utilization handler. It was pretty good as the initial implementation of utilization handlers, there are several drawbacks though.

The biggest drawback is that the sg_cpu->flags field doesn't always represent the correct type of tasks that are enqueued on a CPU's rq. For example, if a fair task is enqueued while a RT or DL task is running, we will overwrite the flags with value 0 and that may take the CPU to lower OPPs unintentionally. There can be other corner cases as well which we aren't aware of currently.

This patch changes the current implementation to keep track of all the task types that are currently enqueued to the CPUs rq. A new flag: CLEAR is introduced and is set by the scheduling classes when their last task is dequeued. When the CLEAR flag bit is set, the schedutil governor resets all the other flag bits that are present in the flags parameter. For now, the util update handlers return immediately if they were called to clear the flag.

Signed-off-by Viresh Kumar <[email protected]>

---
 include/linux/sched/cpufreq.h    |  7 ++++++-
 kernel/sched/cpufreq_schedutil.c | 21 ++++++++++++++++++---
 kernel/sched/deadline.c          |  4 ++++
 kernel/sched/fair.c              |  8 ++++++--
 kernel/sched/rt.c                |  4 ++++
 5 files changed, 38 insertions(+), 6 deletions(-)

include/linux/sched/cpufreq.h +0 -0

index d1ad3d825561..6f6641e61236 100644

This flag doesn't do much, does it? I mean RT/DL/IOWAIT are used to bump up frequency, while you are adding CFS for the sake of simmetry, right? And with my patches DL will hopefully soon be in the same situation. If my understanding is correct, maybe add a comment?

Currently the schedutil governor overwrites the sg_cpu->flags field on every call to the utilization handler. It was pretty good as the initial implementation of utilization handlers, there are several drawbacks though.

The biggest drawback is that the sg_cpu->flags field doesn't always represent the correct type of tasks that are enqueued on a CPU's rq. For example, if a fair task is enqueued while a RT or DL task is running, we will overwrite the flags with value 0 and that may take the CPU to lower OPPs unintentionally. There can be other corner cases as well which we aren't aware of currently.

This patch changes the current implementation to keep track of all the task types that are currently enqueued to the CPUs rq. A new flag: CLEAR is introduced and is set by the scheduling classes when their last task is dequeued. When the CLEAR flag bit is set, the schedutil governor resets all the other flag bits that are present in the flags parameter. For now, the util update handlers return immediately if they were called to clear the flag.

Signed-off-by Viresh Kumar <[email protected]>

---
 include/linux/sched/cpufreq.h    |  7 ++++++-
 kernel/sched/cpufreq_schedutil.c | 21 ++++++++++++++++++---
 kernel/sched/deadline.c          |  4 ++++
 kernel/sched/fair.c              |  8 ++++++--
 kernel/sched/rt.c                |  4 ++++
 5 files changed, 38 insertions(+), 6 deletions(-)

include/linux/sched/cpufreq.h +0 -0

index d1ad3d825561..6f6641e61236 100644

Quoted 1 line · Viresh Kumar

+#define SCHED_CPUFREQ_CLEAR (1U << 31)

I'm not thrilled by this, because schedutil is not the only user of the flags and it's totally unclear what the other user(s) should do when this is set.

Quoted 4 lines · Rafael J. Wysocki

Quoted 1 line · Viresh Kumar

+#define SCHED_CPUFREQ_CLEAR (1U << 31)

Quoted 3 lines

I'm not thrilled by this, because schedutil is not the only user of the flags and it's totally unclear what the other user(s) should do when this is set.

intel-pstate is the only other user of the IOWAIT flag, right? In order not to change the current behavior, we can update that to return early for now ?

We can do that in principle, but why should it return early? Maybe it's a good time to update things, incidentally?

I actually don't like the SCHED_CPUFRREQ_CLEAR flag *concept* as it is very much specific to schedutil and blatantly ignores everybody else.

Alternatively, you could add two flags for clearing SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL that could just be ingored entirely by intel_pstate.

So, why don't you make SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL "sticky" until, say, SCHED_CPUFREQ_NO_RT and SCHED_CPUFREQ_NO_DL are passed, respectively?

Quoted 9 lines · Rafael J. Wysocki

We can do that in principle, but why should it return early? Maybe it's a good time to update things, incidentally?

I actually don't like the SCHED_CPUFRREQ_CLEAR flag *concept* as it is very much specific to schedutil and blatantly ignores everybody else.

Alternatively, you could add two flags for clearing SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL that could just be ingored entirely by intel_pstate.

Quoted 2 lines

So, why don't you make SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL "sticky" until, say, SCHED_CPUFREQ_NO_RT and SCHED_CPUFREQ_NO_DL are passed, respectively?

I didn't like adding scheduling class specific flags, and wanted the code to treat all of them in the same way. And then the governors can make a policy over that, on what to ignore and what not to. For example with the current patchset, the governors can know when nothing else is queued on a CPU and CPU is going to get into idle loop. They can choose to (or not to) do something in that case.

Well, if SCHED_CPUFRREQ_CLEAR means "this CPU is going to enter the idle loop" really, then it is better to call it SCHED_CPUFRREQ_ENTER_IDLE, for example.

SCHED_CPUFRREQ_CLEAR meaning basically "you should clear these flags now" doesn't seem to convey any information to whoever doesn't squirrel the flags in the first place.

Quoted 7 lines · Rafael J. Wysocki

Well, if SCHED_CPUFRREQ_CLEAR means "this CPU is going to enter the idle loop" really, then it is better to call it SCHED_CPUFRREQ_ENTER_IDLE, for example.

SCHED_CPUFRREQ_CLEAR meaning basically "you should clear these flags now" doesn't seem to convey any information to whoever doesn't squirrel the flags in the first place.

Right, but when all the flags are cleared, then we can infer that we are going to idle in the most probable case.

Anyway, I will implement RT and DL clear flags as you suggested in the next version.

For example, swithing from:

- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags))
+ void (*func)(struct update_util_data *data, u64 time,
+ unsigned int flags, bool set))

Where the additional boolean is actually used to define which operation we wanna perform on the flags?

Quoted 7 lines · Patrick Bellasi

For example, swithing from:

- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags))
+ void (*func)(struct update_util_data *data, u64 time,
+ unsigned int flags, bool set))

Quoted 2 lines

Where the additional boolean is actually used to define which operation we wanna perform on the flags?

The code will eventually have the same complexity or ugliness in both the cases. I would like to start with another flag for now and see if people prefer another parameter.

Hi Viresh,

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · Patrick Bellasi

For example, swithing from:

- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags))
+ void (*func)(struct update_util_data *data, u64 time,
+ unsigned int flags, bool set))

Where the additional boolean is actually used to define which operation we wanna perform on the flags?

Quoted 3 lines

The code will eventually have the same complexity or ugliness in both the cases. I would like to start with another flag for now and see if people prefer another parameter.

Though I think that will solve Rafael's concern of polluting the flags for something schedutil specific. I also feel adding extra callback parameter is cleaner than 2 new clear flags.

Quoted 18 lines · Joel Fernandes

Hi Viresh,

Quoted 14 lines · Viresh Kumar

Quoted 9 lines · Patrick Bellasi

For example, swithing from:

- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags))
+ void (*func)(struct update_util_data *data, u64 time,
+ unsigned int flags, bool set))

Where the additional boolean is actually used to define which operation we wanna perform on the flags?

The code will eventually have the same complexity or ugliness in both the cases. I would like to start with another flag for now and see if people prefer another parameter.

Quoted 3 lines

Though I think that will solve Rafael's concern of polluting the flags for something schedutil specific. I also feel adding extra callback parameter is cleaner than 2 new clear flags.

Okay, I will then wait for Rafael to come online and comment on what he would prefer before posting.

Quoted 23 lines · Viresh Kumar

Quoted 21 lines · Joel Fernandes

Hi Viresh,

Quoted 8 lines · Viresh Kumar

Quoted 7 lines · Patrick Bellasi

For example, swithing from:

- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags))
+ void (*func)(struct update_util_data *data, u64 time,
+ unsigned int flags, bool set))

Quoted 6 lines

Quoted 2 lines

Where the additional boolean is actually used to define which operation we wanna perform on the flags?

The code will eventually have the same complexity or ugliness in both the cases. I would like to start with another flag for now and see if people prefer another parameter.

Though I think that will solve Rafael's concern of polluting the flags for something schedutil specific. I also feel adding extra callback parameter is cleaner than 2 new clear flags.

Quoted 2 lines

Okay, I will then wait for Rafael to come online and comment on what he would prefer before posting.

I thought about it once more. If we decide eventually to add another parameter, then why isn't the approach that this patch takes better than that? i.e. Use the 31st bit of flags for clear bit ? We can remove setting/clearing flags for CFS, that's it.

Yes that's clean to me but then as Rafael said, the use of this flag will be too specific for schedutil-only sg_cpu->flags clearing purpose right?

Quoted 3 lines · Joel Fernandes

Yes that's clean to me but then as Rafael said, the use of this flag will be too specific for schedutil-only sg_cpu->flags clearing purpose right?

And so would be the extra parameter ?

Quoted 7 lines · Rafael J. Wysocki

Well, if SCHED_CPUFRREQ_CLEAR means "this CPU is going to enter the idle loop" really, then it is better to call it SCHED_CPUFRREQ_ENTER_IDLE, for example.

SCHED_CPUFRREQ_CLEAR meaning basically "you should clear these flags now" doesn't seem to convey any information to whoever doesn't squirrel the flags in the first place.

Right, but when all the flags are cleared, then we can infer that we are going to idle in the most probable case.

Anyway, I will implement RT and DL clear flags as you suggested in the next version.

Currently the schedutil governor overwrites the sg_cpu->flags field on every call to the utilization handler. It was pretty good as the initial implementation of utilization handlers, there are several drawbacks though.

The biggest drawback is that the sg_cpu->flags field doesn't always represent the correct type of tasks that are enqueued on a CPU's rq. For example, if a fair task is enqueued while a RT or DL task is running, we will overwrite the flags with value 0 and that may take the CPU to lower OPPs unintentionally. There can be other corner cases as well which we aren't aware of currently.

This patch changes the current implementation to keep track of all the task types that are currently enqueued to the CPUs rq. A new flag: CLEAR is introduced and is set by the scheduling classes when their last task is dequeued. When the CLEAR flag bit is set, the schedutil governor resets all the other flag bits that are present in the flags parameter. For now, the util update handlers return immediately if they were called to clear the flag.

Yeah, not happy about this either; we had code that did the right thing without this extra tracking I think.

Also, we can look at the rq state if we want to, we don't need to duplicate that state.

Quoted 2 lines · Peter Zijlstra

Yeah, not happy about this either; we had code that did the right thing without this extra tracking I think.

Sure, but how do you suggest we fix the problems we are facing with the current design? Patrick had a completely different proposal for solving those problems, which I didn't like very much. This patchset replaced these patches from Patrick:

Quoted 2 lines

Also, we can look at the rq state if we want to, we don't need to duplicate that state.

Well that also looks fine to me, and that would mean this:

- We remove SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL flags, but still call the utilization callbacks from RT and DL classes.

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

On Wed, Dec 20, 2017 at 09:34:46AM +0530, Viresh Kumar wrote:

Please use the normal link format:

https://lkml.kernel.org/r/$MSGID

Then I can find them without having to resort to a frigging browser thing.

I'll try and dig through the email I have.

Quoted 4 lines

Well that also looks fine to me, and that would mean this:

- We remove SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL flags, but still call the utilization callbacks from RT and DL classes.

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

And for RT all we need to know is if current is of that class, otherwise we don't care.

On 20-12-17, 09:31, Peter Zijlstra wrote:
What about this case: A CFS task is running currently and an RT task is enqueued.

- Is it always the case that the CFS task is preempted immediately and the CPU is given to RT task ? I was talking to Vincent earlier and he told me that for certain configurations the CFS task may keep running until the next tick.

- What if the CFS task has disabled preemption ?

- More corner cases like this ?

Quoted 8 lines · Viresh Kumar

Quoted 2 lines · Peter Zijlstra

Yeah, not happy about this either; we had code that did the right thing without this extra tracking I think.

Sure, but how do you suggest we fix the problems we are facing with the current design? Patrick had a completely different proposal for solving those problems, which I didn't like very much. This patchset replaced these patches from Patrick:

Please use the normal link format:

https://lkml.kernel.org/r/$MSGID

Then I can find them without having to resort to a frigging browser thing.

I'll try and dig through the email I have.

Quoted 3 lines

Quoted 2 lines

Also, we can look at the rq state if we want to, we don't need to duplicate that state.

Quoted 4 lines

Well that also looks fine to me, and that would mean this:

- We remove SCHED_CPUFREQ_RT and SCHED_CPUFREQ_DL flags, but still call the utilization callbacks from RT and DL classes.

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

And for RT all we need to know is if current is of that class, otherwise we don't care.

Quoted 2 lines · Peter Zijlstra

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

He recently reposted them here:

https://lkml.kernel.org/r/[email protected]

Quoted 9 lines

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

Quoted 2 lines

And for RT all we need to know is if current is of that class, otherwise we don't care.

So, this:

   https://marc.info/?i=20171130114723.29210-3-patrick.bellasi%40arm.com

was actually going in this direction, although still working on top of flags to not change the existing interface too much.

IMO, the advantage of flags is that they are a sort-of "pro-active" approach, where the scheduler notify sensible events to schedutil. But keep adding flags seems to overkilling to me too.

If we remove flags then we have to query the scheduler classes "on demand"... but, as Peter suggests, once we have DL bits Juri posted, the only issue if to know if an RT task is running.
This the patch above can be just good enough, with no flags at all and with just a check for current being RT (or DL for the time being).

Quoted 8 lines · Patrick Bellasi

Quoted 2 lines · Peter Zijlstra

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

He recently reposted them here:

https://lkml.kernel.org/r/[email protected]

Yeah, just found them and actually munged them into my queue; did all the modifications you suggested too. Lets see if it comes apart.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

Quoted 6 lines

Quoted 2 lines

And for RT all we need to know is if current is of that class, otherwise we don't care.

So, this:

   https://marc.info/?i=20171130114723.29210-3-patrick.bellasi%40arm.com

Right, I was actually looking for those patches, but I'm searching backwards and hit upon Juri's patches first.

Quoted 7 lines

was actually going in this direction, although still working on top of flags to not change the existing interface too much.

IMO, the advantage of flags is that they are a sort-of "pro-active" approach, where the scheduler notify sensible events to schedutil. But keep adding flags seems to overkilling to me too.

Quoted 5 lines

If we remove flags then we have to query the scheduler classes "on demand"... but, as Peter suggests, once we have DL bits Juri posted, the only issue if to know if an RT task is running.
This the patch above can be just good enough, with no flags at all and with just a check for current being RT (or DL for the time being).

Well, we still need flags for crap like IO-WAIT IIRC. That's sugov internal state and not something the scheduler actually already knows.

But let me continue searching for patches..

Ooh, I found patches from Brendan... should be very close to yours

though, going by that msgid you posted on Nov 30th and I'm now on Dec 1st, soooon... :-)

Quoted 39 lines · Peter Zijlstra

Quoted 8 lines · Patrick Bellasi

Quoted 2 lines · Peter Zijlstra

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

He recently reposted them here:

https://lkml.kernel.org/r/[email protected]

Yeah, just found them and actually munged them into my queue; did all the modifications you suggested too. Lets see if it comes apart.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

Quoted 6 lines

Quoted 2 lines

And for RT all we need to know is if current is of that class, otherwise we don't care.

So, this:

   https://marc.info/?i=20171130114723.29210-3-patrick.bellasi%40arm.com

Right, I was actually looking for those patches, but I'm searching backwards and hit upon Juri's patches first.

Quoted 7 lines

was actually going in this direction, although still working on top of flags to not change the existing interface too much.

IMO, the advantage of flags is that they are a sort-of "pro-active" approach, where the scheduler notify sensible events to schedutil. But keep adding flags seems to overkilling to me too.

Quoted 8 lines

Quoted 5 lines

If we remove flags then we have to query the scheduler classes "on demand"... but, as Peter suggests, once we have DL bits Juri posted, the only issue if to know if an RT task is running.
This the patch above can be just good enough, with no flags at all and with just a check for current being RT (or DL for the time being).

Well, we still need flags for crap like IO-WAIT IIRC. That's sugov internal state and not something the scheduler actually already knows.

Right, that flag is set from:

core.c::io_schedule_prepare()

for the current task, which is going to be dequeued soon.

Once it wakes up the next time, at enqueue time we trigger a boosting by passing schedutil that flag.

Thus, unless we are happy to delay the boosting until the task is actually picked for execution (don't think so), then we need to keep the flag and signal schedutil at enqueue time.

However, was wondering one thing: should't we already have a vruntime bonus for IO sleeping tasks? Because in that case, the task is likely to be on CPU quite soon... and thus, perhaps by removing the flag and moving the schedutil notification into core.c at the end of __schedule() should be working to detect both RT and FAIR::IOWAIT boosted tasks.

Quoted 62 lines · CET Patrick Bellasi

Quoted 39 lines · Peter Zijlstra

Quoted 8 lines · Patrick Bellasi

Quoted 2 lines · Peter Zijlstra

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

He recently reposted them here:

https://lkml.kernel.org/r/[email protected]

Yeah, just found them and actually munged them into my queue; did all the modifications you suggested too. Lets see if it comes apart.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

Quoted 6 lines

Quoted 2 lines

And for RT all we need to know is if current is of that class, otherwise we don't care.

So, this:

   https://marc.info/?i=20171130114723.29210-3-patrick.bellasi%40arm.com

Right, I was actually looking for those patches, but I'm searching backwards and hit upon Juri's patches first.

Quoted 7 lines

was actually going in this direction, although still working on top of flags to not change the existing interface too much.

IMO, the advantage of flags is that they are a sort-of "pro-active" approach, where the scheduler notify sensible events to schedutil. But keep adding flags seems to overkilling to me too.

Quoted 8 lines

Quoted 5 lines

If we remove flags then we have to query the scheduler classes "on demand"... but, as Peter suggests, once we have DL bits Juri posted, the only issue if to know if an RT task is running.
This the patch above can be just good enough, with no flags at all and with just a check for current being RT (or DL for the time being).

Well, we still need flags for crap like IO-WAIT IIRC. That's sugov internal state and not something the scheduler actually already knows.

Right, that flag is set from:

core.c::io_schedule_prepare()

for the current task, which is going to be dequeued soon.

Once it wakes up the next time, at enqueue time we trigger a boosting by passing schedutil that flag.

Thus, unless we are happy to delay the boosting until the task is actually picked for execution (don't think so), then we need to keep the flag and signal schedutil at enqueue time.

Quoted 6 lines

However, was wondering one thing: should't we already have a vruntime bonus for IO sleeping tasks? Because in that case, the task is likely to be on CPU quite soon... and thus, perhaps by removing the flag and moving the schedutil notification into core.c at the end of __schedule() should be working to detect both RT and FAIR::IOWAIT boosted tasks.

schedutil is not the only user of this flag.

Thanks,
Rafael

Quoted 5 lines · Patrick Bellasi

Quoted 2 lines · Peter Zijlstra

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

Quoted 3 lines

He recently reposted them here:

https://lkml.kernel.org/r/[email protected]

Yeah, just found them and actually munged them into my queue; did all the modifications you suggested too. Lets see if it comes apart.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

- From the utilization handler, we check runqueues of all three sched classes to see if they have some work pending (this can be done smartly by checking only RT first and skipping other checks if RT has some work).

No that's wrong. DL should provide a minimum required based on existing reservations, we can add the expected CFS average on top and request that.

Quoted 6 lines

Quoted 2 lines

And for RT all we need to know is if current is of that class, otherwise we don't care.

So, this:

   https://marc.info/?i=20171130114723.29210-3-patrick.bellasi%40arm.com

Right, I was actually looking for those patches, but I'm searching backwards and hit upon Juri's patches first.

Quoted 7 lines

was actually going in this direction, although still working on top of flags to not change the existing interface too much.

IMO, the advantage of flags is that they are a sort-of "pro-active" approach, where the scheduler notify sensible events to schedutil. But keep adding flags seems to overkilling to me too.

Quoted 5 lines

If we remove flags then we have to query the scheduler classes "on demand"... but, as Peter suggests, once we have DL bits Juri posted, the only issue if to know if an RT task is running.
This the patch above can be just good enough, with no flags at all and with just a check for current being RT (or DL for the time being).

Well, we still need flags for crap like IO-WAIT IIRC. That's sugov internal state and not something the scheduler actually already knows.

Quoted 2 lines

Well, we still need flags for crap like IO-WAIT IIRC. That's sugov internal state and not something the scheduler actually already knows.

Not only sugov to be precise, but yes.

Quoted 6 lines · CET Peter Zijlstra

Quoted 5 lines · Patrick Bellasi

Quoted 2 lines · Peter Zijlstra

Didn't juri have patches to make DL do something sane? But yes, I think those flags are part of the problem.

Quoted 6 lines

Quoted 3 lines

He recently reposted them here:

https://lkml.kernel.org/r/[email protected]

Yeah, just found them and actually munged them into my queue; did all the modifications you suggested too. Lets see if it comes apart.

Good, because I think that the Juri's patches should go in first.

Thanks Peter for taking the patches. I was actually waiting for the flag thing to be resolved to post again. :/

We are already passing sg_cpu as argument to sugov_set_iowait_boost() helper and the same can be used to retrieve the flags value. Get rid of the redundant argument.

Signed-off-by Viresh Kumar <[email protected]>

sugov_update_shared() may get called to clear the scheduling class flags and we would return immediately in that case. Calling sugov_get_util() in that case isn't going to be of any use then. Move invocation of sugov_get_util() after the clear flag is checked.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/sched/cpufreq_schedutil.c +0 -2

index 7edfdc59ee8f..b69c37c867fe 100644
@@ -349,8 +349,6 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	unsigned long util, max;
 	unsigned int next_f;
 
-	sugov_get_util(&util, &max, sg_cpu->cpu);
-
 	raw_spin_lock(&sg_policy->update_lock);
 
 	if (unlikely(flags & SCHED_CPUFREQ_CLEAR)) {

It seems that Patrick already posted basically the same change:

https://patchwork.kernel.org/patch/10084669/

Also, Cc-ing him for this.. I just noticed he wasn't Cc-ed to the series.

sugov_update_shared() may get called to clear the scheduling class flags and we would return immediately in that case. Calling sugov_get_util() in that case isn't going to be of any use then. Move invocation of sugov_get_util() after the clear flag is checked.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/sched/cpufreq_schedutil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/sched/cpufreq_schedutil.c +0 -2

index 7edfdc59ee8f..b69c37c867fe 100644
@@ -349,8 +349,6 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,
        unsigned long util, max;
        unsigned int next_f;
 
-       sugov_get_util(&util, &max, sg_cpu->cpu);
-
        raw_spin_lock(&sg_policy->update_lock);
 
        if (unlikely(flags & SCHED_CPUFREQ_CLEAR)) {