Re: [RFC] memory_hotplug: Free pages as pageblock_order

[Arun KS <arunks@codeaurora.org>]

Hello Michal and Balbir,

Thanks for reviewing.

On 2018-09-12 18:27, Balbir Singh wrote:
> On Wed, Sep 12, 2018 at 12:38:53PM +0200, Michal Hocko wrote:
>> On Wed 12-09-18 14:56:45, Arun KS wrote:
>> > When free pages are done with pageblock_order, time spend on
>> > coalescing pages by buddy allocator can be reduced. With
>> > section size of 256MB, hot add latency of a single section
>> > shows improvement from 50-60 ms to less than 1 ms, hence
>> > improving the hot add latency by 60%.
>> 
>> Where does the improvement come from? You are still doing the same
>> amount of work except that the number of callbacks is lower. Is this 
>> the
>> real source of 60% improvement?
>> 
> 
> It looks like only the first page of the pageblock is initialized, is
> some of the cost amortized in terms of doing one initialization for
> the page with order (order) and then relying on split_page and helpers
> to do the rest? Of course the number of callbacks reduce by a 
> significant
> number as well.
Currently, order zero pages are freed one by one, they goes to pcp list 
and later when pcp->count >= pcp->high, kernel calls __free_one_page() 
in a loop. __free_one_page() tries to merge these pages to create bigger 
order page.

But when we free with higher order page(pageblock_order), this merging 
is not done. AFAIU, this is the reason for improvement in hot add 
latency.

> 
> 
>> >
>> > If this looks okey, I'll modify users of set_online_page_callback
>> > and resend clean patch.
>> 
>> [...]
>> 
>> > +static int generic_online_pages(struct page *page, unsigned int order);
>> > +static online_pages_callback_t online_pages_callback = generic_online_pages;
>> > +
>> > +static int generic_online_pages(struct page *page, unsigned int order)
>> > +{
>> > +	unsigned long nr_pages = 1 << order;
>> > +	struct page *p = page;
>> > +	unsigned int loop;
>> > +
>> > +	for (loop = 0 ; loop < nr_pages ; loop++, p++) {
>> > +		__ClearPageReserved(p);
>> > +		set_page_count(p, 0);
>> > +	}
>> > +	adjust_managed_page_count(page, nr_pages);
>> > +	init_page_count(page);
>> > +	__free_pages(page, order);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static int online_pages_blocks(unsigned long start_pfn, unsigned long nr_pages)
>> > +{
>> > +	unsigned long pages_per_block = (1 << pageblock_order);
>> > +	unsigned long nr_pageblocks = nr_pages / pages_per_block;
>> > +//	unsigned long rem_pages = nr_pages % pages_per_block;
>> > +	int i, ret, onlined_pages = 0;
>> > +	struct page *page;
>> > +
>> > +	for (i = 0 ; i < nr_pageblocks ; i++) {
>> > +		page = pfn_to_page(start_pfn + (i * pages_per_block));
>> > +		ret = (*online_pages_callback)(page, pageblock_order);
>> > +		if (!ret)
>> > +			onlined_pages += pages_per_block;
>> > +		else if (ret > 0)
>> > +			onlined_pages += ret;
>> > +	}
>> 
>> Could you explain why does the pages_per_block step makes any sense? 
>> Why
>> don't you simply apply handle the full nr_pages worth of memory range
>> instead?
Yes. We can move the this loop to generic_online_pages and do 
__free_pages() of pageblock_order.

>> 
>> > +/*
>> > +	if (rem_pages)
>> > +		onlined_pages += online_page_single(start_pfn + i, rem_pages);
>> > +*/
> 
> Do we expect no rem_pages with this patch?
I ll remove this code, in assumption that section size will be always 
multiple of pageblock_order.

Regards,
Arun
> 
>> > +
>> > +	return onlined_pages;
>> > +}
>> > +
>> >  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);
>> >
>> >  	online_mem_sections(start_pfn, start_pfn + nr_pages);
> 
> 
> Balbir Singh.