Re: [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()

[Dan Williams]

 On Mon, Jan 23, 2017 at 2:38 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 23 Jan 2017 17:51:17 +0100 Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
>> gcc cannot track the combined state of the 'mask' variable across the
>> barrier in pgdat_resize_unlock() at compile time, so it warns that we
>> can run into undefined behavior:
>>
>> mm/sparse.c: In function 'section_deactivate':
>> mm/sparse.c:802:7: error: 'early_section' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> We know that this can't happen because the spin_unlock() doesn't
>> affect the mask variable, so this is a false-postive warning, but
>> rearranging the code to bail out earlier here makes it obvious
>> to the compiler as well.
>>
>> ...
>>
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -807,23 +807,24 @@ static void section_deactivate(struct pglist_data *pgdat, unsigned long pfn,
>>       unsigned long mask = section_active_mask(pfn, nr_pages), flags;
>>
>>       pgdat_resize_lock(pgdat, &flags);
>> -     if (!ms->usage) {
>> -             mask = 0;
>> -     } else if ((ms->usage->map_active & mask) != mask) {
>> -             WARN(1, "section already deactivated active: %#lx mask: %#lx\n",
>> -                             ms->usage->map_active, mask);
>> -             mask = 0;
>> -     } else {
>> -             early_section = is_early_section(ms);
>> -             ms->usage->map_active ^= mask;
>> -             if (ms->usage->map_active == 0) {
>> -                     usage = ms->usage;
>> -                     ms->usage = NULL;
>> -                     memmap = sparse_decode_mem_map(ms->section_mem_map,
>> -                                     section_nr);
>> -                     ms->section_mem_map = 0;
>> -             }
>> +     if (!ms->usage ||
>> +         WARN((ms->usage->map_active & mask) != mask,
>> +              "section already deactivated active: %#lx mask: %#lx\n",
>> +                     ms->usage->map_active, mask)) {
>> +             pgdat_resize_unlock(pgdat, &flags);
>> +             return;
>>       }
>> +
>> +     early_section = is_early_section(ms);
>> +     ms->usage->map_active ^= mask;
>> +     if (ms->usage->map_active == 0) {
>> +             usage = ms->usage;
>> +             ms->usage = NULL;
>> +             memmap = sparse_decode_mem_map(ms->section_mem_map,
>> +                             section_nr);
>> +             ms->section_mem_map = 0;
>> +     }
>> +
>
> hm, OK, that looks equivalent.
>
> I wonder if we still need the later
>
>         if (!mask)
>                 return;
>
> I wonder if this code is appropriately handling the `mask == -1' case.
> section_active_mask() can do that.
>
> What does that -1 in section_active_mask() mean anyway?  Was it really
> intended to represent the all-ones pattern or is it an error?

It's supposed to represent a full section's worth of bits, patch below
to add comments and switch over to ULONG_MAX to make it clearer. I
also fixed a bug with the case where the start pfn is section aligned,
but nr_pages is less than a section.

> If the
> latter, was it appropriate for section_active_mask() to return an
> unsigned type?
>
> How come section_active_mask() is __init but its caller
> section_deactivate() is not?

section_deactivate() is called from the memory hot-remove path which
has traditionally not been tagged __meminit, so  section_active_mask()
can't be __init either.  I missed this earlier when I reviewed your
fix, and it seems you got it clarified now with the fix from Arnd.

Fix up patch attached, and possibly whitespace damaged version below:


--->8---