[bug report] sched/fair: Prefer prev cpu in asymmetric wakeup path

[bug report] sched/fair: Prefer prev cpu in asymmetric wakeup path

[Dan Carpenter]

Hello Vincent Guittot,

The patch b4c9c9f15649: "sched/fair: Prefer prev cpu in asymmetric
wakeup path" from Oct 29, 2020, leads to the following static checker
warning:

	kernel/sched/fair.c:6249 select_idle_sibling()
	error: uninitialized symbol 'task_util'.

kernel/sched/fair.c
  6233  static int select_idle_sibling(struct task_struct *p, int prev, int target)
  6234  {
  6235          struct sched_domain *sd;
  6236          unsigned long task_util;
  6237          int i, recent_used_cpu;
  6238  
  6239          /*
  6240           * On asymmetric system, update task utilization because we will check
  6241           * that the task fits with cpu's capacity.
  6242           */

The original comment was a bit more clear...  Perhaps "On asymmetric
system[s], [record the] task utilization because we will check that the
task [can be done within] the cpu's capacity."

  6243          if (static_branch_unlikely(&sched_asym_cpucapacity)) {
  6244                  sync_entity_load_avg(&p->se);
  6245                  task_util = uclamp_task_util(p);
  6246          }

"task_util" is not initialized on the else path.

  6247  
  6248          if ((available_idle_cpu(target) || sched_idle_cpu(target)) &&
  6249              asym_fits_capacity(task_util, target))
                                       ^^^^^^^^^
Uninitialized variable warning.

  6250                  return target;
  6251  
  6252          /*
  6253           * If the previous CPU is cache affine and idle, don't be stupid:
  6254           */
  6255          if (prev != target && cpus_share_cache(prev, target) &&
  6256              (available_idle_cpu(prev) || sched_idle_cpu(prev)) &&
  6257              asym_fits_capacity(task_util, prev))
  6258                  return prev;
  6259  
  6260          /*
  6261           * Allow a per-cpu kthread to stack with the wakee if the

regards,
dan carpenter

Re: [bug report] sched/fair: Prefer prev cpu in asymmetric wakeup path

[Vincent Guittot]

Hi Dan,

Le vendredi 13 nov. 2020 à 11:46:57 (+0300), Dan Carpenter a écrit :
> Hello Vincent Guittot,
> 
> The patch b4c9c9f15649: "sched/fair: Prefer prev cpu in asymmetric
> wakeup path" from Oct 29, 2020, leads to the following static checker
> warning:
> 
> 	kernel/sched/fair.c:6249 select_idle_sibling()
> 	error: uninitialized symbol 'task_util'.
> 
> kernel/sched/fair.c
>   6233  static int select_idle_sibling(struct task_struct *p, int prev, int target)
>   6234  {
>   6235          struct sched_domain *sd;
>   6236          unsigned long task_util;
>   6237          int i, recent_used_cpu;
>   6238  
>   6239          /*
>   6240           * On asymmetric system, update task utilization because we will check
>   6241           * that the task fits with cpu's capacity.
>   6242           */
> 
> The original comment was a bit more clear...  Perhaps "On asymmetric
> system[s], [record the] task utilization because we will check that the
> task [can be done within] the cpu's capacity."

The comment "update task utilization because we will check ..." refers to
sync_entity_load_avg()

> 
>   6243          if (static_branch_unlikely(&sched_asym_cpucapacity)) {
>   6244                  sync_entity_load_avg(&p->se);
>   6245                  task_util = uclamp_task_util(p);
>   6246          }
> 
> "task_util" is not initialized on the else path.

no need because it will not be used

> 
>   6247  
>   6248          if ((available_idle_cpu(target) || sched_idle_cpu(target)) &&
>   6249              asym_fits_capacity(task_util, target))
>                                        ^^^^^^^^^
> Uninitialized variable warning.

asym_fits_capacity includes the same condition as above when we set task_util
so task_util can't be used unintialize

static inline bool asym_fits_capacity(int task_util, int cpu)
{
	if (static_branch_unlikely(&sched_asym_cpucapacity))
		return fits_capacity(task_util, capacity_of(cpu));

	return true;
}


> 
>   6250                  return target;
>   6251  
>   6252          /*
>   6253           * If the previous CPU is cache affine and idle, don't be stupid:
>   6254           */
>   6255          if (prev != target && cpus_share_cache(prev, target) &&
>   6256              (available_idle_cpu(prev) || sched_idle_cpu(prev)) &&
>   6257              asym_fits_capacity(task_util, prev))
>   6258                  return prev;
>   6259  
>   6260          /*
>   6261           * Allow a per-cpu kthread to stack with the wakee if the
> 
> regards,
> dan carpenter

Re: [bug report] sched/fair: Prefer prev cpu in asymmetric wakeup path

[Dan Carpenter]

On Fri, Nov 13, 2020 at 09:56:37AM +0100, Vincent Guittot wrote:
> Hi Dan,
> 
> Le vendredi 13 nov. 2020 à 11:46:57 (+0300), Dan Carpenter a écrit :
> > Hello Vincent Guittot,
> > 
> > The patch b4c9c9f15649: "sched/fair: Prefer prev cpu in asymmetric
> > wakeup path" from Oct 29, 2020, leads to the following static checker
> > warning:
> > 
> > 	kernel/sched/fair.c:6249 select_idle_sibling()
> > 	error: uninitialized symbol 'task_util'.
> > 
> > kernel/sched/fair.c
> >   6233  static int select_idle_sibling(struct task_struct *p, int prev, int target)
> >   6234  {
> >   6235          struct sched_domain *sd;
> >   6236          unsigned long task_util;
> >   6237          int i, recent_used_cpu;
> >   6238  
> >   6239          /*
> >   6240           * On asymmetric system, update task utilization because we will check
> >   6241           * that the task fits with cpu's capacity.
> >   6242           */
> > 
> > The original comment was a bit more clear...  Perhaps "On asymmetric
> > system[s], [record the] task utilization because we will check that the
> > task [can be done within] the cpu's capacity."
> 
> The comment "update task utilization because we will check ..." refers to
> sync_entity_load_avg()
> 
> > 
> >   6243          if (static_branch_unlikely(&sched_asym_cpucapacity)) {
> >   6244                  sync_entity_load_avg(&p->se);
> >   6245                  task_util = uclamp_task_util(p);
> >   6246          }
> > 
> > "task_util" is not initialized on the else path.
> 
> no need because it will not be used
> 
> > 
> >   6247  
> >   6248          if ((available_idle_cpu(target) || sched_idle_cpu(target)) &&
> >   6249              asym_fits_capacity(task_util, target))
> >                                        ^^^^^^^^^
> > Uninitialized variable warning.
> 
> asym_fits_capacity includes the same condition as above when we set task_util
> so task_util can't be used unintialize
> 
> static inline bool asym_fits_capacity(int task_util, int cpu)
> {
> 	if (static_branch_unlikely(&sched_asym_cpucapacity))
> 		return fits_capacity(task_util, capacity_of(cpu));
> 
> 	return true;

It's an interesting question, because unless the compiler makes this
inline, then it will lead to a KASan/syzbot warning at runtime.  The
function is, of course, marked as inline but the compiler, also of
course,  generally ignores those hints (use __always_inline if you want
the compiler to pay attention).  On the other hand, the compiler will
still probably inline it...  So this is *probably* not going to
lead to a runtime warning.

regards,
dan carpenter