On Mon, 23 Jan 2017 17:51:17 +0100 Arnd Bergmann <a...@arndb.de> 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? 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?
