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.
