Re: [bug report] mm/mempolicy: skip extra call to __alloc_pages_bulk in weighted interleave

[Joshua Hahn <joshua.hahnjy@gmail.com>]

+ Cc Kees, who also reported this yesterday!

On Tue, 1 Jul 2025 13:07:53 -0500 Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hello Joshua Hahn,
> 
> Commit 72cf498b9e8a ("mm/mempolicy: skip extra call to
> __alloc_pages_bulk in weighted interleave") from Jun 26, 2025
> (linux-next), leads to the following Smatch static checker warning:
> 
> 	mm/mempolicy.c:2641 alloc_pages_bulk_weighted_interleave()
> 	error: uninitialized symbol 'i'.

[...snip...]

>     2593         /* Continue allocating from most recent node and adjust the nr_pages */
>     2594         node = me->il_prev;
>     2595         weight = me->il_weight;
>     2596         if (weight && node_isset(node, nodes)) {
>     2597                 if (rem_pages <= weight) {
>     2598                         node_pages = rem_pages;
>     2599                         me->il_weight -= node_pages;
>     2600                         goto allocate;
> 
> i is not initialized here.
> 
>     2601                 }
>     2602                 carryover = weight;
>     2603         }
>     2604         /* clear active weight in case of an allocation failure */
>     2605         me->il_weight = 0;
>     2606         prev_node = node;
>     2607 
>     2608         /* create a local copy of node weights to operate on outside rcu */
>     2609         weights = kzalloc(nr_node_ids, GFP_KERNEL);
>     2610         if (!weights)
>     2611                 return 0;
>     2612 
>     2613         rcu_read_lock();
>     2614         state = rcu_dereference(wi_state);
>     2615         if (state) {
>     2616                 memcpy(weights, state->iw_table, nr_node_ids * sizeof(u8));
>     2617                 rcu_read_unlock();
>     2618         } else {
>     2619                 rcu_read_unlock();
>     2620                 for (i = 0; i < nr_node_ids; i++)
>     2621                         weights[i] = 1;
>     2622         }
>     2623 
>     2624         /* calculate total, detect system default usage */
>     2625         for_each_node_mask(node, nodes)
>     2626                 weight_total += weights[node];
>     2627 
>     2628         /*
>     2629          * Calculate rounds/partial rounds to minimize __alloc_pages_bulk calls.
>     2630          * Track which node weighted interleave should resume from.
>     2631          * Account for carryover. It is always allocated from the first node.
>     2632          *
>     2633          * if (rounds > 0) and (delta == 0), resume_node will always be
>     2634          * the node following prev_node and its weight.
>     2635          */
>     2636         rounds = (rem_pages - carryover) / weight_total;
>     2637         delta = (rem_pages - carryover) % weight_total;
>     2638         resume_node = next_node_in(prev_node, nodes);
>     2639         resume_weight = weights[resume_node];
>     2640         node = carryover ? prev_node : next_node_in(prev_node, nodes);
> --> 2641         for (i = 0; i < nnodes; i++) {
>                              ^^^^^^^^^^^^^^^
> Uninitialized variable.  In production people should use the config to
> zero out stack variables but I always encourage developers to use
> CONFIG_INIT_STACK_ALL_PATTERN=y in their testing.

Hello Dan,

Thank you for your feedback! I will look into the config, I am sure it will
be very helpful for future patches that I work on. As for this specific
uninitialized variable, Kees brought it up in [1], and I'll be revising
this in future versions. Sorry for the bad coding style, I think I got a 
litle too eager to make the code look prettier.

I hope you have a great day!
Joshua

[1] https://lore.kernel.org/all/7c1180f4-923c-4138-b756-618cb5d597ac@ijzerbout.nl/

Sent using hkml (https://github.com/sjp38/hackermail)