Re: [PATCH v7] mm/page_alloc.c: memory_hotplug: free pages as higher order

[Arun KS]

On 2019-01-09 14:10, Michal Hocko wrote:
> On Wed 09-01-19 13:58:50, Arun KS wrote:
> > On 2019-01-09 13:07, Michal Hocko wrote:
> > > On Wed 09-01-19 11:28:52, Arun KS wrote:
> > > > On 2019-01-08 23:43, Michal Hocko wrote:
> > > > > On Tue 08-01-19 09:56:09, Alexander Duyck wrote:
> > > > > > On Fri, 2019-01-04 at 10:31 +0530, Arun KS wrote:
> > > > > [...]
> > > > > > >  static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > > > > > >  			void *arg)
> > > > > > >  {
> > > > > > > -	unsigned long i;
> > > > > > >  	unsigned long onlined_pages = *(unsigned long *)arg;
> > > > > > > -	struct page *page;
> > > > > > >
> > > > > > >  	if (PageReserved(pfn_to_page(start_pfn)))
> > > > > > > -		for (i = 0; i < nr_pages; i++) {
> > > > > > > -			page = pfn_to_page(start_pfn + i);
> > > > > > > -			(*online_page_callback)(page);
> > > > > > > -			onlined_pages++;
> > > > > > > -		}
> > > > > > > +		onlined_pages = online_pages_blocks(start_pfn, nr_pages);
> > > > > >
> > > > > > Shouldn't this be a "+=" instead of an "="? It seems like you are
> > > > > > going
> > > > > > to lose your count otherwise.
> > > > >
> > > > > You are right of course. I should have noticed during the review.
> > > > > Thanks!
> > > >
> > > > I think we don't need to. The caller function is setting
> > > > onlined_pages = 0
> > > > before calling online_pages_range().
> > > > And there are no other reference to online_pages_range other than from
> > > > online_pages().
> > >
> > > Are you missing that we accumulate onlined_pages via
> > > 	*(unsigned long *)arg = onlined_pages;
> > > in online_pages_range?
> >
> > In my testing I didn't find any problem. To match the code being 
> > replaced
> > and to avoid any corner cases, it is better to use +=
> > Will update the patch.
>
> Have you checked that the number of present pages both in the zone and
> the node is correct because I fail to see how that would be possible.

Yes they are showing correct values.

Previous value of cat /proc/zoneinfo,

Node 0, zone   Normal
  pages free     65492
        min      300
        low      375
        high     450
        spanned  65536
        present  65536
        managed  65536

Value after hotadd,

Node 0, zone   Normal
  pages free     129970
        min      518
        low      649
        high     780
        spanned  983040
        present  131072
        managed  131072

I added prints in online_pages_range function.
It will be called once per online of a section and the arg value is 
always set to 0 while entering online_pages_range.

/sys/devices/system/memory # echo online > memory16/state
[   52.956558] online_pages_range start_pfn = 100000 nr_pages = 65536 
arg = 0
[   52.964104] Built 1 zonelists, mobility grouping on.  Total pages: 
187367
[   52.964828] Policy zone: Normal

But still I'll change to += to match with the previous code.

Regards,
Arun