Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Thu, Oct 11, 2018 at 6:05 PM Vlastimil Babka wrote:
>
> On 10/10/18 6:56 PM, Arun KS wrote:
> > On 2018-10-10 21:00, Vlastimil Babka wrote:
> >> On 10/5/18 10:10 AM, Arun KS wrote:
> >>> When free pages are done with higher 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%. Modify external
> >>> providers of online callback to align with the change.
> >>>
> >>> Signed-off-by: Arun KS
> >>
> >> [...]
> >>
> >>> @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
> >>> }
> >>> EXPORT_SYMBOL_GPL(__online_page_free);
> >>>
> >>> -static void generic_online_page(struct page *page)
> >>> +static int generic_online_page(struct page *page, unsigned int order)
> >>> {
> >>> - __online_page_set_limits(page);
> >>
> >> This is now not called anymore, although the xen/hv variants still do
> >> it. The function seems empty these days, maybe remove it as a followup
> >> cleanup?
> >>
> >>> - __online_page_increment_counters(page);
> >>> - __online_page_free(page);
> >>> + __free_pages_core(page, order);
> >>> + totalram_pages += (1UL << order);
> >>> +#ifdef CONFIG_HIGHMEM
> >>> + if (PageHighMem(page))
> >>> + totalhigh_pages += (1UL << order);
> >>> +#endif
> >>
> >> __online_page_increment_counters() would have used
> >> adjust_managed_page_count() which would do the changes under
> >> managed_page_count_lock. Are we safe without the lock? If yes, there
> >> should perhaps be a comment explaining why.
> >
> > Looks unsafe without managed_page_count_lock. I think better have a
> > similar implementation of free_boot_core() in memory_hotplug.c like we
> > had in version 1 of patch. And use adjust_managed_page_count() instead
> > of page_zone(page)->managed_pages += nr_pages;
> >
> > https://lore.kernel.org/patchwork/patch/989445/
>
> Looks like deferred_free_range() has the same problem calling
> __free_pages_core() to adjust zone->managed_pages. I expect
> __free_pages_bootmem() is OK because at that point the system is still
> single-threaded?
> Could be solved by moving that out of __free_pages_core().
>
Seems deferred_free_range() is protected by
pgdat_resize_lock()/pgdat_resize_unlock().
Which protects pgdat's zones, if I am right.
> But do we care about readers potentially seeing a store tear? If yes
> then maybe these counters should be converted to atomics...
>
> > -static void generic_online_page(struct page *page)
> > +static int generic_online_page(struct page *page, unsigned int order)
> > {
> > - __online_page_set_limits(page);
> > - __online_page_increment_counters(page);
> > - __online_page_free(page);
> > + unsigned long nr_pages = 1 << order;
> > + struct page *p = page;
> > +
> > + for (loop = 0 ; loop < nr_pages ; loop++, p++) {
> > + __ClearPageReserved(p);
> > + set_page_count(p, 0);
> > + }
> > +
> > + adjust_managed_page_count(page, nr_pages);
> > + set_page_refcounted(page);
> > + __free_pages(page, order);
> > +
> > + return 0;
> > +}
> >
> >
> > Regards,
> > Arun
> >
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-11-06 03:14, Andrew Morton wrote: On Mon, 05 Nov 2018 15:12:27 +0530 Arun KS wrote: On 2018-10-22 16:03, Arun KS wrote: > On 2018-10-19 13:37, Michal Hocko wrote: >> On Thu 18-10-18 19:18:25, Andrew Morton wrote: >> [...] >>> So this patch needs more work, yes? >> >> Yes, I've talked to Arun (he is offline until next week) offlist and >> he >> will play with this some more. > > Converted totalhigh_pages, totalram_pages and zone->managed_page to > atomic and tested hot add. Latency is not effected with this change. > Will send out a separate patch on top of this one. Hello Andrew/Michal, Will this be going in subsequent -rcs? I thought were awaiting a new version? "Will send out a separate patch on top of this one"? Sorry for confusion. I sent out an incremental patch converting counters to atomics. https://patchwork.kernel.org/cover/10657217/ I do think a resend would be useful, please. Ensure the changelog is updated to capture the above info and any other worthy issues which arose during review. Will do that. Regards, Arun ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Mon, 05 Nov 2018 15:12:27 +0530 Arun KS wrote: > On 2018-10-22 16:03, Arun KS wrote: > > On 2018-10-19 13:37, Michal Hocko wrote: > >> On Thu 18-10-18 19:18:25, Andrew Morton wrote: > >> [...] > >>> So this patch needs more work, yes? > >> > >> Yes, I've talked to Arun (he is offline until next week) offlist and > >> he > >> will play with this some more. > > > > Converted totalhigh_pages, totalram_pages and zone->managed_page to > > atomic and tested hot add. Latency is not effected with this change. > > Will send out a separate patch on top of this one. > Hello Andrew/Michal, > > Will this be going in subsequent -rcs? I thought were awaiting a new version? "Will send out a separate patch on top of this one"? I do think a resend would be useful, please. Ensure the changelog is updated to capture the above info and any other worthy issues which arose during review. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-10-22 16:03, Arun KS wrote: On 2018-10-19 13:37, Michal Hocko wrote: On Thu 18-10-18 19:18:25, Andrew Morton wrote: [...] So this patch needs more work, yes? Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more. Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one. Hello Andrew/Michal, Will this be going in subsequent -rcs? Regards, Arun ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-10-19 13:37, Michal Hocko wrote: On Thu 18-10-18 19:18:25, Andrew Morton wrote: [...] So this patch needs more work, yes? Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more. Converted totalhigh_pages, totalram_pages and zone->managed_page to atomic and tested hot add. Latency is not effected with this change. Will send out a separate patch on top of this one. Regards, Arun ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Thu 18-10-18 19:18:25, Andrew Morton wrote: [...] > So this patch needs more work, yes? Yes, I've talked to Arun (he is offline until next week) offlist and he will play with this some more. -- Michal Hocko SUSE Labs ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Thu, 11 Oct 2018 09:55:03 +0200 Michal Hocko wrote:
> > > > > This is now not called anymore, although the xen/hv variants still do
> > > > > it. The function seems empty these days, maybe remove it as a followup
> > > > > cleanup?
> > > > >
> > > > > > - __online_page_increment_counters(page);
> > > > > > - __online_page_free(page);
> > > > > > + __free_pages_core(page, order);
> > > > > > + totalram_pages += (1UL << order);
> > > > > > +#ifdef CONFIG_HIGHMEM
> > > > > > + if (PageHighMem(page))
> > > > > > + totalhigh_pages += (1UL << order);
> > > > > > +#endif
> > > > >
> > > > > __online_page_increment_counters() would have used
> > > > > adjust_managed_page_count() which would do the changes under
> > > > > managed_page_count_lock. Are we safe without the lock? If yes, there
> > > > > should perhaps be a comment explaining why.
> > > >
> > > > Looks unsafe without managed_page_count_lock.
> > >
> > > Why does it matter actually? We cannot online/offline memory in
> > > parallel. This is not the case for the boot where we initialize memory
> > > in parallel on multiple nodes. So this seems to be safe currently unless
> > > I am missing something. A comment explaining that would be helpful
> > > though.
> >
> > Other main callers of adjust_manage_page_count(),
> >
> > static inline void free_reserved_page(struct page *page)
> > {
> > __free_reserved_page(page);
> > adjust_managed_page_count(page, 1);
> > }
> >
> > static inline void mark_page_reserved(struct page *page)
> > {
> > SetPageReserved(page);
> > adjust_managed_page_count(page, -1);
> > }
> >
> > Won't they race with memory hotplug?
> >
> > Few more,
> > ./drivers/xen/balloon.c:519:adjust_managed_page_count(page, -1);
> > ./drivers/virtio/virtio_balloon.c:175: adjust_managed_page_count(page, -1);
> > ./drivers/virtio/virtio_balloon.c:196: adjust_managed_page_count(page, 1);
> > ./mm/hugetlb.c:2158:adjust_managed_page_count(page, 1 <<
> > h->order);
>
> They can, and I have missed those.
So this patch needs more work, yes?
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Thu 11-10-18 10:07:02, Vlastimil Babka wrote:
> On 10/10/18 6:56 PM, Arun KS wrote:
> > On 2018-10-10 21:00, Vlastimil Babka wrote:
> >> On 10/5/18 10:10 AM, Arun KS wrote:
> >>> When free pages are done with higher 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%. Modify external
> >>> providers of online callback to align with the change.
> >>>
> >>> Signed-off-by: Arun KS
> >>
> >> [...]
> >>
> >>> @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
> >>> }
> >>> EXPORT_SYMBOL_GPL(__online_page_free);
> >>>
> >>> -static void generic_online_page(struct page *page)
> >>> +static int generic_online_page(struct page *page, unsigned int order)
> >>> {
> >>> - __online_page_set_limits(page);
> >>
> >> This is now not called anymore, although the xen/hv variants still do
> >> it. The function seems empty these days, maybe remove it as a followup
> >> cleanup?
> >>
> >>> - __online_page_increment_counters(page);
> >>> - __online_page_free(page);
> >>> + __free_pages_core(page, order);
> >>> + totalram_pages += (1UL << order);
> >>> +#ifdef CONFIG_HIGHMEM
> >>> + if (PageHighMem(page))
> >>> + totalhigh_pages += (1UL << order);
> >>> +#endif
> >>
> >> __online_page_increment_counters() would have used
> >> adjust_managed_page_count() which would do the changes under
> >> managed_page_count_lock. Are we safe without the lock? If yes, there
> >> should perhaps be a comment explaining why.
> >
> > Looks unsafe without managed_page_count_lock. I think better have a
> > similar implementation of free_boot_core() in memory_hotplug.c like we
> > had in version 1 of patch. And use adjust_managed_page_count() instead
> > of page_zone(page)->managed_pages += nr_pages;
> >
> > https://lore.kernel.org/patchwork/patch/989445/
>
> Looks like deferred_free_range() has the same problem calling
> __free_pages_core() to adjust zone->managed_pages.
deferred initialization has one thread per node AFAIR so we cannot race
on managed_pages updates. Well, unless some of the mentioned can run
that early which I dunno.
> __free_pages_bootmem() is OK because at that point the system is still
> single-threaded?
> Could be solved by moving that out of __free_pages_core().
>
> But do we care about readers potentially seeing a store tear? If yes
> then maybe these counters should be converted to atomics...
I wanted to suggest that already but I have no idea whether the lock
instructions would cause more overhead.
--
Michal Hocko
SUSE Labs
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 10/10/18 6:56 PM, Arun KS wrote:
> On 2018-10-10 21:00, Vlastimil Babka wrote:
>> On 10/5/18 10:10 AM, Arun KS wrote:
>>> When free pages are done with higher 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%. Modify external
>>> providers of online callback to align with the change.
>>>
>>> Signed-off-by: Arun KS
>>
>> [...]
>>
>>> @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
>>> }
>>> EXPORT_SYMBOL_GPL(__online_page_free);
>>>
>>> -static void generic_online_page(struct page *page)
>>> +static int generic_online_page(struct page *page, unsigned int order)
>>> {
>>> - __online_page_set_limits(page);
>>
>> This is now not called anymore, although the xen/hv variants still do
>> it. The function seems empty these days, maybe remove it as a followup
>> cleanup?
>>
>>> - __online_page_increment_counters(page);
>>> - __online_page_free(page);
>>> + __free_pages_core(page, order);
>>> + totalram_pages += (1UL << order);
>>> +#ifdef CONFIG_HIGHMEM
>>> + if (PageHighMem(page))
>>> + totalhigh_pages += (1UL << order);
>>> +#endif
>>
>> __online_page_increment_counters() would have used
>> adjust_managed_page_count() which would do the changes under
>> managed_page_count_lock. Are we safe without the lock? If yes, there
>> should perhaps be a comment explaining why.
>
> Looks unsafe without managed_page_count_lock. I think better have a
> similar implementation of free_boot_core() in memory_hotplug.c like we
> had in version 1 of patch. And use adjust_managed_page_count() instead
> of page_zone(page)->managed_pages += nr_pages;
>
> https://lore.kernel.org/patchwork/patch/989445/
Looks like deferred_free_range() has the same problem calling
__free_pages_core() to adjust zone->managed_pages. I expect
__free_pages_bootmem() is OK because at that point the system is still
single-threaded?
Could be solved by moving that out of __free_pages_core().
But do we care about readers potentially seeing a store tear? If yes
then maybe these counters should be converted to atomics...
> -static void generic_online_page(struct page *page)
> +static int generic_online_page(struct page *page, unsigned int order)
> {
> - __online_page_set_limits(page);
> - __online_page_increment_counters(page);
> - __online_page_free(page);
> + unsigned long nr_pages = 1 << order;
> + struct page *p = page;
> +
> + for (loop = 0 ; loop < nr_pages ; loop++, p++) {
> + __ClearPageReserved(p);
> + set_page_count(p, 0);
> + }
> +
> + adjust_managed_page_count(page, nr_pages);
> + set_page_refcounted(page);
> + __free_pages(page, order);
> +
> + return 0;
> +}
>
>
> Regards,
> Arun
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Thu 11-10-18 07:59:32, Arun KS wrote:
> On 2018-10-10 23:03, Michal Hocko wrote:
> > On Wed 10-10-18 22:26:41, Arun KS wrote:
> > > On 2018-10-10 21:00, Vlastimil Babka wrote:
> > > > On 10/5/18 10:10 AM, Arun KS wrote:
> > > > > When free pages are done with higher 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%. Modify external
> > > > > providers of online callback to align with the change.
> > > > >
> > > > > Signed-off-by: Arun KS
> > > >
> > > > [...]
> > > >
> > > > > @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(__online_page_free);
> > > > >
> > > > > -static void generic_online_page(struct page *page)
> > > > > +static int generic_online_page(struct page *page, unsigned int order)
> > > > > {
> > > > > - __online_page_set_limits(page);
> > > >
> > > > This is now not called anymore, although the xen/hv variants still do
> > > > it. The function seems empty these days, maybe remove it as a followup
> > > > cleanup?
> > > >
> > > > > - __online_page_increment_counters(page);
> > > > > - __online_page_free(page);
> > > > > + __free_pages_core(page, order);
> > > > > + totalram_pages += (1UL << order);
> > > > > +#ifdef CONFIG_HIGHMEM
> > > > > + if (PageHighMem(page))
> > > > > + totalhigh_pages += (1UL << order);
> > > > > +#endif
> > > >
> > > > __online_page_increment_counters() would have used
> > > > adjust_managed_page_count() which would do the changes under
> > > > managed_page_count_lock. Are we safe without the lock? If yes, there
> > > > should perhaps be a comment explaining why.
> > >
> > > Looks unsafe without managed_page_count_lock.
> >
> > Why does it matter actually? We cannot online/offline memory in
> > parallel. This is not the case for the boot where we initialize memory
> > in parallel on multiple nodes. So this seems to be safe currently unless
> > I am missing something. A comment explaining that would be helpful
> > though.
>
> Other main callers of adjust_manage_page_count(),
>
> static inline void free_reserved_page(struct page *page)
> {
> __free_reserved_page(page);
> adjust_managed_page_count(page, 1);
> }
>
> static inline void mark_page_reserved(struct page *page)
> {
> SetPageReserved(page);
> adjust_managed_page_count(page, -1);
> }
>
> Won't they race with memory hotplug?
>
> Few more,
> ./drivers/xen/balloon.c:519:adjust_managed_page_count(page, -1);
> ./drivers/virtio/virtio_balloon.c:175: adjust_managed_page_count(page, -1);
> ./drivers/virtio/virtio_balloon.c:196: adjust_managed_page_count(page, 1);
> ./mm/hugetlb.c:2158:adjust_managed_page_count(page, 1 <<
> h->order);
They can, and I have missed those.
--
Michal Hocko
SUSE Labs
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-10-10 23:03, Michal Hocko wrote:
On Wed 10-10-18 22:26:41, Arun KS wrote:
On 2018-10-10 21:00, Vlastimil Babka wrote:
> On 10/5/18 10:10 AM, Arun KS wrote:
> > When free pages are done with higher 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%. Modify external
> > providers of online callback to align with the change.
> >
> > Signed-off-by: Arun KS
>
> [...]
>
> > @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
> > }
> > EXPORT_SYMBOL_GPL(__online_page_free);
> >
> > -static void generic_online_page(struct page *page)
> > +static int generic_online_page(struct page *page, unsigned int order)
> > {
> > - __online_page_set_limits(page);
>
> This is now not called anymore, although the xen/hv variants still do
> it. The function seems empty these days, maybe remove it as a followup
> cleanup?
>
> > - __online_page_increment_counters(page);
> > - __online_page_free(page);
> > + __free_pages_core(page, order);
> > + totalram_pages += (1UL << order);
> > +#ifdef CONFIG_HIGHMEM
> > + if (PageHighMem(page))
> > + totalhigh_pages += (1UL << order);
> > +#endif
>
> __online_page_increment_counters() would have used
> adjust_managed_page_count() which would do the changes under
> managed_page_count_lock. Are we safe without the lock? If yes, there
> should perhaps be a comment explaining why.
Looks unsafe without managed_page_count_lock.
Why does it matter actually? We cannot online/offline memory in
parallel. This is not the case for the boot where we initialize memory
in parallel on multiple nodes. So this seems to be safe currently
unless
I am missing something. A comment explaining that would be helpful
though.
Other main callers of adjust_manage_page_count(),
static inline void free_reserved_page(struct page *page)
{
__free_reserved_page(page);
adjust_managed_page_count(page, 1);
}
static inline void mark_page_reserved(struct page *page)
{
SetPageReserved(page);
adjust_managed_page_count(page, -1);
}
Won't they race with memory hotplug?
Few more,
./drivers/xen/balloon.c:519:adjust_managed_page_count(page,
-1);
./drivers/virtio/virtio_balloon.c:175: adjust_managed_page_count(page,
-1);
./drivers/virtio/virtio_balloon.c:196: adjust_managed_page_count(page,
1);
./mm/hugetlb.c:2158:adjust_managed_page_count(page,
1 << h->order);
Regards,
Arun
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Wed 10-10-18 22:26:41, Arun KS wrote:
> On 2018-10-10 21:00, Vlastimil Babka wrote:
> > On 10/5/18 10:10 AM, Arun KS wrote:
> > > When free pages are done with higher 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%. Modify external
> > > providers of online callback to align with the change.
> > >
> > > Signed-off-by: Arun KS
> >
> > [...]
> >
> > > @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
> > > }
> > > EXPORT_SYMBOL_GPL(__online_page_free);
> > >
> > > -static void generic_online_page(struct page *page)
> > > +static int generic_online_page(struct page *page, unsigned int order)
> > > {
> > > - __online_page_set_limits(page);
> >
> > This is now not called anymore, although the xen/hv variants still do
> > it. The function seems empty these days, maybe remove it as a followup
> > cleanup?
> >
> > > - __online_page_increment_counters(page);
> > > - __online_page_free(page);
> > > + __free_pages_core(page, order);
> > > + totalram_pages += (1UL << order);
> > > +#ifdef CONFIG_HIGHMEM
> > > + if (PageHighMem(page))
> > > + totalhigh_pages += (1UL << order);
> > > +#endif
> >
> > __online_page_increment_counters() would have used
> > adjust_managed_page_count() which would do the changes under
> > managed_page_count_lock. Are we safe without the lock? If yes, there
> > should perhaps be a comment explaining why.
>
> Looks unsafe without managed_page_count_lock.
Why does it matter actually? We cannot online/offline memory in
parallel. This is not the case for the boot where we initialize memory
in parallel on multiple nodes. So this seems to be safe currently unless
I am missing something. A comment explaining that would be helpful
though.
--
Michal Hocko
SUSE Labs
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-10-10 21:00, Vlastimil Babka wrote:
On 10/5/18 10:10 AM, Arun KS wrote:
When free pages are done with higher 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%. Modify external
providers of online callback to align with the change.
Signed-off-by: Arun KS
[...]
@@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
}
EXPORT_SYMBOL_GPL(__online_page_free);
-static void generic_online_page(struct page *page)
+static int generic_online_page(struct page *page, unsigned int order)
{
- __online_page_set_limits(page);
This is now not called anymore, although the xen/hv variants still do
it. The function seems empty these days, maybe remove it as a followup
cleanup?
- __online_page_increment_counters(page);
- __online_page_free(page);
+ __free_pages_core(page, order);
+ totalram_pages += (1UL << order);
+#ifdef CONFIG_HIGHMEM
+ if (PageHighMem(page))
+ totalhigh_pages += (1UL << order);
+#endif
__online_page_increment_counters() would have used
adjust_managed_page_count() which would do the changes under
managed_page_count_lock. Are we safe without the lock? If yes, there
should perhaps be a comment explaining why.
Looks unsafe without managed_page_count_lock. I think better have a
similar implementation of free_boot_core() in memory_hotplug.c like we
had in version 1 of patch. And use adjust_managed_page_count() instead
of page_zone(page)->managed_pages += nr_pages;
https://lore.kernel.org/patchwork/patch/989445/
-static void generic_online_page(struct page *page)
+static int generic_online_page(struct page *page, unsigned int order)
{
- __online_page_set_limits(page);
- __online_page_increment_counters(page);
- __online_page_free(page);
+ unsigned long nr_pages = 1 << order;
+ struct page *p = page;
+
+ for (loop = 0 ; loop < nr_pages ; loop++, p++) {
+ __ClearPageReserved(p);
+ set_page_count(p, 0);
+ }
+
+ adjust_managed_page_count(page, nr_pages);
+ set_page_refcounted(page);
+ __free_pages(page, order);
+
+ return 0;
+}
Regards,
Arun
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 10/5/18 10:10 AM, Arun KS wrote:
> When free pages are done with higher 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%. Modify external
> providers of online callback to align with the change.
>
> Signed-off-by: Arun KS
[...]
> @@ -655,26 +655,44 @@ void __online_page_free(struct page *page)
> }
> EXPORT_SYMBOL_GPL(__online_page_free);
>
> -static void generic_online_page(struct page *page)
> +static int generic_online_page(struct page *page, unsigned int order)
> {
> - __online_page_set_limits(page);
This is now not called anymore, although the xen/hv variants still do
it. The function seems empty these days, maybe remove it as a followup
cleanup?
> - __online_page_increment_counters(page);
> - __online_page_free(page);
> + __free_pages_core(page, order);
> + totalram_pages += (1UL << order);
> +#ifdef CONFIG_HIGHMEM
> + if (PageHighMem(page))
> + totalhigh_pages += (1UL << order);
> +#endif
__online_page_increment_counters() would have used
adjust_managed_page_count() which would do the changes under
managed_page_count_lock. Are we safe without the lock? If yes, there
should perhaps be a comment explaining why.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Wed, Oct 10, 2018 at 04:21:16PM +0530, Arun KS wrote:
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e379e85..2416136 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -690,9 +690,13 @@ static int online_pages_range(unsigned long start_pfn,
> unsigned long nr_pages,
> void *arg)
> {
> unsigned long onlined_pages = *(unsigned long *)arg;
> + u64 t1, t2;
>
> + t1 = local_clock();
> if (PageReserved(pfn_to_page(start_pfn)))
> onlined_pages = online_pages_blocks(start_pfn, nr_pages);
> + t2 = local_clock();
> + trace_printk("time spend = %llu us\n", (t2-t1)/(1000));
>
> online_mem_sections(start_pfn, start_pfn + nr_pages);
Thanks ;-)
--
Oscar Salvador
SUSE L3
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-10-10 13:37, Oscar Salvador wrote:
On Fri, Oct 05, 2018 at 01:40:05PM +0530, Arun KS wrote:
When free pages are done with higher 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%. Modify external
providers of online callback to align with the change.
Hi Arun, out of curiosity:
could you please explain how exactly did you mesure the speed
improvement?
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e379e85..2416136 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -690,9 +690,13 @@ static int online_pages_range(unsigned long
start_pfn, unsigned long nr_pages,
void *arg)
{
unsigned long onlined_pages = *(unsigned long *)arg;
+ u64 t1, t2;
+ t1 = local_clock();
if (PageReserved(pfn_to_page(start_pfn)))
onlined_pages = online_pages_blocks(start_pfn,
nr_pages);
+ t2 = local_clock();
+ trace_printk("time spend = %llu us\n", (t2-t1)/(1000));
online_mem_sections(start_pfn, start_pfn + nr_pages);
Regards,
Arun
Thanks
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Fri, Oct 05, 2018 at 01:40:05PM +0530, Arun KS wrote: > When free pages are done with higher 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%. Modify external > providers of online callback to align with the change. Hi Arun, out of curiosity: could you please explain how exactly did you mesure the speed improvement? Thanks -- Oscar Salvador SUSE L3 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Tue 09-10-18 15:24:13, Arun KS wrote: > On 2018-10-09 14:59, Michal Hocko wrote: > > On Fri 05-10-18 13:40:05, Arun KS wrote: > > > When free pages are done with higher 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%. Modify external > > > providers of online callback to align with the change. > > > > Acked-by: Michal Hocko > > > > Thanks for your patience with all the resubmission. > > Hello Michal, > > I got the below email few days back. Do I still need to resubmit the patch? > or is it already on the way to merge? It is sitting in the queue for now. Andrew collects acks and reviewed-bys and add them to the patch. -- Michal Hocko SUSE Labs ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On 2018-10-09 14:59, Michal Hocko wrote: On Fri 05-10-18 13:40:05, Arun KS wrote: When free pages are done with higher 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%. Modify external providers of online callback to align with the change. Acked-by: Michal Hocko Thanks for your patience with all the resubmission. Hello Michal, I got the below email few days back. Do I still need to resubmit the patch? or is it already on the way to merge? Regards, Arun The patch titled Subject: mm/page_alloc.c: memory hotplug: free pages as higher order has been added to the -mm tree. Its filename is memory_hotplug-free-pages-as-higher-order.patch This patch should soon appear at http://ozlabs.org/~akpm/mmots/broken-out/memory_hotplug-free-pages-as-higher-order.patch and later at http://ozlabs.org/~akpm/mmotm/broken-out/memory_hotplug-free-pages-as-higher-order.patch Before you just go and hit "reply", please: a) Consider who else should be cc'ed b) Prefer to cc a suitable mailing list as well c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's *** Remember to use Documentation/process/submit-checklist.rst when testing your code *** The -mm tree is included into linux-next and is updated there every 3-4 working days ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Fri 05-10-18 13:40:05, Arun KS wrote: > When free pages are done with higher 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%. Modify external > providers of online callback to align with the change. Acked-by: Michal Hocko Thanks for your patience with all the resubmission. -- Michal Hocko SUSE Labs ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
On Fri, Oct 05, 2018 at 01:40:05PM +0530, Arun KS wrote:
> When free pages are done with higher 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%. Modify external
> providers of online callback to align with the change.
>
> Signed-off-by: Arun KS
Looks good to me.
Reviewed-by: Oscar Salvador
Just one thing below:
> @@ -1331,7 +1331,7 @@ void __init __free_pages_bootmem(struct page *page,
> unsigned long pfn,
> {
> if (early_page_uninitialised(pfn))
> return;
> - return __free_pages_boot_core(page, order);
> + return __free_pages_core(page, order);
__free_pages_core is void, so I guess we do not need that return there.
Probably the code generated is the same though.
--
Oscar Salvador
SUSE L3
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v5 1/2] memory_hotplug: Free pages as higher order
When free pages are done with higher 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%. Modify external
providers of online callback to align with the change.
Signed-off-by: Arun KS
---
Changes since v4:
- As suggested by Michal Hocko,
- Simplify logic in online_pages_block() by using get_order().
- Seperate out removal of prefetch from __free_pages_core().
Changes since v3:
- Renamed _free_pages_boot_core -> __free_pages_core.
- Removed prefetch from __free_pages_core.
- Removed xen_online_page().
Changes since v2:
- Reuse code from __free_pages_boot_core().
Changes since v1:
- Removed prefetch().
Changes since RFC:
- Rebase.
- As suggested by Michal Hocko remove pages_per_block.
- Modifed external providers of online_page_callback.
v4: https://lore.kernel.org/patchwork/patch/995111/
v3: https://lore.kernel.org/patchwork/patch/992348/
v2: https://lore.kernel.org/patchwork/patch/991363/
v1: https://lore.kernel.org/patchwork/patch/989445/
RFC: https://lore.kernel.org/patchwork/patch/984754/
---
drivers/hv/hv_balloon.c| 6 --
drivers/xen/balloon.c | 23 +++
include/linux/memory_hotplug.h | 2 +-
mm/internal.h | 1 +
mm/memory_hotplug.c| 42 ++
mm/page_alloc.c| 8
6 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index b1b7880..c5bc0b5 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -771,7 +771,7 @@ static void hv_mem_hot_add(unsigned long start, unsigned
long size,
}
}
-static void hv_online_page(struct page *pg)
+static int hv_online_page(struct page *pg, unsigned int order)
{
struct hv_hotadd_state *has;
unsigned long flags;
@@ -783,10 +783,12 @@ static void hv_online_page(struct page *pg)
if ((pfn < has->start_pfn) || (pfn >= has->end_pfn))
continue;
- hv_page_online_one(has, pg);
+ hv_bring_pgs_online(has, pfn, (1UL << order));
break;
}
spin_unlock_irqrestore(&dm_device.ha_lock, flags);
+
+ return 0;
}
static int pfn_covered(unsigned long start_pfn, unsigned long pfn_cnt)
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index e12bb25..58ddf48 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -390,8 +390,8 @@ static enum bp_state reserve_additional_memory(void)
/*
* add_memory_resource() will call online_pages() which in its turn
-* will call xen_online_page() callback causing deadlock if we don't
-* release balloon_mutex here. Unlocking here is safe because the
+* will call xen_bring_pgs_online() callback causing deadlock if we
+* don't release balloon_mutex here. Unlocking here is safe because the
* callers drop the mutex before trying again.
*/
mutex_unlock(&balloon_mutex);
@@ -411,15 +411,22 @@ static enum bp_state reserve_additional_memory(void)
return BP_ECANCELED;
}
-static void xen_online_page(struct page *page)
+static int xen_bring_pgs_online(struct page *pg, unsigned int order)
{
- __online_page_set_limits(page);
+ unsigned long i, size = (1 << order);
+ unsigned long start_pfn = page_to_pfn(pg);
+ struct page *p;
+ pr_debug("Online %lu pages starting at pfn 0x%lx\n", size, start_pfn);
mutex_lock(&balloon_mutex);
-
- __balloon_append(page);
-
+ for (i = 0; i < size; i++) {
+ p = pfn_to_page(start_pfn + i);
+ __online_page_set_limits(p);
+ __balloon_append(p);
+ }
mutex_unlock(&balloon_mutex);
+
+ return 0;
}
static int xen_memory_notifier(struct notifier_block *nb, unsigned long val,
void *v)
@@ -744,7 +751,7 @@ static int __init balloon_init(void)
balloon_stats.max_retry_count = RETRY_UNLIMITED;
#ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
- set_online_page_callback(&xen_online_page);
+ set_online_page_callback(&xen_bring_pgs_online);
register_memory_notifier(&xen_memory_nb);
register_sysctl_table(xen_root);
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 34a2822..7b04c1d 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -87,7 +87,7 @@ extern int test_pages_in_a_zone(unsigned long start_pfn,
unsigned long end_pfn,
unsigned long *valid_start, unsigned long *valid_end);
extern void __offline_isolated_pages(unsigned long, unsigned long);
-typedef void (*online_page_callback_t)(struct page *page);
+typedef int (*online_page_callback_t)(struct page *page, unsigned int order);
extern int set_online_page_c
