[PATCH v2 1/2] sched/fair: Fix asym_fits_capacity() task_util type

[PATCH v2 1/2] sched/fair: Fix asym_fits_capacity() task_util type

[Vincent Donnefort]

task_util is an unsigned long value, compared with a CPU capacity which is
unsigned long as well. There's no need for an intermediate implicit int
cast.

Fixes: b4c9c9f15649 ("sched/fair: Prefer prev cpu in asymmetric wakeup path")
Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 945d987246c5..8fde6e10e24b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6346,7 +6346,7 @@ select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target)
 	return best_cpu;
 }
 
-static inline bool asym_fits_capacity(int task_util, int cpu)
+static inline bool asym_fits_capacity(unsigned long task_util, int cpu)
 {
 	if (static_branch_unlikely(&sched_asym_cpucapacity))
 		return fits_capacity(task_util, capacity_of(cpu));
-- 
2.25.1

[PATCH v2 2/2] sched/fair: Fix task_fits_capacity() capacity type

[Vincent Donnefort]

capacity is an unsigned long value, compared with a task utilization which
is unsigned long as well. There's no need for an intermediate implicit
long cast.

Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8fde6e10e24b..26a88975f68a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4070,7 +4070,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	trace_sched_util_est_se_tp(&p->se);
 }
 
-static inline int task_fits_capacity(struct task_struct *p, long capacity)
+static inline int task_fits_capacity(struct task_struct *p,
+				     unsigned long capacity)
 {
 	return fits_capacity(uclamp_task_util(p), capacity);
 }
-- 
2.25.1

Re: [PATCH v2 2/2] sched/fair: Fix task_fits_capacity() capacity type

[Vincent Guittot]

On Thu, 18 Nov 2021 at 15:15, Vincent Donnefort
<vincent.donnefort@arm.com> wrote:
>
> capacity is an unsigned long value, compared with a task utilization which
> is unsigned long as well. There's no need for an intermediate implicit
> long cast.
>
> Fixes: 3b1baa6496e6 ("sched/fair: Add 'group_misfit_task' load-balance type")
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8fde6e10e24b..26a88975f68a 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4070,7 +4070,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
>         trace_sched_util_est_se_tp(&p->se);
>  }
>
> -static inline int task_fits_capacity(struct task_struct *p, long capacity)
> +static inline int task_fits_capacity(struct task_struct *p,
> +                                    unsigned long capacity)
>  {
>         return fits_capacity(uclamp_task_util(p), capacity);
>  }
> --
> 2.25.1
>

Re: [PATCH v2 1/2] sched/fair: Fix asym_fits_capacity() task_util type

[Peter Zijlstra]

On Thu, Nov 18, 2021 at 02:14:10PM +0000, Vincent Donnefort wrote:
> task_util is an unsigned long value, compared with a CPU capacity which is
> unsigned long as well. There's no need for an intermediate implicit int
> cast.
> 
> Fixes: b4c9c9f15649 ("sched/fair: Prefer prev cpu in asymmetric wakeup path")

Do either of these patches actually *fix* anything? Afaict they're an
absolute no-op, even in terms of code-gen due to the promotion rules.

Yes, its arguably nicer to not rely on those implicit promotions etc..
but I don't think this warrants a Fixes tag or even being split in two
patches.

Hmm?

Re: [PATCH v2 1/2] sched/fair: Fix asym_fits_capacity() task_util type

[Vincent Donnefort]

On Mon, Dec 06, 2021 at 04:20:04PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 18, 2021 at 02:14:10PM +0000, Vincent Donnefort wrote:
> > task_util is an unsigned long value, compared with a CPU capacity which is
> > unsigned long as well. There's no need for an intermediate implicit int
> > cast.
> > 
> > Fixes: b4c9c9f15649 ("sched/fair: Prefer prev cpu in asymmetric wakeup path")
> 
> Do either of these patches actually *fix* anything? Afaict they're an
> absolute no-op, even in terms of code-gen due to the promotion rules.
> 
> Yes, its arguably nicer to not rely on those implicit promotions etc..
> but I don't think this warrants a Fixes tag or even being split in two
> patches.
> 
> Hmm?

As far as I know it doesn't change anything functionally speaking. So yeah,
let's consider it as a cleanup. I'll merge the changes and drop the Fix tag.