Re: [PATCH 07/14] mm: consider zone which is not fully populated to have holes

[Michal Hocko]

On Thu 18-05-17 18:14:39, Vlastimil Babka wrote:
> On 05/15/2017 10:58 AM, Michal Hocko wrote:
[...]
> >  #ifdef CONFIG_MEMORY_HOTPLUG
> > +/*
> > + * Return page for the valid pfn only if the page is online. All pfn
> > + * walkers which rely on the fully initialized page->flags and others
> > + * should use this rather than pfn_valid && pfn_to_page
> > + */
> > +#define pfn_to_online_page(pfn)				\
> > +({							\
> > +	struct page *___page = NULL;			\
> > +							\
> > +	if (online_section_nr(pfn_to_section_nr(pfn)))	\
> > +		___page = pfn_to_page(pfn);		\
> > +	___page;					\
> > +})
> 
> This seems to be already assuming pfn_valid() to be true. There's no
> "pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS" check and the comment
> suggests as such, but...

Yes, we should check the validity of the section number. We do not have
to check whether the section is valid because online sections are a
subset of those that are valid.

> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 05796ee974f7..c3a146028ba6 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -929,6 +929,9 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> >  	unsigned long i;
> >  	unsigned long onlined_pages = *(unsigned long *)arg;
> >  	struct page *page;
> > +
> > +	online_mem_sections(start_pfn, start_pfn + nr_pages);
> 
> Shouldn't this be moved *below* the loop that initializes struct pages?
> In the offline case you do mark sections offline before "tearing" struct
> pages, so that should be symmetric.

You are right! Andrew, could you fold the following intot the patch?
---