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

[Wei Yang <richard.weiyang@gmail.com>]

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.
>> > > > > 
[...]
>> > > > 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.
>
>So maybe it's not a big concern and this separate totalram pages accounting
>is much rather some legacy leftover.
>

I grepped the whole tree and found following 4 points may need to adjust. Hope
I don't miss one.

   * arch/s390/mm/init.c:73:	while (order > 2 && (totalram_pages() >> 10) < (1UL << order))
   * arch/um/kernel/mem.c:76:	max_low_pfn = totalram_pages();
   * kernel/fork.c:1002:	unsigned long nr_pages = totalram_pages();
   * mm/mm_init.c:2689:		K(physpages - totalram_pages() - totalcma_pages),

* arch/s390/mm/init.c:73:	while (order > 2 && (totalram_pages() >> 10) < (1UL << order))

    mem_init
        memblock_free_all
        setup_zero_pages

This calculate the size of empty_zero_page. Not sure if we can postpone this
function call after defer init.

* arch/um/kernel/mem.c:76:	max_low_pfn = totalram_pages();

    mem_init
        memblock_free_all
        max_low_pfn = totalram_pages

The usage seems not correct. totalram_pages return number of pages, but here
it seems need the pfn value.

* kernel/fork.c:1002:	unsigned long nr_pages = totalram_pages();

    start_kernel
        fork_init
            set_max_threads

Not sure it would be fine to set rlimit again after defer init.

* mm/mm_init.c:2689:		K(physpages - totalram_pages() - totalcma_pages),

Per my understanding, we can print the info after defer init.

>> 
>> > Further, we currently require only a single atomic RMW instruction to adjust
>> > totalram during boot, moving it to __free_pages_core() would imply more
>> > atomics: but usually only one per MAX_ORDER page, so I doubt this would make
>> > a big difference.
>> > 
>> 
>> I took a rough calculation on this.One MAX_ORDER page accounts for 2MB, and
>> with defer_init only low zone's memory is initialized during boot. Per my
>> understanding, low zone's memory is 4GB for x86. So the extra calculation is
>> 4GB / 2MB = 2K.
>
>Well, for all deferred-initialized memory you would now also require these --
>or if deferred-init would be disabled. Sounds like an interesting measurement
>if that would be measurable at all.
>
>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me