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--- >From 8dc5ba15c54942aabf0e2f30b73c202a252633c1 Mon Sep 17 00:00:00 2001 From: Dan Williams <dan.j.williams@xxxxxxxxx> Date: Mon, 23 Jan 2017 17:05:19 -0800 Subject: [PATCH] mm: fix and clarify section_active_mask() section_active_mask() converts a range of pfns into a bitmask where each bit represents a 2M sub-range of a 128MB section (on x86_64). Clarify that we expect that the arguments do not cross a section boundary, fix the case where the start pfn is section aligned, but nr_pages is less than a section, and use ULONG_MAX instead of -1 to represent to the mask full case. Cc: Michal Hocko <mhocko@xxxxxxxx> Cc: Vlastimil Babka <vbabka@xxxxxxx> Cc: Johannes Weiner <hannes@xxxxxxxxxxx> Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx> Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> Cc: Stephen Bates <stephen.bates@xxxxxxxxxxxxx> Reported-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> --- mm/sparse.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/mm/sparse.c b/mm/sparse.c index 4267d09b656b..7bb792b719b5 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -195,11 +195,18 @@ static unsigned long section_active_mask(unsigned long pfn, int idx_start, idx_size; phys_addr_t start, size; + WARN_ON((pfn & ~PAGE_SECTION_MASK) + nr_pages > PAGES_PER_SECTION); if (!nr_pages) return 0; + /* + * The size is the number of pages left in the section or + * nr_pages, whichever is smaller. The size will be rounded up + * to the next SECTION_ACTIVE_SIZE boundary, the start will be + * rounded down. + */ start = PFN_PHYS(pfn); - size = PFN_PHYS(min(nr_pages, PAGES_PER_SECTION + size = PFN_PHYS(min_not_zero(nr_pages, PAGES_PER_SECTION - (pfn & ~PAGE_SECTION_MASK))); size = ALIGN(size, SECTION_ACTIVE_SIZE); @@ -207,7 +214,7 @@ static unsigned long section_active_mask(unsigned long pfn, idx_size = section_active_index(size); if (idx_size == 0) - return -1; + return ULONG_MAX; /* full section */ return ((1UL << idx_size) - 1) << idx_start; } -- 2.7.4
From 8dc5ba15c54942aabf0e2f30b73c202a252633c1 Mon Sep 17 00:00:00 2001 From: Dan Williams <dan.j.williams@xxxxxxxxx> Date: Mon, 23 Jan 2017 17:05:19 -0800 Subject: [PATCH] mm: fix and clarify section_active_mask() section_active_mask() converts a range of pfns into a bitmask where each bit represents a 2M sub-range of a 128MB section (on x86_64). Clarify that we expect that the arguments do not cross a section boundary, fix the case where the start pfn is section aligned, but nr_pages is less than a section, and use ULONG_MAX instead of -1 to represent to the mask full case. Cc: Michal Hocko <mhocko@xxxxxxxx> Cc: Vlastimil Babka <vbabka@xxxxxxx> Cc: Johannes Weiner <hannes@xxxxxxxxxxx> Cc: Logan Gunthorpe <logang@xxxxxxxxxxxx> Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> Cc: Stephen Bates <stephen.bates@xxxxxxxxxxxxx> Reported-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> --- mm/sparse.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/mm/sparse.c b/mm/sparse.c index 4267d09b656b..7bb792b719b5 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -195,11 +195,18 @@ static unsigned long section_active_mask(unsigned long pfn, int idx_start, idx_size; phys_addr_t start, size; + WARN_ON((pfn & ~PAGE_SECTION_MASK) + nr_pages > PAGES_PER_SECTION); if (!nr_pages) return 0; + /* + * The size is the number of pages left in the section or + * nr_pages, whichever is smaller. The size will be rounded up + * to the next SECTION_ACTIVE_SIZE boundary, the start will be + * rounded down. + */ start = PFN_PHYS(pfn); - size = PFN_PHYS(min(nr_pages, PAGES_PER_SECTION + size = PFN_PHYS(min_not_zero(nr_pages, PAGES_PER_SECTION - (pfn & ~PAGE_SECTION_MASK))); size = ALIGN(size, SECTION_ACTIVE_SIZE); @@ -207,7 +214,7 @@ static unsigned long section_active_mask(unsigned long pfn, idx_size = section_active_index(size); if (idx_size == 0) - return -1; + return ULONG_MAX; /* full section */ return ((1UL << idx_size) - 1) << idx_start; } -- 2.7.4