Re: sched/fair: avoid using uninitialized variable in preferred_group_nid()

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Jan 23, 2015 at 08:25:38AM +0000, Jan Beulich wrote:
> At least some gcc versions - validly afaict - warn about potentially
> using max_group uninitialized: There's no way the compiler can prove
> that the body of the conditional where it and max_faults get set/
> updated gets executed; in fact, without knowing all the details of
> other scheduler code, I can't prove this either.
> 
> Generally the necessary change would appear to be to clear max_group
> prior to entering the inner loop, and break out of the outer loop when
> it ends up being all clear after the inner one. This, however, seems
> inefficient, and afaict the same effect can be achieved by exiting the
> outer loop when max_faults is still zero after the inner loop. For the
> compiler's sake, mark max_group uninitialized, as we're now able to
> prove it's not actually being used uninitalized anymore.


Yes this is somewhat challenging. What compiler version in specific did
you get this warning wiht? I cannot remember seeing it with whatever it
is I use (4.7-4.9 it seems).


> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Rik van Riel <riel@redhat.com>
> ---
>  kernel/sched/fair.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> --- 3.19-rc5/kernel/sched/fair.c
> +++ 3.19-rc5-sched-fair-preferred-group-nid/kernel/sched/fair.c
> @@ -1730,7 +1730,7 @@ static int preferred_group_nid(struct ta
>  	nodes = node_online_map;
>  	for (dist = sched_max_numa_distance; dist > LOCAL_DISTANCE; dist--) {
>  		unsigned long max_faults = 0;
> -		nodemask_t max_group;
> +		nodemask_t uninitialized_var(max_group);
>  		int a, b;
>  
>  		/* Are there nodes at this distance from each other? */
> @@ -1764,6 +1764,8 @@ static int preferred_group_nid(struct ta
>  			}
>  		}
>  		/* Next round, evaluate the nodes within max_group. */
> +		if (!max_faults)
> +			break;
>  		nodes = max_group;
>  	}
>  	return nid;

Yes I think you're right; there is no guarantee the faults sum will be
greater than 0, and therefore we might not trigger the assignment and
end up with uninitialized use.