All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()
@ 2017-01-23 16:51 ` Arnd Bergmann
  0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-01-23 16:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnd Bergmann, Dan Williams, Yasuaki Ishimatsu, Fabian Frederick,
	linux-mm, linux-kernel

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.

Fixes: mmotm ("mm: support section-unaligned ZONE_DEVICE memory ranges")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/sparse.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 4267d09b656b..dd0c2dd08ee2 100644
--- 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;
+	}
+
 	pgdat_resize_unlock(pgdat, &flags);
 
 	/*
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()
@ 2017-01-23 16:51 ` Arnd Bergmann
  0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-01-23 16:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnd Bergmann, Dan Williams, Yasuaki Ishimatsu, Fabian Frederick,
	linux-mm, linux-kernel

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.

Fixes: mmotm ("mm: support section-unaligned ZONE_DEVICE memory ranges")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/sparse.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 4267d09b656b..dd0c2dd08ee2 100644
--- 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;
+	}
+
 	pgdat_resize_unlock(pgdat, &flags);
 
 	/*
-- 
2.9.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()
  2017-01-23 16:51 ` Arnd Bergmann
@ 2017-01-23 22:38   ` Andrew Morton
  -1 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2017-01-23 22:38 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dan Williams, Yasuaki Ishimatsu, Fabian Frederick, linux-mm,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()
@ 2017-01-23 22:38   ` Andrew Morton
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2017-01-23 22:38 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Dan Williams, Yasuaki Ishimatsu, Fabian Frederick, linux-mm,
	linux-kernel

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



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()
  2017-01-23 22:38   ` Andrew Morton
@ 2017-01-24  1:24     ` Dan Williams
  -1 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2017-01-24  1:24 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnd Bergmann, Yasuaki Ishimatsu, Fabian Frederick, Linux MM,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5681 bytes --]

On Mon, Jan 23, 2017 at 2:38 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 23 Jan 2017 17:51:17 +0100 Arnd Bergmann <arnd@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?

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@intel.com>
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@suse.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Stephen Bates <stephen.bates@microsemi.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 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

[-- Attachment #2: 0001-mm-fix-and-clarify-section_active_mask.patch --]
[-- Type: text/x-patch, Size: 2078 bytes --]

From 8dc5ba15c54942aabf0e2f30b73c202a252633c1 Mon Sep 17 00:00:00 2001
From: Dan Williams <dan.j.williams@intel.com>
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@suse.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Stephen Bates <stephen.bates@microsemi.com>
Reported-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: fix maybe-uninitialized warning in section_deactivate()
@ 2017-01-24  1:24     ` Dan Williams
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2017-01-24  1:24 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnd Bergmann, Yasuaki Ishimatsu, Fabian Frederick, Linux MM,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3618 bytes --]

On Mon, Jan 23, 2017 at 2:38 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 23 Jan 2017 17:51:17 +0100 Arnd Bergmann <arnd@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?

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---

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-01-24  1:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-23 16:51 [PATCH] mm: fix maybe-uninitialized warning in section_deactivate() Arnd Bergmann
2017-01-23 16:51 ` Arnd Bergmann
2017-01-23 22:38 ` Andrew Morton
2017-01-23 22:38   ` Andrew Morton
2017-01-24  1:24   ` Dan Williams
2017-01-24  1:24     ` Dan Williams

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.