Re: [PATCH] mm: increase totalram_pages on freeing to buddy system

[David Hildenbrand <david@redhat.com>]

On 06.06.24 00:44, Wei Yang wrote:
> On Mon, Jun 03, 2024 at 10:43:51PM +0200, David Hildenbrand wrote:
>> On 03.06.24 22:01, Wei Yang wrote:
>>> On Mon, Jun 03, 2024 at 10:55:10AM +0200, David Hildenbrand wrote:
>>>> On 02.06.24 02:58, Wei Yang wrote:
>>>>> On Sat, Jun 01, 2024 at 06:15:33PM +0200, David Hildenbrand wrote:
>>>>>> On 01.06.24 17:32, David Hildenbrand wrote:
>>>>>>> On 01.06.24 15:34, Wei Yang wrote:
>>>>>>>> Total memory represents pages managed by buddy system.
>>>>>>>
>>>>>>> No, that's managed pages.
>>>>>>>
>>>>>>>> After the
>>>>>>>> introduction of DEFERRED_STRUCT_PAGE_INIT, it may count the pages before
>>>>>>>> being managed.
>>>>>>>>
>>>>>>>
>>>>>>> I recall one reason that is done, so other subsystem know the total
>>>>>>> memory size even before deferred init is done.
>>>>>>>
>>>>>>>> free_low_memory_core_early() returns number of pages for all free pages,
>>>>>>>> even at this moment only early initialized pages are freed to buddy
>>>>>>>> system. This means the total memory at this moment is not correct.
>>>>>>>>
>>>>>>>> Let's increase it when pages are freed to buddy system.
>>>>>>>
>>>>>>> I'm missing the "why", and the very first sentence of this patch is wrong.
>>>>>>
>>>>>> Correction: your statement was correct :) That's why
>>>>>> adjust_managed_page_count() adjusts that as well.
>>>>>>
>>>>>> __free_pages_core() only adjusts managed page count, because it assumes
>>>>>> totalram has already been adjusted early during boot.
>>>>>>
>>>>>> The reason we have this split for now, I think, is because of subsystems that
>>>>>> call totalram_pages() during init.
>>>>>>
>>>>>> So the "why" question remains, because this change has the potential to break
>>>>>> other stuff.
>>>>>>
>>>>>
>>>>> Thanks, I didn't notice this.
>>>>
>>>> I think having your cleanup would be very nice, as I have patches in the
>>>> works that would benefit from being able to move the totalram update from
>>>> memory hotplug code to __free_pages_core().
>>>>
>>>
>>> I got the same feeling.
>>>
>>>> We'd have to make sure that no code relies on totalram being sane/fixed
>>>> during boot for the initial memory. I think right now we might have such
>>>> code.
>>>>
>>>
>>> One concern is totalram would change when hotplug is enabled. That sounds
>>> those codes should do some re-calculation after totalram changes?
>>
>> We don't have such code in place -- there were discussions regarding that
>> recently.
>>
>> It would be reasonable to take a look at all totalram_pages() users and
>> determine if they could be affected by deferring updating it.
>>
>> At least page_alloc_init_late()->deferred_init_memmap() happens before
>> do_basic_setup()->do_initcalls(), which is good.
>>
> 
> But deferred_init_memmap() will spawn threads to do the work. I am afraid
> do_initcalls() won't wait for the completion of defer_init? Do I miss
> something?

Don't we wait for them to finish?

/* Block until all are initialised */
wait_for_completion(&pgdat_init_all_done_comp);

-- 
Cheers,

David / dhildenb