Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 16-Aug 12:34, Dietmar Eggemann wrote: > On 08/13/2018 05:01 PM, Patrick Bellasi wrote: > >On 13-Aug 16:06, Vincent Guittot wrote: > >>On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi > >>wrote: > >>>On 13-Aug 14:07, Vincent Guittot wrote: > On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi > wrote: > > > >[...] > > > >>>Yes I agree that the current behavior is not completely clean... still > >>>the question is: do you reckon the problem I depicted above, i.e. RT > >>>workloads eclipsing the min_util required by lower priority classes? > >> > >>As said above, I don't think that there is a problem that is specific > >>to cross class scheduling that can't also happen in the same class. > >> > >>Regarding your example: > >>task TA util=40% with uclamp_min 50% > >>task TB util=10% with uclamp_min 0% > >> > >>If TA and TB are cfs, util=50% and it doesn't seem to be a problem > >>whereas TB will steal some bandwidth to TA and delay it (and i even > >>don't speak about the impact of the nice priority of TB) > >>If TA is cfs and TB is rt, Why util=50% is now a problem for TA ? > > > >You right, in the current implementation, where we _do not_ > >distinguish among scheduling classes it's not possible to get a > >reasonable implementation of a per sched class clamping. > > > >>>To a certain extend I see this problem similar to the rt/dl/irq pressure > >>>in defining cpu_capacity, isn't it? > > > >However, I still think that higher priority classes eclipsing the > >clamping of lower priority classes can still be a problem. > > > >In your example above, the main difference between TA and TB being on > >the same class or different classes is that in the second case TB > >is granted to always preempt TA. We can end up with a non boosted RT > >task consuming all the boosted bandwidth required by a CFS task. > > > >This does not happen, apart maybe for the corner case of really > >different nice values, if the tasks are both CFS, since the fair > >scheduler will grant some progress for both of them. > > > >Thus, given the current implementation, I think it makes sense to drop > >the UCLAMP_SCHED_CLASS policy and stick with a more clean and > >consistent design. > > I agree with everything said in this thread so far. Cool! > So in case you skip UCLAMP_SCHED_CLASS [(B) combine the clamped > utilizations] in v4, you will only provide [A) clamp the combined > utilization]? Right... unless I find time to add support to per scheduling class tracking of clamps values. It should be relatively simple... but I guess it's also something we can keep as a really low prio and propose it once the main bits are not controversial anymore. > I assume that we don't have to guard the util clamping for rt tasks behind a > disabled by default sched feature because all runnable rt tasks will have > util_min = SCHED_CAPACITY_SCALE by default? So, that's what Quentin also proposed in a previous discussion: https://lore.kernel.org/lkml/2018080911.bp46sixk4u3ilcnh@queper01-lin/ but yes, you're right: it's in my todo list to ensure that by default RT tasks get a task-specific util_min set SCHED_CAPACITY_SCALE. > >I'll then see if it makes sense to add a dedicated patch on top of the > >series to add a proper per-class clamp tracking. > > I assume if you introduce this per-class clamping you will switch to use the > UCLAMP_SCHED_CLASS approach? Likely... but at that point we probably don't need the sched feature anymore and it could be just the default and unique aggregation policy. But let see when we will have the patches... and we don't necessarily need them for v4. Best, Patrick -- #include Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 16-Aug 12:34, Dietmar Eggemann wrote: > On 08/13/2018 05:01 PM, Patrick Bellasi wrote: > >On 13-Aug 16:06, Vincent Guittot wrote: > >>On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi > >>wrote: > >>>On 13-Aug 14:07, Vincent Guittot wrote: > On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi > wrote: > > > >[...] > > > >>>Yes I agree that the current behavior is not completely clean... still > >>>the question is: do you reckon the problem I depicted above, i.e. RT > >>>workloads eclipsing the min_util required by lower priority classes? > >> > >>As said above, I don't think that there is a problem that is specific > >>to cross class scheduling that can't also happen in the same class. > >> > >>Regarding your example: > >>task TA util=40% with uclamp_min 50% > >>task TB util=10% with uclamp_min 0% > >> > >>If TA and TB are cfs, util=50% and it doesn't seem to be a problem > >>whereas TB will steal some bandwidth to TA and delay it (and i even > >>don't speak about the impact of the nice priority of TB) > >>If TA is cfs and TB is rt, Why util=50% is now a problem for TA ? > > > >You right, in the current implementation, where we _do not_ > >distinguish among scheduling classes it's not possible to get a > >reasonable implementation of a per sched class clamping. > > > >>>To a certain extend I see this problem similar to the rt/dl/irq pressure > >>>in defining cpu_capacity, isn't it? > > > >However, I still think that higher priority classes eclipsing the > >clamping of lower priority classes can still be a problem. > > > >In your example above, the main difference between TA and TB being on > >the same class or different classes is that in the second case TB > >is granted to always preempt TA. We can end up with a non boosted RT > >task consuming all the boosted bandwidth required by a CFS task. > > > >This does not happen, apart maybe for the corner case of really > >different nice values, if the tasks are both CFS, since the fair > >scheduler will grant some progress for both of them. > > > >Thus, given the current implementation, I think it makes sense to drop > >the UCLAMP_SCHED_CLASS policy and stick with a more clean and > >consistent design. > > I agree with everything said in this thread so far. Cool! > So in case you skip UCLAMP_SCHED_CLASS [(B) combine the clamped > utilizations] in v4, you will only provide [A) clamp the combined > utilization]? Right... unless I find time to add support to per scheduling class tracking of clamps values. It should be relatively simple... but I guess it's also something we can keep as a really low prio and propose it once the main bits are not controversial anymore. > I assume that we don't have to guard the util clamping for rt tasks behind a > disabled by default sched feature because all runnable rt tasks will have > util_min = SCHED_CAPACITY_SCALE by default? So, that's what Quentin also proposed in a previous discussion: https://lore.kernel.org/lkml/2018080911.bp46sixk4u3ilcnh@queper01-lin/ but yes, you're right: it's in my todo list to ensure that by default RT tasks get a task-specific util_min set SCHED_CAPACITY_SCALE. > >I'll then see if it makes sense to add a dedicated patch on top of the > >series to add a proper per-class clamp tracking. > > I assume if you introduce this per-class clamping you will switch to use the > UCLAMP_SCHED_CLASS approach? Likely... but at that point we probably don't need the sched feature anymore and it could be just the default and unique aggregation policy. But let see when we will have the patches... and we don't necessarily need them for v4. Best, Patrick -- #include Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 08/13/2018 05:01 PM, Patrick Bellasi wrote: On 13-Aug 16:06, Vincent Guittot wrote: On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi wrote: On 13-Aug 14:07, Vincent Guittot wrote: On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote: [...] Yes I agree that the current behavior is not completely clean... still the question is: do you reckon the problem I depicted above, i.e. RT workloads eclipsing the min_util required by lower priority classes? As said above, I don't think that there is a problem that is specific to cross class scheduling that can't also happen in the same class. Regarding your example: task TA util=40% with uclamp_min 50% task TB util=10% with uclamp_min 0% If TA and TB are cfs, util=50% and it doesn't seem to be a problem whereas TB will steal some bandwidth to TA and delay it (and i even don't speak about the impact of the nice priority of TB) If TA is cfs and TB is rt, Why util=50% is now a problem for TA ? You right, in the current implementation, where we _do not_ distinguish among scheduling classes it's not possible to get a reasonable implementation of a per sched class clamping. To a certain extend I see this problem similar to the rt/dl/irq pressure in defining cpu_capacity, isn't it? However, I still think that higher priority classes eclipsing the clamping of lower priority classes can still be a problem. In your example above, the main difference between TA and TB being on the same class or different classes is that in the second case TB is granted to always preempt TA. We can end up with a non boosted RT task consuming all the boosted bandwidth required by a CFS task. This does not happen, apart maybe for the corner case of really different nice values, if the tasks are both CFS, since the fair scheduler will grant some progress for both of them. Thus, given the current implementation, I think it makes sense to drop the UCLAMP_SCHED_CLASS policy and stick with a more clean and consistent design. I agree with everything said in this thread so far. So in case you skip UCLAMP_SCHED_CLASS [(B) combine the clamped utilizations] in v4, you will only provide [A) clamp the combined utilization]? I assume that we don't have to guard the util clamping for rt tasks behind a disabled by default sched feature because all runnable rt tasks will have util_min = SCHED_CAPACITY_SCALE by default? I'll then see if it makes sense to add a dedicated patch on top of the series to add a proper per-class clamp tracking. I assume if you introduce this per-class clamping you will switch to use the UCLAMP_SCHED_CLASS approach?
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 08/13/2018 05:01 PM, Patrick Bellasi wrote: On 13-Aug 16:06, Vincent Guittot wrote: On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi wrote: On 13-Aug 14:07, Vincent Guittot wrote: On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote: [...] Yes I agree that the current behavior is not completely clean... still the question is: do you reckon the problem I depicted above, i.e. RT workloads eclipsing the min_util required by lower priority classes? As said above, I don't think that there is a problem that is specific to cross class scheduling that can't also happen in the same class. Regarding your example: task TA util=40% with uclamp_min 50% task TB util=10% with uclamp_min 0% If TA and TB are cfs, util=50% and it doesn't seem to be a problem whereas TB will steal some bandwidth to TA and delay it (and i even don't speak about the impact of the nice priority of TB) If TA is cfs and TB is rt, Why util=50% is now a problem for TA ? You right, in the current implementation, where we _do not_ distinguish among scheduling classes it's not possible to get a reasonable implementation of a per sched class clamping. To a certain extend I see this problem similar to the rt/dl/irq pressure in defining cpu_capacity, isn't it? However, I still think that higher priority classes eclipsing the clamping of lower priority classes can still be a problem. In your example above, the main difference between TA and TB being on the same class or different classes is that in the second case TB is granted to always preempt TA. We can end up with a non boosted RT task consuming all the boosted bandwidth required by a CFS task. This does not happen, apart maybe for the corner case of really different nice values, if the tasks are both CFS, since the fair scheduler will grant some progress for both of them. Thus, given the current implementation, I think it makes sense to drop the UCLAMP_SCHED_CLASS policy and stick with a more clean and consistent design. I agree with everything said in this thread so far. So in case you skip UCLAMP_SCHED_CLASS [(B) combine the clamped utilizations] in v4, you will only provide [A) clamp the combined utilization]? I assume that we don't have to guard the util clamping for rt tasks behind a disabled by default sched feature because all runnable rt tasks will have util_min = SCHED_CAPACITY_SCALE by default? I'll then see if it makes sense to add a dedicated patch on top of the series to add a proper per-class clamp tracking. I assume if you introduce this per-class clamping you will switch to use the UCLAMP_SCHED_CLASS approach?
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 13-Aug 16:06, Vincent Guittot wrote: > On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi wrote: > > On 13-Aug 14:07, Vincent Guittot wrote: > > > On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi > > > wrote: [...] > > Yes I agree that the current behavior is not completely clean... still > > the question is: do you reckon the problem I depicted above, i.e. RT > > workloads eclipsing the min_util required by lower priority classes? > > As said above, I don't think that there is a problem that is specific > to cross class scheduling that can't also happen in the same class. > > Regarding your example: > task TA util=40% with uclamp_min 50% > task TB util=10% with uclamp_min 0% > > If TA and TB are cfs, util=50% and it doesn't seem to be a problem > whereas TB will steal some bandwidth to TA and delay it (and i even > don't speak about the impact of the nice priority of TB) > If TA is cfs and TB is rt, Why util=50% is now a problem for TA ? You right, in the current implementation, where we _do not_ distinguish among scheduling classes it's not possible to get a reasonable implementation of a per sched class clamping. > > To a certain extend I see this problem similar to the rt/dl/irq pressure > > in defining cpu_capacity, isn't it? However, I still think that higher priority classes eclipsing the clamping of lower priority classes can still be a problem. In your example above, the main difference between TA and TB being on the same class or different classes is that in the second case TB is granted to always preempt TA. We can end up with a non boosted RT task consuming all the boosted bandwidth required by a CFS task. This does not happen, apart maybe for the corner case of really different nice values, if the tasks are both CFS, since the fair scheduler will grant some progress for both of them. Thus, given the current implementation, I think it makes sense to drop the UCLAMP_SCHED_CLASS policy and stick with a more clean and consistent design. I'll then see if it makes sense to add a dedicated patch on top of the series to add a proper per-class clamp tracking. Cheers Patrick -- #include Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 13-Aug 16:06, Vincent Guittot wrote: > On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi wrote: > > On 13-Aug 14:07, Vincent Guittot wrote: > > > On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi > > > wrote: [...] > > Yes I agree that the current behavior is not completely clean... still > > the question is: do you reckon the problem I depicted above, i.e. RT > > workloads eclipsing the min_util required by lower priority classes? > > As said above, I don't think that there is a problem that is specific > to cross class scheduling that can't also happen in the same class. > > Regarding your example: > task TA util=40% with uclamp_min 50% > task TB util=10% with uclamp_min 0% > > If TA and TB are cfs, util=50% and it doesn't seem to be a problem > whereas TB will steal some bandwidth to TA and delay it (and i even > don't speak about the impact of the nice priority of TB) > If TA is cfs and TB is rt, Why util=50% is now a problem for TA ? You right, in the current implementation, where we _do not_ distinguish among scheduling classes it's not possible to get a reasonable implementation of a per sched class clamping. > > To a certain extend I see this problem similar to the rt/dl/irq pressure > > in defining cpu_capacity, isn't it? However, I still think that higher priority classes eclipsing the clamping of lower priority classes can still be a problem. In your example above, the main difference between TA and TB being on the same class or different classes is that in the second case TB is granted to always preempt TA. We can end up with a non boosted RT task consuming all the boosted bandwidth required by a CFS task. This does not happen, apart maybe for the corner case of really different nice values, if the tasks are both CFS, since the fair scheduler will grant some progress for both of them. Thus, given the current implementation, I think it makes sense to drop the UCLAMP_SCHED_CLASS policy and stick with a more clean and consistent design. I'll then see if it makes sense to add a dedicated patch on top of the series to add a proper per-class clamp tracking. Cheers Patrick -- #include Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi wrote:
>
> On 13-Aug 14:07, Vincent Guittot wrote:
> > On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi
> > wrote:
> > >
> > > Hi Vincent!
> > >
> > > On 09-Aug 18:03, Vincent Guittot wrote:
> > > > > On 07-Aug 15:26, Juri Lelli wrote:
> > >
> > > [...]
> > >
> > > > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > > > + util_rt = cpu_util_rt(rq);
> > > > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > > > + util = 0;
> > > > > > > + if (util_cfs)
> > > > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > > > + if (util_rt)
> > > > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > > > + } else {
> > > > > > > + util = cpu_util_cfs(rq);
> > > > > > > + util += cpu_util_rt(rq);
> > > > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > > > + }
> > > > >
> > > > > Regarding the two policies, do you have any comment?
> > > >
> > > > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > > > make sense as it is ?
> > > > I mean, uclamp_util doesn't make any difference between rt and cfs
> > > > tasks when clamping the utilization so why should be add twice the
> > > > returned value ?
> > > > IMHO, this policy would make sense if there were something like
> > > > uclamp_util_rt() and a uclamp_util_cfs()
> > >
> > > The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> > > low-priority classese, especially when we have high RT utilization.
> > >
> > > Let say we have:
> > >
> > > util_rt = 40%, util_min=0%
> > > util_cfs = 10%, util_min=50%
> > >
> > > the two policies will select:
> > >
> > > UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> > > !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
> > >
> > > Which means that, despite the CPU's util_min will be set to 50% when
> > > CFS is running, these tasks will have almost no boost at all, since
> > > their bandwidth margin is eclipsed by RT tasks.
> >
> > Hmm ... At the opposite, even if there is no running rt task but only
> > some remaining blocked rt utilization,
> > even if util_rt = 10%, util_min=0%
> > and util_cfs = 40%, util_min=50%
> > the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
>
> Yes, that's true... since now I clamp util_rt if it's non zero.
> Perhaps this can be fixed by clamping util_rt only:
> if (rt_rq_is_runnable(>rt))
> ?
>
> > So cfs task can get double boosted by a small rt task.
>
> Well, in principle we don't know if the 50% clamp was asserted by the
> RT or the CFS task, since in the current implementation we max
> aggregate clamp values across all RT and CFS tasks.
Yes it was just the assumption of your example above.
IMHO, having util = 100% for your use case looks more like a bug than a feature
As you said below: "what utilization clamping aims to do is to defined
the minimum capacity to run _all_
the RUNNABLE tasks... not the minimum capacity for _each_ one of them "
>
> > Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
> > the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
>
> True, but here we are within the same class and what utilization
> clamping aims to do is to defined the minimum capacity to run _all_
> the RUNNABLE tasks... not the minimum capacity for _each_ one of them.
I fully agree and that's exactly what I want to highlight: With
UCLAMP_SCHED_CLASS policy, you try (but fail because the clamping is
not done per class) to distinguish rt and cfs as different kind of
runnable tasks.
>
> > So in this case cfs tasks don't get more boost and have to share the
> > bandwidth and you don't ensure 50% for each unlike what you try to do
> > for rt.
>
> Above I'm not trying to fix a per-task issue. The UCLAMP_SCHED_CLASS
> policy is just "trying" to fix a cross-class issue... if we agree
> there can be a cross-class issue worth to be fixed.
But the cross class issue that you are describing can also exists
between cfs tasks with different uclamp_min
So I'm not sure that's there is more cross-class issue than in class issue
>
> > You create a difference in the behavior depending of the class of the
> > others co-schedule tasks which is not sane IMHO
>
> Yes I agree that the current behavior is not completely clean... still
> the question is: do you reckon the problem I depicted above, i.e. RT
> workloads eclipsing the min_util required by lower priority classes?
As said above, I don't think that there is a problem that is specific
to cross class scheduling that can't also happen in the same class.
Regarding your example:
task TA util=40% with uclamp_min 50%
task TB util=10% with uclamp_min 0%
If TA and TB are cfs, util=50% and it doesn't seem to be a problem
whereas TB will steal some bandwidth to TA and delay it (and i even
don't speak about the impact of the nice priority of TB)
If TA is
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On Mon, 13 Aug 2018 at 14:49, Patrick Bellasi wrote:
>
> On 13-Aug 14:07, Vincent Guittot wrote:
> > On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi
> > wrote:
> > >
> > > Hi Vincent!
> > >
> > > On 09-Aug 18:03, Vincent Guittot wrote:
> > > > > On 07-Aug 15:26, Juri Lelli wrote:
> > >
> > > [...]
> > >
> > > > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > > > + util_rt = cpu_util_rt(rq);
> > > > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > > > + util = 0;
> > > > > > > + if (util_cfs)
> > > > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > > > + if (util_rt)
> > > > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > > > + } else {
> > > > > > > + util = cpu_util_cfs(rq);
> > > > > > > + util += cpu_util_rt(rq);
> > > > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > > > + }
> > > > >
> > > > > Regarding the two policies, do you have any comment?
> > > >
> > > > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > > > make sense as it is ?
> > > > I mean, uclamp_util doesn't make any difference between rt and cfs
> > > > tasks when clamping the utilization so why should be add twice the
> > > > returned value ?
> > > > IMHO, this policy would make sense if there were something like
> > > > uclamp_util_rt() and a uclamp_util_cfs()
> > >
> > > The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> > > low-priority classese, especially when we have high RT utilization.
> > >
> > > Let say we have:
> > >
> > > util_rt = 40%, util_min=0%
> > > util_cfs = 10%, util_min=50%
> > >
> > > the two policies will select:
> > >
> > > UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> > > !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
> > >
> > > Which means that, despite the CPU's util_min will be set to 50% when
> > > CFS is running, these tasks will have almost no boost at all, since
> > > their bandwidth margin is eclipsed by RT tasks.
> >
> > Hmm ... At the opposite, even if there is no running rt task but only
> > some remaining blocked rt utilization,
> > even if util_rt = 10%, util_min=0%
> > and util_cfs = 40%, util_min=50%
> > the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
>
> Yes, that's true... since now I clamp util_rt if it's non zero.
> Perhaps this can be fixed by clamping util_rt only:
> if (rt_rq_is_runnable(>rt))
> ?
>
> > So cfs task can get double boosted by a small rt task.
>
> Well, in principle we don't know if the 50% clamp was asserted by the
> RT or the CFS task, since in the current implementation we max
> aggregate clamp values across all RT and CFS tasks.
Yes it was just the assumption of your example above.
IMHO, having util = 100% for your use case looks more like a bug than a feature
As you said below: "what utilization clamping aims to do is to defined
the minimum capacity to run _all_
the RUNNABLE tasks... not the minimum capacity for _each_ one of them "
>
> > Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
> > the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
>
> True, but here we are within the same class and what utilization
> clamping aims to do is to defined the minimum capacity to run _all_
> the RUNNABLE tasks... not the minimum capacity for _each_ one of them.
I fully agree and that's exactly what I want to highlight: With
UCLAMP_SCHED_CLASS policy, you try (but fail because the clamping is
not done per class) to distinguish rt and cfs as different kind of
runnable tasks.
>
> > So in this case cfs tasks don't get more boost and have to share the
> > bandwidth and you don't ensure 50% for each unlike what you try to do
> > for rt.
>
> Above I'm not trying to fix a per-task issue. The UCLAMP_SCHED_CLASS
> policy is just "trying" to fix a cross-class issue... if we agree
> there can be a cross-class issue worth to be fixed.
But the cross class issue that you are describing can also exists
between cfs tasks with different uclamp_min
So I'm not sure that's there is more cross-class issue than in class issue
>
> > You create a difference in the behavior depending of the class of the
> > others co-schedule tasks which is not sane IMHO
>
> Yes I agree that the current behavior is not completely clean... still
> the question is: do you reckon the problem I depicted above, i.e. RT
> workloads eclipsing the min_util required by lower priority classes?
As said above, I don't think that there is a problem that is specific
to cross class scheduling that can't also happen in the same class.
Regarding your example:
task TA util=40% with uclamp_min 50%
task TB util=10% with uclamp_min 0%
If TA and TB are cfs, util=50% and it doesn't seem to be a problem
whereas TB will steal some bandwidth to TA and delay it (and i even
don't speak about the impact of the nice priority of TB)
If TA is
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 13-Aug 14:07, Vincent Guittot wrote:
> On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote:
> >
> > Hi Vincent!
> >
> > On 09-Aug 18:03, Vincent Guittot wrote:
> > > > On 07-Aug 15:26, Juri Lelli wrote:
> >
> > [...]
> >
> > > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > > + util_rt = cpu_util_rt(rq);
> > > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > > + util = 0;
> > > > > > + if (util_cfs)
> > > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > > + if (util_rt)
> > > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > > + } else {
> > > > > > + util = cpu_util_cfs(rq);
> > > > > > + util += cpu_util_rt(rq);
> > > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > > + }
> > > >
> > > > Regarding the two policies, do you have any comment?
> > >
> > > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > > make sense as it is ?
> > > I mean, uclamp_util doesn't make any difference between rt and cfs
> > > tasks when clamping the utilization so why should be add twice the
> > > returned value ?
> > > IMHO, this policy would make sense if there were something like
> > > uclamp_util_rt() and a uclamp_util_cfs()
> >
> > The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> > low-priority classese, especially when we have high RT utilization.
> >
> > Let say we have:
> >
> > util_rt = 40%, util_min=0%
> > util_cfs = 10%, util_min=50%
> >
> > the two policies will select:
> >
> > UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> > !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
> >
> > Which means that, despite the CPU's util_min will be set to 50% when
> > CFS is running, these tasks will have almost no boost at all, since
> > their bandwidth margin is eclipsed by RT tasks.
>
> Hmm ... At the opposite, even if there is no running rt task but only
> some remaining blocked rt utilization,
> even if util_rt = 10%, util_min=0%
> and util_cfs = 40%, util_min=50%
> the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
Yes, that's true... since now I clamp util_rt if it's non zero.
Perhaps this can be fixed by clamping util_rt only:
if (rt_rq_is_runnable(>rt))
?
> So cfs task can get double boosted by a small rt task.
Well, in principle we don't know if the 50% clamp was asserted by the
RT or the CFS task, since in the current implementation we max
aggregate clamp values across all RT and CFS tasks.
> Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
> the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
True, but here we are within the same class and what utilization
clamping aims to do is to defined the minimum capacity to run _all_
the RUNNABLE tasks... not the minimum capacity for _each_ one of them.
> So in this case cfs tasks don't get more boost and have to share the
> bandwidth and you don't ensure 50% for each unlike what you try to do
> for rt.
Above I'm not trying to fix a per-task issue. The UCLAMP_SCHED_CLASS
policy is just "trying" to fix a cross-class issue... if we agree
there can be a cross-class issue worth to be fixed.
> You create a difference in the behavior depending of the class of the
> others co-schedule tasks which is not sane IMHO
Yes I agree that the current behavior is not completely clean... still
the question is: do you reckon the problem I depicted above, i.e. RT
workloads eclipsing the min_util required by lower priority classes?
To a certain extend I see this problem similar to the rt/dl/irq pressure
in defining cpu_capacity, isn't it?
Maybe we can make use of (cpu_capacity_orig - cpu_capacity) to factor
in a util_min compensation for CFS tasks?
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 13-Aug 14:07, Vincent Guittot wrote:
> On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote:
> >
> > Hi Vincent!
> >
> > On 09-Aug 18:03, Vincent Guittot wrote:
> > > > On 07-Aug 15:26, Juri Lelli wrote:
> >
> > [...]
> >
> > > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > > + util_rt = cpu_util_rt(rq);
> > > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > > + util = 0;
> > > > > > + if (util_cfs)
> > > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > > + if (util_rt)
> > > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > > + } else {
> > > > > > + util = cpu_util_cfs(rq);
> > > > > > + util += cpu_util_rt(rq);
> > > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > > + }
> > > >
> > > > Regarding the two policies, do you have any comment?
> > >
> > > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > > make sense as it is ?
> > > I mean, uclamp_util doesn't make any difference between rt and cfs
> > > tasks when clamping the utilization so why should be add twice the
> > > returned value ?
> > > IMHO, this policy would make sense if there were something like
> > > uclamp_util_rt() and a uclamp_util_cfs()
> >
> > The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> > low-priority classese, especially when we have high RT utilization.
> >
> > Let say we have:
> >
> > util_rt = 40%, util_min=0%
> > util_cfs = 10%, util_min=50%
> >
> > the two policies will select:
> >
> > UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> > !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
> >
> > Which means that, despite the CPU's util_min will be set to 50% when
> > CFS is running, these tasks will have almost no boost at all, since
> > their bandwidth margin is eclipsed by RT tasks.
>
> Hmm ... At the opposite, even if there is no running rt task but only
> some remaining blocked rt utilization,
> even if util_rt = 10%, util_min=0%
> and util_cfs = 40%, util_min=50%
> the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
Yes, that's true... since now I clamp util_rt if it's non zero.
Perhaps this can be fixed by clamping util_rt only:
if (rt_rq_is_runnable(>rt))
?
> So cfs task can get double boosted by a small rt task.
Well, in principle we don't know if the 50% clamp was asserted by the
RT or the CFS task, since in the current implementation we max
aggregate clamp values across all RT and CFS tasks.
> Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
> the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
True, but here we are within the same class and what utilization
clamping aims to do is to defined the minimum capacity to run _all_
the RUNNABLE tasks... not the minimum capacity for _each_ one of them.
> So in this case cfs tasks don't get more boost and have to share the
> bandwidth and you don't ensure 50% for each unlike what you try to do
> for rt.
Above I'm not trying to fix a per-task issue. The UCLAMP_SCHED_CLASS
policy is just "trying" to fix a cross-class issue... if we agree
there can be a cross-class issue worth to be fixed.
> You create a difference in the behavior depending of the class of the
> others co-schedule tasks which is not sane IMHO
Yes I agree that the current behavior is not completely clean... still
the question is: do you reckon the problem I depicted above, i.e. RT
workloads eclipsing the min_util required by lower priority classes?
To a certain extend I see this problem similar to the rt/dl/irq pressure
in defining cpu_capacity, isn't it?
Maybe we can make use of (cpu_capacity_orig - cpu_capacity) to factor
in a util_min compensation for CFS tasks?
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On Mon, 13 Aug 2018 at 14:07, Vincent Guittot
wrote:
>
> On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote:
> >
> > Hi Vincent!
> >
> > On 09-Aug 18:03, Vincent Guittot wrote:
> > > > On 07-Aug 15:26, Juri Lelli wrote:
> >
> > [...]
> >
> > > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > > + util_rt = cpu_util_rt(rq);
> > > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > > + util = 0;
> > > > > > + if (util_cfs)
> > > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > > + if (util_rt)
> > > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > > + } else {
> > > > > > + util = cpu_util_cfs(rq);
> > > > > > + util += cpu_util_rt(rq);
> > > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > > + }
> > > >
> > > > Regarding the two policies, do you have any comment?
> > >
> > > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > > make sense as it is ?
> > > I mean, uclamp_util doesn't make any difference between rt and cfs
> > > tasks when clamping the utilization so why should be add twice the
> > > returned value ?
> > > IMHO, this policy would make sense if there were something like
> > > uclamp_util_rt() and a uclamp_util_cfs()
> >
> > The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> > low-priority classese, especially when we have high RT utilization.
> >
> > Let say we have:
> >
> > util_rt = 40%, util_min=0%
> > util_cfs = 10%, util_min=50%
> >
> > the two policies will select:
> >
> > UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> > !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
> >
> > Which means that, despite the CPU's util_min will be set to 50% when
> > CFS is running, these tasks will have almost no boost at all, since
> > their bandwidth margin is eclipsed by RT tasks.
>
> Hmm ... At the opposite, even if there is no running rt task but only
> some remaining blocked rt utilization,
> even if util_rt = 10%, util_min=0%
> and util_cfs = 40%, util_min=50%
> the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
>
> So cfs task can get double boosted by a small rt task.
>
> Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
> the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
s/uclamp(40)/uclamp(50)/
>
> So in this case cfs tasks don't get more boost and have to share the
> bandwidth and you don't ensure 50% for each unlike what you try to do
> for rt.
> You create a difference in the behavior depending of the class of the
> others co-schedule tasks which is not sane IMHO
>
>
> >
> > > > We had an internal discussion and we found pro/cons for both... but
> >
> > The UCLAMP_SCHED_CLASS policy is thus less energy efficiency but it
> > should grant a better "isolation" in terms of what is the expected
> > speed-up a task will get at run-time, independently from higher
> > priority classes.
> >
> > Does that make sense?
> >
> > > > I'm not sure keeping the sched_feat is a good solution on the long
> > > > run, i.e. mainline merge ;)
> >
> > This problem still stands...
> >
> > --
> > #include
> >
> > Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On Mon, 13 Aug 2018 at 14:07, Vincent Guittot
wrote:
>
> On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote:
> >
> > Hi Vincent!
> >
> > On 09-Aug 18:03, Vincent Guittot wrote:
> > > > On 07-Aug 15:26, Juri Lelli wrote:
> >
> > [...]
> >
> > > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > > + util_rt = cpu_util_rt(rq);
> > > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > > + util = 0;
> > > > > > + if (util_cfs)
> > > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > > + if (util_rt)
> > > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > > + } else {
> > > > > > + util = cpu_util_cfs(rq);
> > > > > > + util += cpu_util_rt(rq);
> > > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > > + }
> > > >
> > > > Regarding the two policies, do you have any comment?
> > >
> > > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > > make sense as it is ?
> > > I mean, uclamp_util doesn't make any difference between rt and cfs
> > > tasks when clamping the utilization so why should be add twice the
> > > returned value ?
> > > IMHO, this policy would make sense if there were something like
> > > uclamp_util_rt() and a uclamp_util_cfs()
> >
> > The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> > low-priority classese, especially when we have high RT utilization.
> >
> > Let say we have:
> >
> > util_rt = 40%, util_min=0%
> > util_cfs = 10%, util_min=50%
> >
> > the two policies will select:
> >
> > UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> > !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
> >
> > Which means that, despite the CPU's util_min will be set to 50% when
> > CFS is running, these tasks will have almost no boost at all, since
> > their bandwidth margin is eclipsed by RT tasks.
>
> Hmm ... At the opposite, even if there is no running rt task but only
> some remaining blocked rt utilization,
> even if util_rt = 10%, util_min=0%
> and util_cfs = 40%, util_min=50%
> the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
>
> So cfs task can get double boosted by a small rt task.
>
> Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
> the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
s/uclamp(40)/uclamp(50)/
>
> So in this case cfs tasks don't get more boost and have to share the
> bandwidth and you don't ensure 50% for each unlike what you try to do
> for rt.
> You create a difference in the behavior depending of the class of the
> others co-schedule tasks which is not sane IMHO
>
>
> >
> > > > We had an internal discussion and we found pro/cons for both... but
> >
> > The UCLAMP_SCHED_CLASS policy is thus less energy efficiency but it
> > should grant a better "isolation" in terms of what is the expected
> > speed-up a task will get at run-time, independently from higher
> > priority classes.
> >
> > Does that make sense?
> >
> > > > I'm not sure keeping the sched_feat is a good solution on the long
> > > > run, i.e. mainline merge ;)
> >
> > This problem still stands...
> >
> > --
> > #include
> >
> > Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote:
>
> Hi Vincent!
>
> On 09-Aug 18:03, Vincent Guittot wrote:
> > > On 07-Aug 15:26, Juri Lelli wrote:
>
> [...]
>
> > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > + util_rt = cpu_util_rt(rq);
> > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > + util = 0;
> > > > > + if (util_cfs)
> > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > + if (util_rt)
> > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > + } else {
> > > > > + util = cpu_util_cfs(rq);
> > > > > + util += cpu_util_rt(rq);
> > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > + }
> > >
> > > Regarding the two policies, do you have any comment?
> >
> > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > make sense as it is ?
> > I mean, uclamp_util doesn't make any difference between rt and cfs
> > tasks when clamping the utilization so why should be add twice the
> > returned value ?
> > IMHO, this policy would make sense if there were something like
> > uclamp_util_rt() and a uclamp_util_cfs()
>
> The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> low-priority classese, especially when we have high RT utilization.
>
> Let say we have:
>
> util_rt = 40%, util_min=0%
> util_cfs = 10%, util_min=50%
>
> the two policies will select:
>
> UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
>
> Which means that, despite the CPU's util_min will be set to 50% when
> CFS is running, these tasks will have almost no boost at all, since
> their bandwidth margin is eclipsed by RT tasks.
Hmm ... At the opposite, even if there is no running rt task but only
some remaining blocked rt utilization,
even if util_rt = 10%, util_min=0%
and util_cfs = 40%, util_min=50%
the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
So cfs task can get double boosted by a small rt task.
Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
So in this case cfs tasks don't get more boost and have to share the
bandwidth and you don't ensure 50% for each unlike what you try to do
for rt.
You create a difference in the behavior depending of the class of the
others co-schedule tasks which is not sane IMHO
>
> > > We had an internal discussion and we found pro/cons for both... but
>
> The UCLAMP_SCHED_CLASS policy is thus less energy efficiency but it
> should grant a better "isolation" in terms of what is the expected
> speed-up a task will get at run-time, independently from higher
> priority classes.
>
> Does that make sense?
>
> > > I'm not sure keeping the sched_feat is a good solution on the long
> > > run, i.e. mainline merge ;)
>
> This problem still stands...
>
> --
> #include
>
> Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On Mon, 13 Aug 2018 at 12:12, Patrick Bellasi wrote:
>
> Hi Vincent!
>
> On 09-Aug 18:03, Vincent Guittot wrote:
> > > On 07-Aug 15:26, Juri Lelli wrote:
>
> [...]
>
> > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > + util_rt = cpu_util_rt(rq);
> > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > + util = 0;
> > > > > + if (util_cfs)
> > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > + if (util_rt)
> > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > + } else {
> > > > > + util = cpu_util_cfs(rq);
> > > > > + util += cpu_util_rt(rq);
> > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > + }
> > >
> > > Regarding the two policies, do you have any comment?
> >
> > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > make sense as it is ?
> > I mean, uclamp_util doesn't make any difference between rt and cfs
> > tasks when clamping the utilization so why should be add twice the
> > returned value ?
> > IMHO, this policy would make sense if there were something like
> > uclamp_util_rt() and a uclamp_util_cfs()
>
> The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> low-priority classese, especially when we have high RT utilization.
>
> Let say we have:
>
> util_rt = 40%, util_min=0%
> util_cfs = 10%, util_min=50%
>
> the two policies will select:
>
> UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
>
> Which means that, despite the CPU's util_min will be set to 50% when
> CFS is running, these tasks will have almost no boost at all, since
> their bandwidth margin is eclipsed by RT tasks.
Hmm ... At the opposite, even if there is no running rt task but only
some remaining blocked rt utilization,
even if util_rt = 10%, util_min=0%
and util_cfs = 40%, util_min=50%
the UCLAMP_SCHED_CLASS: util = uclamp(10) + uclamp(40) = 50 + 50 = 100%
So cfs task can get double boosted by a small rt task.
Furthermore, if there is no rt task but 2 cfs tasks of 40% and 10%
the UCLAMP_SCHED_CLASS: util = uclamp(0) + uclamp(40) = 50 = 50%
So in this case cfs tasks don't get more boost and have to share the
bandwidth and you don't ensure 50% for each unlike what you try to do
for rt.
You create a difference in the behavior depending of the class of the
others co-schedule tasks which is not sane IMHO
>
> > > We had an internal discussion and we found pro/cons for both... but
>
> The UCLAMP_SCHED_CLASS policy is thus less energy efficiency but it
> should grant a better "isolation" in terms of what is the expected
> speed-up a task will get at run-time, independently from higher
> priority classes.
>
> Does that make sense?
>
> > > I'm not sure keeping the sched_feat is a good solution on the long
> > > run, i.e. mainline merge ;)
>
> This problem still stands...
>
> --
> #include
>
> Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 13/08/18 11:12, Patrick Bellasi wrote:
> Hi Vincent!
>
> On 09-Aug 18:03, Vincent Guittot wrote:
> > > On 07-Aug 15:26, Juri Lelli wrote:
>
> [...]
>
> > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > + util_rt = cpu_util_rt(rq);
> > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > + util = 0;
> > > > > + if (util_cfs)
> > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > + if (util_rt)
> > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > + } else {
> > > > > + util = cpu_util_cfs(rq);
> > > > > + util += cpu_util_rt(rq);
> > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > + }
> > >
> > > Regarding the two policies, do you have any comment?
> >
> > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > make sense as it is ?
> > I mean, uclamp_util doesn't make any difference between rt and cfs
> > tasks when clamping the utilization so why should be add twice the
> > returned value ?
> > IMHO, this policy would make sense if there were something like
> > uclamp_util_rt() and a uclamp_util_cfs()
>
> The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> low-priority classese, especially when we have high RT utilization.
>
> Let say we have:
>
> util_rt = 40%, util_min=0%
> util_cfs = 10%, util_min=50%
>
> the two policies will select:
>
> UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
>
> Which means that, despite the CPU's util_min will be set to 50% when
> CFS is running, these tasks will have almost no boost at all, since
> their bandwidth margin is eclipsed by RT tasks.
Ah, right. But isn't possible to distinguish between classes? I mean, if
you would know that only CFS is clamped (boosted) in this case, you
could have:
util = util_rt + uclamp(util_cfs) = 40 + 50 = 90%
Which should do what one expects w/o energy side effects?
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 13/08/18 11:12, Patrick Bellasi wrote:
> Hi Vincent!
>
> On 09-Aug 18:03, Vincent Guittot wrote:
> > > On 07-Aug 15:26, Juri Lelli wrote:
>
> [...]
>
> > > > > + util_cfs = cpu_util_cfs(rq);
> > > > > + util_rt = cpu_util_rt(rq);
> > > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > > + util = 0;
> > > > > + if (util_cfs)
> > > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > > + if (util_rt)
> > > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > > + } else {
> > > > > + util = cpu_util_cfs(rq);
> > > > > + util += cpu_util_rt(rq);
> > > > > + util = uclamp_util(cpu_of(rq), util);
> > > > > + }
> > >
> > > Regarding the two policies, do you have any comment?
> >
> > Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> > make sense as it is ?
> > I mean, uclamp_util doesn't make any difference between rt and cfs
> > tasks when clamping the utilization so why should be add twice the
> > returned value ?
> > IMHO, this policy would make sense if there were something like
> > uclamp_util_rt() and a uclamp_util_cfs()
>
> The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
> low-priority classese, especially when we have high RT utilization.
>
> Let say we have:
>
> util_rt = 40%, util_min=0%
> util_cfs = 10%, util_min=50%
>
> the two policies will select:
>
> UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
> !UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
>
> Which means that, despite the CPU's util_min will be set to 50% when
> CFS is running, these tasks will have almost no boost at all, since
> their bandwidth margin is eclipsed by RT tasks.
Ah, right. But isn't possible to distinguish between classes? I mean, if
you would know that only CFS is clamped (boosted) in this case, you
could have:
util = util_rt + uclamp(util_cfs) = 40 + 50 = 90%
Which should do what one expects w/o energy side effects?
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Quentin! On 09-Aug 16:55, Quentin Perret wrote: > Hi Patrick, > > On Thursday 09 Aug 2018 at 16:41:56 (+0100), Patrick Bellasi wrote: > > > IIUC, not far below this you should still have something like: > > > > > > if (rt_rq_is_runnable(>rt)) > > > return max; > > > > Do you mean that when RT tasks are RUNNABLE we still want to got to > > MAX? Not sure to understand... since this patch is actually to clamp > > the RT class... > > Argh, reading my message again it wasn't very clear indeed. Sorry about > that ... > > What I'm try to say is that your patch does _not_ remove the snippet of code > above from sugov_get_util(). So I think that when a RT task is runnable, > you will not reach the end of the function where the clamping is done. > And this is not what you want AFAICT. > > Does that make any sense ? Oh gotcha... you right, I've missed that bit when I rebased on tip. Will fix on the next iteration! -- #include Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Quentin! On 09-Aug 16:55, Quentin Perret wrote: > Hi Patrick, > > On Thursday 09 Aug 2018 at 16:41:56 (+0100), Patrick Bellasi wrote: > > > IIUC, not far below this you should still have something like: > > > > > > if (rt_rq_is_runnable(>rt)) > > > return max; > > > > Do you mean that when RT tasks are RUNNABLE we still want to got to > > MAX? Not sure to understand... since this patch is actually to clamp > > the RT class... > > Argh, reading my message again it wasn't very clear indeed. Sorry about > that ... > > What I'm try to say is that your patch does _not_ remove the snippet of code > above from sugov_get_util(). So I think that when a RT task is runnable, > you will not reach the end of the function where the clamping is done. > And this is not what you want AFAICT. > > Does that make any sense ? Oh gotcha... you right, I've missed that bit when I rebased on tip. Will fix on the next iteration! -- #include Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Vincent!
On 09-Aug 18:03, Vincent Guittot wrote:
> > On 07-Aug 15:26, Juri Lelli wrote:
[...]
> > > > + util_cfs = cpu_util_cfs(rq);
> > > > + util_rt = cpu_util_rt(rq);
> > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > + util = 0;
> > > > + if (util_cfs)
> > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > + if (util_rt)
> > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > + } else {
> > > > + util = cpu_util_cfs(rq);
> > > > + util += cpu_util_rt(rq);
> > > > + util = uclamp_util(cpu_of(rq), util);
> > > > + }
> >
> > Regarding the two policies, do you have any comment?
>
> Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> make sense as it is ?
> I mean, uclamp_util doesn't make any difference between rt and cfs
> tasks when clamping the utilization so why should be add twice the
> returned value ?
> IMHO, this policy would make sense if there were something like
> uclamp_util_rt() and a uclamp_util_cfs()
The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
low-priority classese, especially when we have high RT utilization.
Let say we have:
util_rt = 40%, util_min=0%
util_cfs = 10%, util_min=50%
the two policies will select:
UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
!UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
Which means that, despite the CPU's util_min will be set to 50% when
CFS is running, these tasks will have almost no boost at all, since
their bandwidth margin is eclipsed by RT tasks.
> > We had an internal discussion and we found pro/cons for both... but
The UCLAMP_SCHED_CLASS policy is thus less energy efficiency but it
should grant a better "isolation" in terms of what is the expected
speed-up a task will get at run-time, independently from higher
priority classes.
Does that make sense?
> > I'm not sure keeping the sched_feat is a good solution on the long
> > run, i.e. mainline merge ;)
This problem still stands...
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Vincent!
On 09-Aug 18:03, Vincent Guittot wrote:
> > On 07-Aug 15:26, Juri Lelli wrote:
[...]
> > > > + util_cfs = cpu_util_cfs(rq);
> > > > + util_rt = cpu_util_rt(rq);
> > > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > > + util = 0;
> > > > + if (util_cfs)
> > > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > > + if (util_rt)
> > > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > > + } else {
> > > > + util = cpu_util_cfs(rq);
> > > > + util += cpu_util_rt(rq);
> > > > + util = uclamp_util(cpu_of(rq), util);
> > > > + }
> >
> > Regarding the two policies, do you have any comment?
>
> Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
> make sense as it is ?
> I mean, uclamp_util doesn't make any difference between rt and cfs
> tasks when clamping the utilization so why should be add twice the
> returned value ?
> IMHO, this policy would make sense if there were something like
> uclamp_util_rt() and a uclamp_util_cfs()
The idea for the UCLAMP_SCHED_CLASS policy is to improve fairness on
low-priority classese, especially when we have high RT utilization.
Let say we have:
util_rt = 40%, util_min=0%
util_cfs = 10%, util_min=50%
the two policies will select:
UCLAMP_SCHED_CLASS: util = uclamp(40) + uclamp(10) = 50 + 50 = 100%
!UCLAMP_SCHED_CLASS: util = uclamp(40 + 10) = uclmp(50) = 50%
Which means that, despite the CPU's util_min will be set to 50% when
CFS is running, these tasks will have almost no boost at all, since
their bandwidth margin is eclipsed by RT tasks.
> > We had an internal discussion and we found pro/cons for both... but
The UCLAMP_SCHED_CLASS policy is thus less energy efficiency but it
should grant a better "isolation" in terms of what is the expected
speed-up a task will get at run-time, independently from higher
priority classes.
Does that make sense?
> > I'm not sure keeping the sched_feat is a good solution on the long
> > run, i.e. mainline merge ;)
This problem still stands...
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Patrick,
On Thu, 9 Aug 2018 at 17:34, Patrick Bellasi wrote:
>
> On 07-Aug 15:26, Juri Lelli wrote:
> > Hi,
> >
> > On 06/08/18 17:39, Patrick Bellasi wrote:
> >
> > [...]
> >
> > > @@ -223,13 +224,25 @@ static unsigned long sugov_get_util(struct
> > > sugov_cpu *sg_cpu)
> > > * utilization (PELT windows are synchronized) we can directly add
> > > them
> > > * to obtain the CPU's actual utilization.
> > > *
> > > -* CFS utilization can be boosted or capped, depending on utilization
> > > -* clamp constraints configured for currently RUNNABLE tasks.
> > > +* CFS and RT utilizations can be boosted or capped, depending on
> > > +* utilization constraints enforce by currently RUNNABLE tasks.
> > > +* They are individually clamped to ensure fairness across classes,
> > > +* meaning that CFS always gets (if possible) the (minimum) required
> > > +* bandwidth on top of that required by higher priority classes.
> >
> > Is this a stale comment written before UCLAMP_SCHED_CLASS was
> > introduced? It seems to apply to the below if branch only.
>
> Yes, you right... I'll update the comment.
>
> > > */
> > > - util = cpu_util_cfs(rq);
> > > - if (util)
> > > - util = uclamp_util(cpu_of(rq), util);
> > > - util += cpu_util_rt(rq);
> > > + util_cfs = cpu_util_cfs(rq);
> > > + util_rt = cpu_util_rt(rq);
> > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > + util = 0;
> > > + if (util_cfs)
> > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > + if (util_rt)
> > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > + } else {
> > > + util = cpu_util_cfs(rq);
> > > + util += cpu_util_rt(rq);
> > > + util = uclamp_util(cpu_of(rq), util);
> > > + }
>
> Regarding the two policies, do you have any comment?
Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
make sense as it is ?
I mean, uclamp_util doesn't make any difference between rt and cfs
tasks when clamping the utilization so why should be add twice the
returned value ?
IMHO, this policy would make sense if there were something like
uclamp_util_rt() and a uclamp_util_cfs()
>
> We had an internal discussion and we found pro/cons for both... but
> I'm not sure keeping the sched_feat is a good solution on the long
> run, i.e. mainline merge ;)
>
> --
> #include
>
> Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Patrick,
On Thu, 9 Aug 2018 at 17:34, Patrick Bellasi wrote:
>
> On 07-Aug 15:26, Juri Lelli wrote:
> > Hi,
> >
> > On 06/08/18 17:39, Patrick Bellasi wrote:
> >
> > [...]
> >
> > > @@ -223,13 +224,25 @@ static unsigned long sugov_get_util(struct
> > > sugov_cpu *sg_cpu)
> > > * utilization (PELT windows are synchronized) we can directly add
> > > them
> > > * to obtain the CPU's actual utilization.
> > > *
> > > -* CFS utilization can be boosted or capped, depending on utilization
> > > -* clamp constraints configured for currently RUNNABLE tasks.
> > > +* CFS and RT utilizations can be boosted or capped, depending on
> > > +* utilization constraints enforce by currently RUNNABLE tasks.
> > > +* They are individually clamped to ensure fairness across classes,
> > > +* meaning that CFS always gets (if possible) the (minimum) required
> > > +* bandwidth on top of that required by higher priority classes.
> >
> > Is this a stale comment written before UCLAMP_SCHED_CLASS was
> > introduced? It seems to apply to the below if branch only.
>
> Yes, you right... I'll update the comment.
>
> > > */
> > > - util = cpu_util_cfs(rq);
> > > - if (util)
> > > - util = uclamp_util(cpu_of(rq), util);
> > > - util += cpu_util_rt(rq);
> > > + util_cfs = cpu_util_cfs(rq);
> > > + util_rt = cpu_util_rt(rq);
> > > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > > + util = 0;
> > > + if (util_cfs)
> > > + util += uclamp_util(cpu_of(rq), util_cfs);
> > > + if (util_rt)
> > > + util += uclamp_util(cpu_of(rq), util_rt);
> > > + } else {
> > > + util = cpu_util_cfs(rq);
> > > + util += cpu_util_rt(rq);
> > > + util = uclamp_util(cpu_of(rq), util);
> > > + }
>
> Regarding the two policies, do you have any comment?
Does the policy for (sched_feat(UCLAMP_SCHED_CLASS)== true) really
make sense as it is ?
I mean, uclamp_util doesn't make any difference between rt and cfs
tasks when clamping the utilization so why should be add twice the
returned value ?
IMHO, this policy would make sense if there were something like
uclamp_util_rt() and a uclamp_util_cfs()
>
> We had an internal discussion and we found pro/cons for both... but
> I'm not sure keeping the sched_feat is a good solution on the long
> run, i.e. mainline merge ;)
>
> --
> #include
>
> Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Patrick, On Thursday 09 Aug 2018 at 16:41:56 (+0100), Patrick Bellasi wrote: > > IIUC, not far below this you should still have something like: > > > > if (rt_rq_is_runnable(>rt)) > > return max; > > Do you mean that when RT tasks are RUNNABLE we still want to got to > MAX? Not sure to understand... since this patch is actually to clamp > the RT class... Argh, reading my message again it wasn't very clear indeed. Sorry about that ... What I'm try to say is that your patch does _not_ remove the snippet of code above from sugov_get_util(). So I think that when a RT task is runnable, you will not reach the end of the function where the clamping is done. And this is not what you want AFAICT. Does that make any sense ? > > > So you won't reach the actual clamping code at the end of the function > > when a RT task is runnable no? > > ... mmm... no... this patch is to clamp RT tasks... Am I missing > something? > > > Also, I always had the feeling that the default for RT should be > > util_min == 1024, and then users could decide to lower the bar if they > > want to. > > Mmm... good point! This would keep the policy unaltered for RT tasks. > > I want to keep sched class specific code in uclamp at minimum but > likely this should be achievable by just properly initializing the > task-specific util_min for RT tasks, if the original task has > UCLAM_NOT_VALID. +1, it'd be nice to keep the cross-class mess to a minimum IMO. But hopefully this RT thing isn't too ugly to implement ... > > > For the specific case of RT, that feels more natural than > > applying a max util clamp IMO. What do you think ? > > I'll look better into this for the next posting! Sounds good :-) Thanks, Quentin
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Patrick, On Thursday 09 Aug 2018 at 16:41:56 (+0100), Patrick Bellasi wrote: > > IIUC, not far below this you should still have something like: > > > > if (rt_rq_is_runnable(>rt)) > > return max; > > Do you mean that when RT tasks are RUNNABLE we still want to got to > MAX? Not sure to understand... since this patch is actually to clamp > the RT class... Argh, reading my message again it wasn't very clear indeed. Sorry about that ... What I'm try to say is that your patch does _not_ remove the snippet of code above from sugov_get_util(). So I think that when a RT task is runnable, you will not reach the end of the function where the clamping is done. And this is not what you want AFAICT. Does that make any sense ? > > > So you won't reach the actual clamping code at the end of the function > > when a RT task is runnable no? > > ... mmm... no... this patch is to clamp RT tasks... Am I missing > something? > > > Also, I always had the feeling that the default for RT should be > > util_min == 1024, and then users could decide to lower the bar if they > > want to. > > Mmm... good point! This would keep the policy unaltered for RT tasks. > > I want to keep sched class specific code in uclamp at minimum but > likely this should be achievable by just properly initializing the > task-specific util_min for RT tasks, if the original task has > UCLAM_NOT_VALID. +1, it'd be nice to keep the cross-class mess to a minimum IMO. But hopefully this RT thing isn't too ugly to implement ... > > > For the specific case of RT, that feels more natural than > > applying a max util clamp IMO. What do you think ? > > I'll look better into this for the next posting! Sounds good :-) Thanks, Quentin
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 07-Aug 14:54, Quentin Perret wrote:
> Hi Patrick,
Hi Quentin!
> On Monday 06 Aug 2018 at 17:39:38 (+0100), Patrick Bellasi wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c
> > b/kernel/sched/cpufreq_schedutil.c
> > index a7affc729c25..bb25ef66c2d3 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -200,6 +200,7 @@ static unsigned int get_next_freq(struct sugov_policy
> > *sg_policy,
> > static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
> > {
> > struct rq *rq = cpu_rq(sg_cpu->cpu);
> > + unsigned long util_cfs, util_rt;
> > unsigned long util, irq, max;
> >
> > sg_cpu->max = max = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
>
> IIUC, not far below this you should still have something like:
>
> if (rt_rq_is_runnable(>rt))
> return max;
Do you mean that when RT tasks are RUNNABLE we still want to got to
MAX? Not sure to understand... since this patch is actually to clamp
the RT class...
> So you won't reach the actual clamping code at the end of the function
> when a RT task is runnable no?
... mmm... no... this patch is to clamp RT tasks... Am I missing
something?
> Also, I always had the feeling that the default for RT should be
> util_min == 1024, and then users could decide to lower the bar if they
> want to.
Mmm... good point! This would keep the policy unaltered for RT tasks.
I want to keep sched class specific code in uclamp at minimum, but
likely this should be achievable by just properly initializing the
task-specific util_min for RT tasks, if the original task has
UCLAM_NOT_VALID.
> For the specific case of RT, that feels more natural than
> applying a max util clamp IMO. What do you think ?
I'll look better into this for the next posting!
Cheers Patrick
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 07-Aug 14:54, Quentin Perret wrote:
> Hi Patrick,
Hi Quentin!
> On Monday 06 Aug 2018 at 17:39:38 (+0100), Patrick Bellasi wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c
> > b/kernel/sched/cpufreq_schedutil.c
> > index a7affc729c25..bb25ef66c2d3 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -200,6 +200,7 @@ static unsigned int get_next_freq(struct sugov_policy
> > *sg_policy,
> > static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
> > {
> > struct rq *rq = cpu_rq(sg_cpu->cpu);
> > + unsigned long util_cfs, util_rt;
> > unsigned long util, irq, max;
> >
> > sg_cpu->max = max = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
>
> IIUC, not far below this you should still have something like:
>
> if (rt_rq_is_runnable(>rt))
> return max;
Do you mean that when RT tasks are RUNNABLE we still want to got to
MAX? Not sure to understand... since this patch is actually to clamp
the RT class...
> So you won't reach the actual clamping code at the end of the function
> when a RT task is runnable no?
... mmm... no... this patch is to clamp RT tasks... Am I missing
something?
> Also, I always had the feeling that the default for RT should be
> util_min == 1024, and then users could decide to lower the bar if they
> want to.
Mmm... good point! This would keep the policy unaltered for RT tasks.
I want to keep sched class specific code in uclamp at minimum, but
likely this should be achievable by just properly initializing the
task-specific util_min for RT tasks, if the original task has
UCLAM_NOT_VALID.
> For the specific case of RT, that feels more natural than
> applying a max util clamp IMO. What do you think ?
I'll look better into this for the next posting!
Cheers Patrick
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 07-Aug 15:26, Juri Lelli wrote:
> Hi,
>
> On 06/08/18 17:39, Patrick Bellasi wrote:
>
> [...]
>
> > @@ -223,13 +224,25 @@ static unsigned long sugov_get_util(struct sugov_cpu
> > *sg_cpu)
> > * utilization (PELT windows are synchronized) we can directly add them
> > * to obtain the CPU's actual utilization.
> > *
> > -* CFS utilization can be boosted or capped, depending on utilization
> > -* clamp constraints configured for currently RUNNABLE tasks.
> > +* CFS and RT utilizations can be boosted or capped, depending on
> > +* utilization constraints enforce by currently RUNNABLE tasks.
> > +* They are individually clamped to ensure fairness across classes,
> > +* meaning that CFS always gets (if possible) the (minimum) required
> > +* bandwidth on top of that required by higher priority classes.
>
> Is this a stale comment written before UCLAMP_SCHED_CLASS was
> introduced? It seems to apply to the below if branch only.
Yes, you right... I'll update the comment.
> > */
> > - util = cpu_util_cfs(rq);
> > - if (util)
> > - util = uclamp_util(cpu_of(rq), util);
> > - util += cpu_util_rt(rq);
> > + util_cfs = cpu_util_cfs(rq);
> > + util_rt = cpu_util_rt(rq);
> > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > + util = 0;
> > + if (util_cfs)
> > + util += uclamp_util(cpu_of(rq), util_cfs);
> > + if (util_rt)
> > + util += uclamp_util(cpu_of(rq), util_rt);
> > + } else {
> > + util = cpu_util_cfs(rq);
> > + util += cpu_util_rt(rq);
> > + util = uclamp_util(cpu_of(rq), util);
> > + }
Regarding the two policies, do you have any comment?
We had an internal discussion and we found pro/cons for both... but
I'm not sure keeping the sched_feat is a good solution on the long
run, i.e. mainline merge ;)
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
On 07-Aug 15:26, Juri Lelli wrote:
> Hi,
>
> On 06/08/18 17:39, Patrick Bellasi wrote:
>
> [...]
>
> > @@ -223,13 +224,25 @@ static unsigned long sugov_get_util(struct sugov_cpu
> > *sg_cpu)
> > * utilization (PELT windows are synchronized) we can directly add them
> > * to obtain the CPU's actual utilization.
> > *
> > -* CFS utilization can be boosted or capped, depending on utilization
> > -* clamp constraints configured for currently RUNNABLE tasks.
> > +* CFS and RT utilizations can be boosted or capped, depending on
> > +* utilization constraints enforce by currently RUNNABLE tasks.
> > +* They are individually clamped to ensure fairness across classes,
> > +* meaning that CFS always gets (if possible) the (minimum) required
> > +* bandwidth on top of that required by higher priority classes.
>
> Is this a stale comment written before UCLAMP_SCHED_CLASS was
> introduced? It seems to apply to the below if branch only.
Yes, you right... I'll update the comment.
> > */
> > - util = cpu_util_cfs(rq);
> > - if (util)
> > - util = uclamp_util(cpu_of(rq), util);
> > - util += cpu_util_rt(rq);
> > + util_cfs = cpu_util_cfs(rq);
> > + util_rt = cpu_util_rt(rq);
> > + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> > + util = 0;
> > + if (util_cfs)
> > + util += uclamp_util(cpu_of(rq), util_cfs);
> > + if (util_rt)
> > + util += uclamp_util(cpu_of(rq), util_rt);
> > + } else {
> > + util = cpu_util_cfs(rq);
> > + util += cpu_util_rt(rq);
> > + util = uclamp_util(cpu_of(rq), util);
> > + }
Regarding the two policies, do you have any comment?
We had an internal discussion and we found pro/cons for both... but
I'm not sure keeping the sched_feat is a good solution on the long
run, i.e. mainline merge ;)
--
#include
Patrick Bellasi
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Patrick,
On Monday 06 Aug 2018 at 17:39:38 (+0100), Patrick Bellasi wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index a7affc729c25..bb25ef66c2d3 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -200,6 +200,7 @@ static unsigned int get_next_freq(struct sugov_policy
> *sg_policy,
> static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
> {
> struct rq *rq = cpu_rq(sg_cpu->cpu);
> + unsigned long util_cfs, util_rt;
> unsigned long util, irq, max;
>
> sg_cpu->max = max = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
IIUC, not far below this you should still have something like:
if (rt_rq_is_runnable(>rt))
return max;
So you won't reach the actual clamping code at the end of the function
when a RT task is runnable no?
Also, I always had the feeling that the default for RT should be
util_min == 1024, and then users could decide to lower the bar if they
want to. For the specific case of RT, that feels more natural than
applying a max util clamp IMO. What do you think ?
Thanks,
Quentin
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi Patrick,
On Monday 06 Aug 2018 at 17:39:38 (+0100), Patrick Bellasi wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index a7affc729c25..bb25ef66c2d3 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -200,6 +200,7 @@ static unsigned int get_next_freq(struct sugov_policy
> *sg_policy,
> static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
> {
> struct rq *rq = cpu_rq(sg_cpu->cpu);
> + unsigned long util_cfs, util_rt;
> unsigned long util, irq, max;
>
> sg_cpu->max = max = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
IIUC, not far below this you should still have something like:
if (rt_rq_is_runnable(>rt))
return max;
So you won't reach the actual clamping code at the end of the function
when a RT task is runnable no?
Also, I always had the feeling that the default for RT should be
util_min == 1024, and then users could decide to lower the bar if they
want to. For the specific case of RT, that feels more natural than
applying a max util clamp IMO. What do you think ?
Thanks,
Quentin
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi,
On 06/08/18 17:39, Patrick Bellasi wrote:
[...]
> @@ -223,13 +224,25 @@ static unsigned long sugov_get_util(struct sugov_cpu
> *sg_cpu)
>* utilization (PELT windows are synchronized) we can directly add them
>* to obtain the CPU's actual utilization.
>*
> - * CFS utilization can be boosted or capped, depending on utilization
> - * clamp constraints configured for currently RUNNABLE tasks.
> + * CFS and RT utilizations can be boosted or capped, depending on
> + * utilization constraints enforce by currently RUNNABLE tasks.
> + * They are individually clamped to ensure fairness across classes,
> + * meaning that CFS always gets (if possible) the (minimum) required
> + * bandwidth on top of that required by higher priority classes.
Is this a stale comment written before UCLAMP_SCHED_CLASS was
introduced? It seems to apply to the below if branch only.
>*/
> - util = cpu_util_cfs(rq);
> - if (util)
> - util = uclamp_util(cpu_of(rq), util);
> - util += cpu_util_rt(rq);
> + util_cfs = cpu_util_cfs(rq);
> + util_rt = cpu_util_rt(rq);
> + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> + util = 0;
> + if (util_cfs)
> + util += uclamp_util(cpu_of(rq), util_cfs);
> + if (util_rt)
> + util += uclamp_util(cpu_of(rq), util_rt);
> + } else {
> + util = cpu_util_cfs(rq);
> + util += cpu_util_rt(rq);
> + util = uclamp_util(cpu_of(rq), util);
> + }
Best,
- Juri
Re: [PATCH v3 06/14] sched/cpufreq: uclamp: add utilization clamping for RT tasks
Hi,
On 06/08/18 17:39, Patrick Bellasi wrote:
[...]
> @@ -223,13 +224,25 @@ static unsigned long sugov_get_util(struct sugov_cpu
> *sg_cpu)
>* utilization (PELT windows are synchronized) we can directly add them
>* to obtain the CPU's actual utilization.
>*
> - * CFS utilization can be boosted or capped, depending on utilization
> - * clamp constraints configured for currently RUNNABLE tasks.
> + * CFS and RT utilizations can be boosted or capped, depending on
> + * utilization constraints enforce by currently RUNNABLE tasks.
> + * They are individually clamped to ensure fairness across classes,
> + * meaning that CFS always gets (if possible) the (minimum) required
> + * bandwidth on top of that required by higher priority classes.
Is this a stale comment written before UCLAMP_SCHED_CLASS was
introduced? It seems to apply to the below if branch only.
>*/
> - util = cpu_util_cfs(rq);
> - if (util)
> - util = uclamp_util(cpu_of(rq), util);
> - util += cpu_util_rt(rq);
> + util_cfs = cpu_util_cfs(rq);
> + util_rt = cpu_util_rt(rq);
> + if (sched_feat(UCLAMP_SCHED_CLASS)) {
> + util = 0;
> + if (util_cfs)
> + util += uclamp_util(cpu_of(rq), util_cfs);
> + if (util_rt)
> + util += uclamp_util(cpu_of(rq), util_rt);
> + } else {
> + util = cpu_util_cfs(rq);
> + util += cpu_util_rt(rq);
> + util = uclamp_util(cpu_of(rq), util);
> + }
Best,
- Juri
