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
