* [PATCH] sched/uclamp: Fix uclamp_tg_restrict() @ 2021-06-11 12:22 Qais Yousef 2021-06-16 3:06 ` Xuewen Yan 2021-06-16 17:09 ` Dietmar Eggemann 0 siblings, 2 replies; 5+ messages in thread From: Qais Yousef @ 2021-06-11 12:22 UTC (permalink / raw) To: Peter Zijlstra (Intel), Ingo Molnar Cc: Vincent Guittot, Dietmar Eggemann, Patrick Bellasi, Tejun Heo, Quentin Perret, Wei Wang, Yun Hsiang, Xuewen Yan, linux-kernel, Qais Yousef Now cpu.uclamp.min acts as a protection, we need to make sure that the uclamp request of the task is within the allowed range of the cgroup, that is it is clamp()'ed correctly by tg->uclamp[UCLAMP_MIN] and tg->uclamp[UCLAMP_MAX]. As reported by Xuewen [1] we can have some corner cases where there's inverstion between uclamp requested by task (p) and the uclamp values of the taskgroup it's attached to (tg). Following table demonstrates 2 corner cases: | p | tg | effective -----------+-----+------+----------- CASE 1 -----------+-----+------+----------- uclamp_min | 60% | 0% | 60% -----------+-----+------+----------- uclamp_max | 80% | 50% | 50% -----------+-----+------+----------- CASE 2 -----------+-----+------+----------- uclamp_min | 0% | 30% | 30% -----------+-----+------+----------- uclamp_max | 20% | 50% | 20% -----------+-----+------+----------- With this fix we get: | p | tg | effective -----------+-----+------+----------- CASE 1 -----------+-----+------+----------- uclamp_min | 60% | 0% | 50% -----------+-----+------+----------- uclamp_max | 80% | 50% | 50% -----------+-----+------+----------- CASE 2 -----------+-----+------+----------- uclamp_min | 0% | 30% | 30% -----------+-----+------+----------- uclamp_max | 20% | 50% | 30% -----------+-----+------+----------- Additionally uclamp_update_active_tasks() must now unconditionally update both UCLAMP_MIN/MAX because changing the tg's UCLAMP_MAX for instance could have an impact on the effective UCLAMP_MIN of the tasks. | p | tg | effective -----------+-----+------+----------- old -----------+-----+------+----------- uclamp_min | 60% | 0% | 50% -----------+-----+------+----------- uclamp_max | 80% | 50% | 50% -----------+-----+------+----------- *new* -----------+-----+------+----------- uclamp_min | 60% | 0% | *60%* -----------+-----+------+----------- uclamp_max | 80% |*70%* | *70%* -----------+-----+------+----------- [1] https://lore.kernel.org/lkml/CAB8ipk_a6VFNjiEnHRHkUMBKbA+qzPQvhtNjJ_YNzQhqV_o8Zw@mail.gmail.com/ Reported-by: Xuewen Yan <xuewen.yan94@gmail.com> Fixes: 0c18f2ecfcc2 ("sched/uclamp: Fix wrong implementation of cpu.uclamp.min") Signed-off-by: Qais Yousef <qais.yousef@arm.com> --- Xuewen, Yun, Wei If you can give this a spin and provide Tested-by that would be much appreciated. Thanks! kernel/sched/core.c | 43 +++++++++++++++---------------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 9e9a5be35cde..0318b00baa97 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1403,38 +1403,28 @@ static void uclamp_sync_util_min_rt_default(void) static inline struct uclamp_se uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id) { - struct uclamp_se uc_req = p->uclamp_req[clamp_id]; + /* Copy by value as we could modify it */ + struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; #ifdef CONFIG_UCLAMP_TASK_GROUP + unsigned int tg_min, tg_max, value; /* * Tasks in autogroups or root task group will be * restricted by system defaults. */ if (task_group_is_autogroup(task_group(p))) - return uc_req; + return uc_eff; if (task_group(p) == &root_task_group) - return uc_req; + return uc_eff; - switch (clamp_id) { - case UCLAMP_MIN: { - struct uclamp_se uc_min = task_group(p)->uclamp[clamp_id]; - if (uc_req.value < uc_min.value) - return uc_min; - break; - } - case UCLAMP_MAX: { - struct uclamp_se uc_max = task_group(p)->uclamp[clamp_id]; - if (uc_req.value > uc_max.value) - return uc_max; - break; - } - default: - WARN_ON_ONCE(1); - break; - } + tg_min = task_group(p)->uclamp[UCLAMP_MIN].value; + tg_max = task_group(p)->uclamp[UCLAMP_MAX].value; + value = uc_eff.value; + value = clamp(value, tg_min, tg_max); + uclamp_se_set(&uc_eff, value, false); #endif - return uc_req; + return uc_eff; } /* @@ -1661,8 +1651,7 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id) #ifdef CONFIG_UCLAMP_TASK_GROUP static inline void -uclamp_update_active_tasks(struct cgroup_subsys_state *css, - unsigned int clamps) +uclamp_update_active_tasks(struct cgroup_subsys_state *css) { enum uclamp_id clamp_id; struct css_task_iter it; @@ -1670,10 +1659,8 @@ uclamp_update_active_tasks(struct cgroup_subsys_state *css, css_task_iter_start(css, 0, &it); while ((p = css_task_iter_next(&it))) { - for_each_clamp_id(clamp_id) { - if ((0x1 << clamp_id) & clamps) - uclamp_update_active(p, clamp_id); - } + for_each_clamp_id(clamp_id) + uclamp_update_active(p, clamp_id); } css_task_iter_end(&it); } @@ -9626,7 +9613,7 @@ static void cpu_util_update_eff(struct cgroup_subsys_state *css) } /* Immediately update descendants RUNNABLE tasks */ - uclamp_update_active_tasks(css, clamps); + uclamp_update_active_tasks(css); } } -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] sched/uclamp: Fix uclamp_tg_restrict() 2021-06-11 12:22 [PATCH] sched/uclamp: Fix uclamp_tg_restrict() Qais Yousef @ 2021-06-16 3:06 ` Xuewen Yan 2021-06-17 8:58 ` Qais Yousef 2021-06-16 17:09 ` Dietmar Eggemann 1 sibling, 1 reply; 5+ messages in thread From: Xuewen Yan @ 2021-06-16 3:06 UTC (permalink / raw) To: Qais Yousef Cc: Peter Zijlstra (Intel), Ingo Molnar, Vincent Guittot, Dietmar Eggemann, Patrick Bellasi, Tejun Heo, Quentin Perret, Wei Wang, Yun Hsiang, linux-kernel Hi Qais Sorry for the late reply. I tested the patch on the "UNISOC T610" platform, and it looks fine. According to the two cases: case1: without patch: set task's clamp_min=614(60%), clamp_max=819(80%), the result as: [ 215.780435]c6 uclamp_test : pid=138,req_min=614,req_max=819,eff_min=614,eff_max=819 after set tg's cpu.uclamp.max = 50%,the result: [ 420.580443]c6 uclamp_test : pid=138,req_min=614,req_max=819,eff_min=614,eff_max=512 with patch: set task's clamp_min=614(60%), clamp_max=819(80%), the result as: [ 333.533878]c7 uclamp_test : pid=138,req_min=614,req_max=819,eff_min=614,eff_max=819 after set tg's cpu.uclamp.max = 50%,the result: [ 430.813789]c6 uclamp_test : pid=138,req_min=614,req_max=819,eff_min=512,eff_max=512 case2: without patch: set task's clamp_min=614(60%), clamp_max=819(80%), the result as: [ 169.700544]c0 uclamp_test : pid=137,req_min=0,req_max=209,eff_min=0,eff_max=209 after set tg's cpu.uclamp.min = 30%, tg's cpu.uclamp.max = 50%,the result: [ 246.500634]c7 uclamp_test : pid=137,req_min=0,req_max=209,eff_min=307,eff_max=209 with patch: set task's clamp_min=0(0%), clamp_max=209(20%), the result as: [ 169.700544]c0 uclamp_test : pid=137,req_min=0,req_max=209,eff_min=0,eff_max=209 after set tg's cpu.uclamp.min = 30%, tg's cpu.uclamp.max = 50%,the result: [ 179.933868]c6 uclamp_test : pid=137,req_min=0,req_max=209,eff_min=307,eff_max=307 --- Cheers xuewen On Fri, Jun 11, 2021 at 8:23 PM Qais Yousef <qais.yousef@arm.com> wrote: > > Now cpu.uclamp.min acts as a protection, we need to make sure that the > uclamp request of the task is within the allowed range of the cgroup, > that is it is clamp()'ed correctly by tg->uclamp[UCLAMP_MIN] and > tg->uclamp[UCLAMP_MAX]. > > As reported by Xuewen [1] we can have some corner cases where there's > inverstion between uclamp requested by task (p) and the uclamp values of > the taskgroup it's attached to (tg). Following table demonstrates > 2 corner cases: > > | p | tg | effective > -----------+-----+------+----------- > CASE 1 > -----------+-----+------+----------- > uclamp_min | 60% | 0% | 60% > -----------+-----+------+----------- > uclamp_max | 80% | 50% | 50% > -----------+-----+------+----------- > CASE 2 > -----------+-----+------+----------- > uclamp_min | 0% | 30% | 30% > -----------+-----+------+----------- > uclamp_max | 20% | 50% | 20% > -----------+-----+------+----------- > > With this fix we get: > > | p | tg | effective > -----------+-----+------+----------- > CASE 1 > -----------+-----+------+----------- > uclamp_min | 60% | 0% | 50% > -----------+-----+------+----------- > uclamp_max | 80% | 50% | 50% > -----------+-----+------+----------- > CASE 2 > -----------+-----+------+----------- > uclamp_min | 0% | 30% | 30% > -----------+-----+------+----------- > uclamp_max | 20% | 50% | 30% > -----------+-----+------+----------- > > Additionally uclamp_update_active_tasks() must now unconditionally > update both UCLAMP_MIN/MAX because changing the tg's UCLAMP_MAX for > instance could have an impact on the effective UCLAMP_MIN of the tasks. > > | p | tg | effective > -----------+-----+------+----------- > old > -----------+-----+------+----------- > uclamp_min | 60% | 0% | 50% > -----------+-----+------+----------- > uclamp_max | 80% | 50% | 50% > -----------+-----+------+----------- > *new* > -----------+-----+------+----------- > uclamp_min | 60% | 0% | *60%* > -----------+-----+------+----------- > uclamp_max | 80% |*70%* | *70%* > -----------+-----+------+----------- > > [1] https://lore.kernel.org/lkml/CAB8ipk_a6VFNjiEnHRHkUMBKbA+qzPQvhtNjJ_YNzQhqV_o8Zw@mail.gmail.com/ > > Reported-by: Xuewen Yan <xuewen.yan94@gmail.com> > Fixes: 0c18f2ecfcc2 ("sched/uclamp: Fix wrong implementation of cpu.uclamp.min") > Signed-off-by: Qais Yousef <qais.yousef@arm.com> > --- > > Xuewen, Yun, Wei > > If you can give this a spin and provide Tested-by that would be much > appreciated. > > Thanks! > > > kernel/sched/core.c | 43 +++++++++++++++---------------------------- > 1 file changed, 15 insertions(+), 28 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 9e9a5be35cde..0318b00baa97 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -1403,38 +1403,28 @@ static void uclamp_sync_util_min_rt_default(void) > static inline struct uclamp_se > uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id) > { > - struct uclamp_se uc_req = p->uclamp_req[clamp_id]; > + /* Copy by value as we could modify it */ > + struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; > #ifdef CONFIG_UCLAMP_TASK_GROUP > + unsigned int tg_min, tg_max, value; > > /* > * Tasks in autogroups or root task group will be > * restricted by system defaults. > */ > if (task_group_is_autogroup(task_group(p))) > - return uc_req; > + return uc_eff; > if (task_group(p) == &root_task_group) > - return uc_req; > + return uc_eff; > > - switch (clamp_id) { > - case UCLAMP_MIN: { > - struct uclamp_se uc_min = task_group(p)->uclamp[clamp_id]; > - if (uc_req.value < uc_min.value) > - return uc_min; > - break; > - } > - case UCLAMP_MAX: { > - struct uclamp_se uc_max = task_group(p)->uclamp[clamp_id]; > - if (uc_req.value > uc_max.value) > - return uc_max; > - break; > - } > - default: > - WARN_ON_ONCE(1); > - break; > - } > + tg_min = task_group(p)->uclamp[UCLAMP_MIN].value; > + tg_max = task_group(p)->uclamp[UCLAMP_MAX].value; > + value = uc_eff.value; > + value = clamp(value, tg_min, tg_max); > + uclamp_se_set(&uc_eff, value, false); > #endif > > - return uc_req; > + return uc_eff; > } > > /* > @@ -1661,8 +1651,7 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id) > > #ifdef CONFIG_UCLAMP_TASK_GROUP > static inline void > -uclamp_update_active_tasks(struct cgroup_subsys_state *css, > - unsigned int clamps) > +uclamp_update_active_tasks(struct cgroup_subsys_state *css) > { > enum uclamp_id clamp_id; > struct css_task_iter it; > @@ -1670,10 +1659,8 @@ uclamp_update_active_tasks(struct cgroup_subsys_state *css, > > css_task_iter_start(css, 0, &it); > while ((p = css_task_iter_next(&it))) { > - for_each_clamp_id(clamp_id) { > - if ((0x1 << clamp_id) & clamps) > - uclamp_update_active(p, clamp_id); > - } > + for_each_clamp_id(clamp_id) > + uclamp_update_active(p, clamp_id); > } > css_task_iter_end(&it); > } > @@ -9626,7 +9613,7 @@ static void cpu_util_update_eff(struct cgroup_subsys_state *css) > } > > /* Immediately update descendants RUNNABLE tasks */ > - uclamp_update_active_tasks(css, clamps); > + uclamp_update_active_tasks(css); > } > } > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched/uclamp: Fix uclamp_tg_restrict() 2021-06-16 3:06 ` Xuewen Yan @ 2021-06-17 8:58 ` Qais Yousef 0 siblings, 0 replies; 5+ messages in thread From: Qais Yousef @ 2021-06-17 8:58 UTC (permalink / raw) To: Xuewen Yan Cc: Peter Zijlstra (Intel), Ingo Molnar, Vincent Guittot, Dietmar Eggemann, Patrick Bellasi, Tejun Heo, Quentin Perret, Wei Wang, Yun Hsiang, linux-kernel Hi Xuewen On 06/16/21 11:06, Xuewen Yan wrote: > Hi Qais > > Sorry for the late reply. > > I tested the patch on the "UNISOC T610" platform, and it looks fine. > > According to the two cases: > case1: > without patch: > set task's clamp_min=614(60%), clamp_max=819(80%), the result as: > [ 215.780435]c6 uclamp_test : > pid=138,req_min=614,req_max=819,eff_min=614,eff_max=819 > > after set tg's cpu.uclamp.max = 50%,the result: > [ 420.580443]c6 uclamp_test : > pid=138,req_min=614,req_max=819,eff_min=614,eff_max=512 > > with patch: > set task's clamp_min=614(60%), clamp_max=819(80%), the result as: > [ 333.533878]c7 uclamp_test : > pid=138,req_min=614,req_max=819,eff_min=614,eff_max=819 > > after set tg's cpu.uclamp.max = 50%,the result: > [ 430.813789]c6 uclamp_test : > pid=138,req_min=614,req_max=819,eff_min=512,eff_max=512 > > case2: > without patch: > set task's clamp_min=614(60%), clamp_max=819(80%), the result as: > [ 169.700544]c0 uclamp_test : > pid=137,req_min=0,req_max=209,eff_min=0,eff_max=209 > > after set tg's cpu.uclamp.min = 30%, tg's cpu.uclamp.max = 50%,the result: > [ 246.500634]c7 uclamp_test : > pid=137,req_min=0,req_max=209,eff_min=307,eff_max=209 > > with patch: > set task's clamp_min=0(0%), clamp_max=209(20%), the result as: > [ 169.700544]c0 uclamp_test : > pid=137,req_min=0,req_max=209,eff_min=0,eff_max=209 > > after set tg's cpu.uclamp.min = 30%, tg's cpu.uclamp.max = 50%,the result: > [ 179.933868]c6 uclamp_test : > pid=137,req_min=0,req_max=209,eff_min=307,eff_max=307 Thanks a lot for trying it out. Cheers -- Qais Yousef ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched/uclamp: Fix uclamp_tg_restrict() 2021-06-11 12:22 [PATCH] sched/uclamp: Fix uclamp_tg_restrict() Qais Yousef 2021-06-16 3:06 ` Xuewen Yan @ 2021-06-16 17:09 ` Dietmar Eggemann 2021-06-17 9:02 ` Qais Yousef 1 sibling, 1 reply; 5+ messages in thread From: Dietmar Eggemann @ 2021-06-16 17:09 UTC (permalink / raw) To: Qais Yousef, Peter Zijlstra (Intel), Ingo Molnar Cc: Vincent Guittot, Patrick Bellasi, Tejun Heo, Quentin Perret, Wei Wang, Yun Hsiang, Xuewen Yan, linux-kernel On 11/06/2021 14:22, Qais Yousef wrote: > Now cpu.uclamp.min acts as a protection, we need to make sure that the > uclamp request of the task is within the allowed range of the cgroup, > that is it is clamp()'ed correctly by tg->uclamp[UCLAMP_MIN] and > tg->uclamp[UCLAMP_MAX]. > > As reported by Xuewen [1] we can have some corner cases where there's > inverstion between uclamp requested by task (p) and the uclamp values of s/inverstion/inversion [...] > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 9e9a5be35cde..0318b00baa97 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -1403,38 +1403,28 @@ static void uclamp_sync_util_min_rt_default(void) > static inline struct uclamp_se > uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id) > { > - struct uclamp_se uc_req = p->uclamp_req[clamp_id]; > + /* Copy by value as we could modify it */ > + struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; > #ifdef CONFIG_UCLAMP_TASK_GROUP > + unsigned int tg_min, tg_max, value; > > /* > * Tasks in autogroups or root task group will be > * restricted by system defaults. > */ > if (task_group_is_autogroup(task_group(p))) > - return uc_req; > + return uc_eff; > if (task_group(p) == &root_task_group) > - return uc_req; > + return uc_eff; > > - switch (clamp_id) { > - case UCLAMP_MIN: { > - struct uclamp_se uc_min = task_group(p)->uclamp[clamp_id]; > - if (uc_req.value < uc_min.value) > - return uc_min; > - break; > - } > - case UCLAMP_MAX: { > - struct uclamp_se uc_max = task_group(p)->uclamp[clamp_id]; > - if (uc_req.value > uc_max.value) > - return uc_max; > - break; > - } > - default: > - WARN_ON_ONCE(1); > - break; > - } > + tg_min = task_group(p)->uclamp[UCLAMP_MIN].value; > + tg_max = task_group(p)->uclamp[UCLAMP_MAX].value; > + value = uc_eff.value; > + value = clamp(value, tg_min, tg_max); > + uclamp_se_set(&uc_eff, value, false); > #endif > > - return uc_req; > + return uc_eff; > } I got confused by the renaming uc_req -> uc_eff. We have: uclamp_eff_value() (1) uclamp_se uc_eff = uclamp_eff_get(p, clamp_id); (2) uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id) (3) struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; .... (3) is now calling it uc_eff where (2) still uses uc_req for the return of (3). IMHO uc_*eff* was used after the system level ( uclamp_default) have been applied. [...] > @@ -1670,10 +1659,8 @@ uclamp_update_active_tasks(struct cgroup_subsys_state *css, > > css_task_iter_start(css, 0, &it); > while ((p = css_task_iter_next(&it))) { > - for_each_clamp_id(clamp_id) { > - if ((0x1 << clamp_id) & clamps) > - uclamp_update_active(p, clamp_id); > - } > + for_each_clamp_id(clamp_id) > + uclamp_update_active(p, clamp_id); > } > css_task_iter_end(&it); > } > @@ -9626,7 +9613,7 @@ static void cpu_util_update_eff(struct cgroup_subsys_state *css) > } > > /* Immediately update descendants RUNNABLE tasks */ > - uclamp_update_active_tasks(css, clamps); > + uclamp_update_active_tasks(css); Since we now always have to update both clamp_id's, can you not update both under the same task_rq_lock() (in uclamp_update_active())? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sched/uclamp: Fix uclamp_tg_restrict() 2021-06-16 17:09 ` Dietmar Eggemann @ 2021-06-17 9:02 ` Qais Yousef 0 siblings, 0 replies; 5+ messages in thread From: Qais Yousef @ 2021-06-17 9:02 UTC (permalink / raw) To: Dietmar Eggemann Cc: Peter Zijlstra (Intel), Ingo Molnar, Vincent Guittot, Patrick Bellasi, Tejun Heo, Quentin Perret, Wei Wang, Yun Hsiang, Xuewen Yan, linux-kernel On 06/16/21 19:09, Dietmar Eggemann wrote: > On 11/06/2021 14:22, Qais Yousef wrote: > > Now cpu.uclamp.min acts as a protection, we need to make sure that the > > uclamp request of the task is within the allowed range of the cgroup, > > that is it is clamp()'ed correctly by tg->uclamp[UCLAMP_MIN] and > > tg->uclamp[UCLAMP_MAX]. > > > > As reported by Xuewen [1] we can have some corner cases where there's > > inverstion between uclamp requested by task (p) and the uclamp values of > > s/inverstion/inversion Fixed. > > [...] > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 9e9a5be35cde..0318b00baa97 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -1403,38 +1403,28 @@ static void uclamp_sync_util_min_rt_default(void) > > static inline struct uclamp_se > > uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id) > > { > > - struct uclamp_se uc_req = p->uclamp_req[clamp_id]; > > + /* Copy by value as we could modify it */ > > + struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; > > #ifdef CONFIG_UCLAMP_TASK_GROUP > > + unsigned int tg_min, tg_max, value; > > > > /* > > * Tasks in autogroups or root task group will be > > * restricted by system defaults. > > */ > > if (task_group_is_autogroup(task_group(p))) > > - return uc_req; > > + return uc_eff; > > if (task_group(p) == &root_task_group) > > - return uc_req; > > + return uc_eff; > > > > - switch (clamp_id) { > > - case UCLAMP_MIN: { > > - struct uclamp_se uc_min = task_group(p)->uclamp[clamp_id]; > > - if (uc_req.value < uc_min.value) > > - return uc_min; > > - break; > > - } > > - case UCLAMP_MAX: { > > - struct uclamp_se uc_max = task_group(p)->uclamp[clamp_id]; > > - if (uc_req.value > uc_max.value) > > - return uc_max; > > - break; > > - } > > - default: > > - WARN_ON_ONCE(1); > > - break; > > - } > > + tg_min = task_group(p)->uclamp[UCLAMP_MIN].value; > > + tg_max = task_group(p)->uclamp[UCLAMP_MAX].value; > > + value = uc_eff.value; > > + value = clamp(value, tg_min, tg_max); > > + uclamp_se_set(&uc_eff, value, false); > > #endif > > > > - return uc_req; > > + return uc_eff; > > } > > I got confused by the renaming uc_req -> uc_eff. > > We have: > > uclamp_eff_value() (1) > > uclamp_se uc_eff = uclamp_eff_get(p, clamp_id); (2) > > uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id) (3) > > struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; > .... > > (3) is now calling it uc_eff where (2) still uses uc_req for the return > of (3). IMHO uc_*eff* was used after the system level ( > uclamp_default) have been applied. Renamed it back to uc_req. > > [...] > > > @@ -1670,10 +1659,8 @@ uclamp_update_active_tasks(struct cgroup_subsys_state *css, > > > > css_task_iter_start(css, 0, &it); > > while ((p = css_task_iter_next(&it))) { > > - for_each_clamp_id(clamp_id) { > > - if ((0x1 << clamp_id) & clamps) > > - uclamp_update_active(p, clamp_id); > > - } > > + for_each_clamp_id(clamp_id) > > + uclamp_update_active(p, clamp_id); > > } > > css_task_iter_end(&it); > > } > > @@ -9626,7 +9613,7 @@ static void cpu_util_update_eff(struct cgroup_subsys_state *css) > > } > > > > /* Immediately update descendants RUNNABLE tasks */ > > - uclamp_update_active_tasks(css, clamps); > > + uclamp_update_active_tasks(css); > > Since we now always have to update both clamp_id's, can you not update > both under the same task_rq_lock() (in uclamp_update_active())? Good idea. Done this --->8--- diff --git a/kernel/sched/core.c b/kernel/sched/core.c index b4e856a4335d..fdb9a109fd68 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1620,8 +1620,9 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p) } static inline void -uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id) +uclamp_update_active(struct task_struct *p) { + enum uclamp_id clamp_id; struct rq_flags rf; struct rq *rq; @@ -1641,9 +1642,11 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id) * affecting a valid clamp bucket, the next time it's enqueued, * it will already see the updated clamp bucket value. */ - if (p->uclamp[clamp_id].active) { - uclamp_rq_dec_id(rq, p, clamp_id); - uclamp_rq_inc_id(rq, p, clamp_id); + for_each_clamp_id(clamp_id) { + if (p->uclamp[clamp_id].active) { + uclamp_rq_dec_id(rq, p, clamp_id); + uclamp_rq_inc_id(rq, p, clamp_id); + } } task_rq_unlock(rq, p, &rf); @@ -1653,15 +1656,12 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id) static inline void uclamp_update_active_tasks(struct cgroup_subsys_state *css) { - enum uclamp_id clamp_id; struct css_task_iter it; struct task_struct *p; css_task_iter_start(css, 0, &it); - while ((p = css_task_iter_next(&it))) { - for_each_clamp_id(clamp_id) - uclamp_update_active(p, clamp_id); - } + while ((p = css_task_iter_next(&it))) + uclamp_update_active(p); css_task_iter_end(&it); } --->8--- Thanks! -- Qais Yousef ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-17 9:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-11 12:22 [PATCH] sched/uclamp: Fix uclamp_tg_restrict() Qais Yousef 2021-06-16 3:06 ` Xuewen Yan 2021-06-17 8:58 ` Qais Yousef 2021-06-16 17:09 ` Dietmar Eggemann 2021-06-17 9:02 ` Qais Yousef
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.