Message ID | 20190827053656.32191-3-alastair@au1.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mm: Minor cleanup | expand |
On Tue 27-08-19 15:36:55, Alastair D'Silva wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > By adding offset to memmap before passing it in to clear_hwpoisoned_pages, > we hide a theoretically null memmap from the null check inside > clear_hwpoisoned_pages. Isn't that other way around? Calculating the offset struct page pointer will actually make the null check effective. Besides that I cannot really see how pfn_to_page would return NULL. I have to confess that I cannot really see how offset could lead to a NULL struct page either and I strongly suspect that the NULL check is not really needed. Maybe it used to be in the past. > This patch passes the offset to clear_hwpoisoned_pages instead, allowing > memmap to successfully perform it's null check. I do not see any improvement in this patch. It just adds a new argument unnecessarily. > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > --- > mm/sparse.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/mm/sparse.c b/mm/sparse.c > index e41917a7e844..3ff84e627e58 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -882,7 +882,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn, > } > > #ifdef CONFIG_MEMORY_FAILURE > -static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) > +static void clear_hwpoisoned_pages(struct page *memmap, int start, int count) > { > int i; > > @@ -898,7 +898,7 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) > if (atomic_long_read(&num_poisoned_pages) == 0) > return; > > - for (i = 0; i < nr_pages; i++) { > + for (i = start; i < start + count; i++) { > if (PageHWPoison(&memmap[i])) { > num_poisoned_pages_dec(); > ClearPageHWPoison(&memmap[i]); > @@ -906,7 +906,8 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) > } > } > #else > -static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) > +static inline void clear_hwpoisoned_pages(struct page *memmap, int start, > + int count) > { > } > #endif > @@ -915,7 +916,7 @@ void sparse_remove_section(struct mem_section *ms, unsigned long pfn, > unsigned long nr_pages, unsigned long map_offset, > struct vmem_altmap *altmap) > { > - clear_hwpoisoned_pages(pfn_to_page(pfn) + map_offset, > + clear_hwpoisoned_pages(pfn_to_page(pfn), map_offset, > nr_pages - map_offset); > section_deactivate(pfn, nr_pages, altmap); > } > -- > 2.21.0 >
On Tue, 2019-08-27 at 08:24 +0200, Michal Hocko wrote: > On Tue 27-08-19 15:36:55, Alastair D'Silva wrote: > > From: Alastair D'Silva <alastair@d-silva.org> > > > > By adding offset to memmap before passing it in to > > clear_hwpoisoned_pages, > > we hide a theoretically null memmap from the null check inside > > clear_hwpoisoned_pages. > > Isn't that other way around? Calculating the offset struct page > pointer > will actually make the null check effective. Besides that I cannot > really see how pfn_to_page would return NULL. I have to confess that > I > cannot really see how offset could lead to a NULL struct page either > and > I strongly suspect that the NULL check is not really needed. Maybe it > used to be in the past. > You're probably right, but I didn't feel confident in removing the NULL check. While the NULL check remains though, I can't see how adding the offset would turn a non-NULL pointer into a NULL unless the pointer is invalid in the first place, and if this is the case, we should have a comment explaining this. The NULL check was added in commit: 95a4774d055c ("memory-hotplug: update mce_bad_pages when removing the memory") where memmap was originally inited to NULL, and only conditionally given a value. With this in mind, since that situation is no longer true, I think we could instead drop the NULL check.
On 27.08.19 09:00, Alastair D'Silva wrote: > On Tue, 2019-08-27 at 08:24 +0200, Michal Hocko wrote: >> On Tue 27-08-19 15:36:55, Alastair D'Silva wrote: >>> From: Alastair D'Silva <alastair@d-silva.org> >>> >>> By adding offset to memmap before passing it in to >>> clear_hwpoisoned_pages, >>> we hide a theoretically null memmap from the null check inside >>> clear_hwpoisoned_pages. >> >> Isn't that other way around? Calculating the offset struct page >> pointer >> will actually make the null check effective. Besides that I cannot >> really see how pfn_to_page would return NULL. I have to confess that >> I >> cannot really see how offset could lead to a NULL struct page either >> and >> I strongly suspect that the NULL check is not really needed. Maybe it >> used to be in the past. >> > > You're probably right, but I didn't feel confident in removing the NULL > check. > > While the NULL check remains though, I can't see how adding the offset > would turn a non-NULL pointer into a NULL unless the pointer is invalid > in the first place, and if this is the case, we should have a comment > explaining this. > > The NULL check was added in commit: > 95a4774d055c ("memory-hotplug: update mce_bad_pages when removing the > memory") > where memmap was originally inited to NULL, and only conditionally > given a value. > > With this in mind, since that situation is no longer true, I think we > could instead drop the NULL check. > Makes sense to me.
On Tue 27-08-19 17:00:16, Alastair D'Silva wrote: [...] > The NULL check was added in commit: > 95a4774d055c ("memory-hotplug: update mce_bad_pages when removing the > memory") > where memmap was originally inited to NULL, and only conditionally > given a value. > > With this in mind, since that situation is no longer true, I think we > could instead drop the NULL check. This would be much more preferable to the original patch.
diff --git a/mm/sparse.c b/mm/sparse.c index e41917a7e844..3ff84e627e58 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -882,7 +882,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn, } #ifdef CONFIG_MEMORY_FAILURE -static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) +static void clear_hwpoisoned_pages(struct page *memmap, int start, int count) { int i; @@ -898,7 +898,7 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) if (atomic_long_read(&num_poisoned_pages) == 0) return; - for (i = 0; i < nr_pages; i++) { + for (i = start; i < start + count; i++) { if (PageHWPoison(&memmap[i])) { num_poisoned_pages_dec(); ClearPageHWPoison(&memmap[i]); @@ -906,7 +906,8 @@ static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) } } #else -static inline void clear_hwpoisoned_pages(struct page *memmap, int nr_pages) +static inline void clear_hwpoisoned_pages(struct page *memmap, int start, + int count) { } #endif @@ -915,7 +916,7 @@ void sparse_remove_section(struct mem_section *ms, unsigned long pfn, unsigned long nr_pages, unsigned long map_offset, struct vmem_altmap *altmap) { - clear_hwpoisoned_pages(pfn_to_page(pfn) + map_offset, + clear_hwpoisoned_pages(pfn_to_page(pfn), map_offset, nr_pages - map_offset); section_deactivate(pfn, nr_pages, altmap); }