hugetlb BUILD REGRESSION in linux-next20221121

hugetlb BUILD REGRESSION in linux-next20221121

[Sidhartha Kumar]

Hello,

One of my patches in linux-next was flagged as a Unverified 
Error/Warning with the following warning[1]:

mm/hugetlb.c:2073 alloc_pool_huge_page() error: uninitialized symbol 
'folio'.

The relevant change is:

-    struct page *page;
+    struct folio *folio;
      int nr_nodes, node;
      gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;

      for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
-        page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
-                        node_alloc_noretry);
-        if (page)
+        folio = alloc_fresh_hugetlb_folio(h, gfp_mask, node,
+                    nodes_allowed, node_alloc_noretry);
+        if (folio)
              break;
      }

-    if (!page)
+    if (!folio)

It looks like I can initialize folio to NULL to avoid this error but I'm 
not sure how this would cause a regression as previously the page 
variable was unitialized as well. Please let me know if I am missing 
something in my patch or if this should be ignored.

Thanks,

Sidhartha Kumar

[1]: 
https://lore.kernel.org/linux-arm-kernel/637bf477.ORnAaf8quqxr%2FcRo%25lkp@intel.com/T/

Re: hugetlb BUILD REGRESSION in linux-next20221121

[Dan Carpenter]

On Mon, Nov 28, 2022 at 02:34:54PM -0800, Sidhartha Kumar wrote:
> Hello,
> 
> One of my patches in linux-next was flagged as a Unverified Error/Warning
> with the following warning[1]:
> 
> mm/hugetlb.c:2073 alloc_pool_huge_page() error: uninitialized symbol
> 'folio'.
> 
> The relevant change is:
> 
> -    struct page *page;
> +    struct folio *folio;
>      int nr_nodes, node;
>      gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
> 
>      for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
> -        page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
> -                        node_alloc_noretry);
> -        if (page)
> +        folio = alloc_fresh_hugetlb_folio(h, gfp_mask, node,
> +                    nodes_allowed, node_alloc_noretry);
> +        if (folio)
>              break;
>      }
> 
> -    if (!page)
> +    if (!folio)
> 
> It looks like I can initialize folio to NULL to avoid this error but I'm not
> sure how this would cause a regression as previously the page variable was
> unitialized as well. Please let me know if I am missing something in my
> patch or if this should be ignored.

Both the original and the new code trigger a Smatch warning.  I don't
know why the kbuild-bot marks this as a new warning.  Possibly that's
because the variable name has changed?  The kbuild-bot is run by Intel.

The problem is obviously that Smatch doesn't know that we always enter
the loop.  There are hack arounds that I could do for this, but
sometimes we dont' actually enter the loop so changing this will hide
bugs...  I am conflicted on this.

regards,
dan carpenter

Re: hugetlb BUILD REGRESSION in linux-next20221121

[Rasmus Villemoes]

On 29/11/2022 05.57, Dan Carpenter wrote:
> On Mon, Nov 28, 2022 at 02:34:54PM -0800, Sidhartha Kumar wrote:
>> Hello,
>>
>> One of my patches in linux-next was flagged as a Unverified Error/Warning
>> with the following warning[1]:
>>
>> mm/hugetlb.c:2073 alloc_pool_huge_page() error: uninitialized symbol
>> 'folio'.
>>
>> The relevant change is:
>>
>> -    struct page *page;
>> +    struct folio *folio;
>>      int nr_nodes, node;
>>      gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>>
>>      for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
>> -        page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
>> -                        node_alloc_noretry);
>> -        if (page)
>> +        folio = alloc_fresh_hugetlb_folio(h, gfp_mask, node,
>> +                    nodes_allowed, node_alloc_noretry);
>> +        if (folio)
>>              break;
>>      }
>>
>> -    if (!page)
>> +    if (!folio)
>>
>> It looks like I can initialize folio to NULL to avoid this error but I'm not
>> sure how this would cause a regression as previously the page variable was
>> unitialized as well. Please let me know if I am missing something in my
>> patch or if this should be ignored.
> 
> Both the original and the new code trigger a Smatch warning.  I don't
> know why the kbuild-bot marks this as a new warning.  Possibly that's
> because the variable name has changed?  The kbuild-bot is run by Intel.
> 
> The problem is obviously that Smatch doesn't know that we always enter
> the loop.  There are hack arounds that I could do for this, but
> sometimes we dont' actually enter the loop so changing this will hide
> bugs...  I am conflicted on this.

It seems that a reasonably clean way to deal with this, since there's
not a lot of code following the loop, is to simply move that code into
the "if (page/folio)" branch. I.e. change the tail to


   if (page) {
     free_huge_page(page); /* free it into the hugepage allocator */
     return 1;
   }
 } // this is the end of loop
 return 0;

or however it looks after the above change.

Rasmus

Re: hugetlb BUILD REGRESSION in linux-next20221121

[Sidhartha Kumar]

On 11/28/22 11:40 PM, Rasmus Villemoes wrote:
> On 29/11/2022 05.57, Dan Carpenter wrote:
>> On Mon, Nov 28, 2022 at 02:34:54PM -0800, Sidhartha Kumar wrote:
>>> Hello,
>>>
>>> One of my patches in linux-next was flagged as a Unverified Error/Warning
>>> with the following warning[1]:
>>>
>>> mm/hugetlb.c:2073 alloc_pool_huge_page() error: uninitialized symbol
>>> 'folio'.
>>>
>>> The relevant change is:
>>>
>>> -    struct page *page;
>>> +    struct folio *folio;
>>>       int nr_nodes, node;
>>>       gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>>>
>>>       for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
>>> -        page = alloc_fresh_huge_page(h, gfp_mask, node, nodes_allowed,
>>> -                        node_alloc_noretry);
>>> -        if (page)
>>> +        folio = alloc_fresh_hugetlb_folio(h, gfp_mask, node,
>>> +                    nodes_allowed, node_alloc_noretry);
>>> +        if (folio)
>>>               break;
>>>       }
>>>
>>> -    if (!page)
>>> +    if (!folio)
>>>
>>> It looks like I can initialize folio to NULL to avoid this error but I'm not
>>> sure how this would cause a regression as previously the page variable was
>>> unitialized as well. Please let me know if I am missing something in my
>>> patch or if this should be ignored.
>> Both the original and the new code trigger a Smatch warning.  I don't
>> know why the kbuild-bot marks this as a new warning.  Possibly that's
>> because the variable name has changed?  The kbuild-bot is run by Intel.
>>
>> The problem is obviously that Smatch doesn't know that we always enter
>> the loop.  There are hack arounds that I could do for this, but
>> sometimes we dont' actually enter the loop so changing this will hide
>> bugs...  I am conflicted on this.
> It seems that a reasonably clean way to deal with this, since there's
> not a lot of code following the loop, is to simply move that code into
> the "if (page/folio)" branch. I.e. change the tail to
>
>
>     if (page) {
>       free_huge_page(page); /* free it into the hugepage allocator */
>       return 1;
>     }
>   } // this is the end of loop
>   return 0;
>
> or however it looks after the above change.

Thanks for the suggestion, this looks like a good way to avoid the 
smatch warning and I'll add it to the next version of my patch.

Thanks.

Sidhartha Kumar


> Rasmus
>