Messages in this thread | | | Date | Tue, 3 Oct 2023 22:38:09 +0800 | Subject | Re: [PATCH v4 2/2] mm: Init page count in reserve_bootmem_region when MEMINIT_EARLY | From | Yajun Deng <> |
| |
On 2023/10/2 19:25, David Hildenbrand wrote: > On 02.10.23 13:10, Mike Rapoport wrote: >> On Mon, Oct 02, 2023 at 10:56:51AM +0200, David Hildenbrand wrote: >>> On 02.10.23 10:47, Mike Rapoport wrote: >>>> On Mon, Oct 02, 2023 at 03:03:56PM +0800, Yajun Deng wrote: >>>>> >>>>> On 2023/10/2 02:59, Mike Rapoport wrote: >>>>>> On Fri, Sep 29, 2023 at 06:27:25PM +0800, Yajun Deng wrote: >>>>>>> On 2023/9/29 18:02, Mike Rapoport wrote: >>>>>>>>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>>>>>>>>> index 06be8821d833..b868caabe8dc 100644 >>>>>>>>>>> --- a/mm/page_alloc.c >>>>>>>>>>> +++ b/mm/page_alloc.c >>>>>>>>>>> @@ -1285,18 +1285,22 @@ void __free_pages_core(struct page >>>>>>>>>>> *page, unsigned int order) >>>>>>>>>>> unsigned int loop; >>>>>>>>>>> /* >>>>>>>>>>> - * When initializing the memmap, __init_single_page() >>>>>>>>>>> sets the refcount >>>>>>>>>>> - * of all pages to 1 ("allocated"/"not free"). We have >>>>>>>>>>> to set the >>>>>>>>>>> - * refcount of all involved pages to 0. >>>>>>>>>>> + * When initializing the memmap, memmap_init_range sets >>>>>>>>>>> the refcount >>>>>>>>>>> + * of all pages to 1 ("reserved" and "free") in hotplug >>>>>>>>>>> context. We >>>>>>>>>>> + * have to set the refcount of all involved pages to 0. >>>>>>>>>>> Otherwise, >>>>>>>>>>> + * we don't do it, as reserve_bootmem_region only set >>>>>>>>>>> the refcount on >>>>>>>>>>> + * reserve region ("reserved") in early context. >>>>>>>>>>> */ >>>>>>>>>> Again, why hotplug and early init should be different? >>>>>>>>> I will add a comment that describes it will save boot time. >>>>>>>> But why do we need initialize struct pages differently at boot >>>>>>>> time vs >>>>>>>> memory hotplug? >>>>>>>> Is there a reason memory hotplug cannot have page count set to >>>>>>>> 0 just like >>>>>>>> for pages reserved at boot time? >>>>>>> This patch just save boot time in MEMINIT_EARLY. If someone >>>>>>> finds out that >>>>>>> it can save time in >>>>>>> >>>>>>> MEMINIT_HOTPLUG, I think it can be done in another patch later. >>>>>>> I just >>>>>>> keeping it in the same. >>>>>> But it's not the same. It becomes slower after your patch and the >>>>>> code that >>>>>> frees the pages for MEMINIT_EARLY and MEMINIT_HOTPLUG becomes >>>>>> non-uniform >>>>>> for no apparent reason. >>>>> >>>>> __free_pages_core will also be called by others, such as: >>>>> deferred_free_range, do_collection and memblock_free_late. >>>>> >>>>> We couldn't remove 'if (page_count(page))' even if we set page >>>>> count to 0 >>>>> when MEMINIT_HOTPLUG. >>>> >>>> That 'if' breaks the invariant that __free_pages_core is always >>>> called for >>>> pages with initialized page count. Adding it may lead to subtle >>>> bugs and >>>> random memory corruption so we don't want to add it at the first >>>> place. >>> >>> As long as we have to special-case memory hotplug, we know that we are >>> always coming via generic_online_page() in that case. We could >>> either move >>> some logic over there, or let __free_pages_core() know what it >>> should do. >> >> Looks like the patch rather special cases MEMINIT_EARLY, although I >> didn't >> check throughfully other code paths. >> Anyway, relying on page_count() to be correct in different ways for >> different callers of __free_pages_core() does not sound right to me. > > Absolutely agreed. > I already sent v5 a few days ago. Comments, please...
| |