On 25 Aug 2022 09:11:29 +0200, mhocko@xxxxxxxx wrote:
>On Thu 25-08-22 14:31:02, lizhe.67@xxxxxxxxxxxxx wrote:
>> From: Li Zhe <lizhe.67@xxxxxxxxxxxxx>
>>
>> In 'commit 2f1ee0913ce5 ("Revert "mm: use early_pfn_to_nid in page_ext_init"")',
>> we call page_ext_init() after page_alloc_init_late() to avoid some panic
>> problem. It seems that we cannot track early page allocations in current
>> kernel even if page structure has been initialized early.
>>
>> This patch introduce a new boot parameter 'early_page_ext' to resolve this
>> problem. If we pass it to kernel, function page_ext_init() will be moved
>> up and feature 'deferred initialization of struct pages' will be disabled.
>
>will be disabled to initialize the page allocator early and prevent from
>the OOM mentioned above.
>
>> It can help us to catch early page allocations. This is useful especially
>> when we find that the free memory value is not the same right after
>> different kernel booting.
>>
>> Changelogs:
>>
>> v1->v2:
>> - use a cmd line parameter to move up function page_ext_init() instead of
>> using CONFIG_DEFERRED_STRUCT_PAGE_INIT
>> - fix oom problem[1]
>>
>> v2->v3:
>> - make adjustments suggested by Michal Hocko
>>
>> v1 patch: https://lore.kernel.org/lkml/Yv3r6Y1vh+6AbY4+@xxxxxxxxxxxxxx/T/
>> v2 patch: https://lore.kernel.org/lkml/20220824065058.81051-1-lizhe.67@xxxxxxxxxxxxx/T/
>>
>> [1]: https://lore.kernel.org/linux-mm/YwHmXLu5txij+p35@xsang-OptiPlex-9020/
>
>the changelog is usually not part of the changelog and goes under ---
Thanks for correcting my mistake!
>>
>> Suggested-by: Michal Hocko <mhocko@xxxxxxxx>
>> Signed-off-by: Li Zhe <lizhe.67@xxxxxxxxxxxxx>
>
>I still have few comments below before I am going to ack. But this looks
>much better already.
>
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 6 ++++++
>> include/linux/page_ext.h | 11 +++++++++++
>> init/main.c | 6 +++++-
>> mm/page_alloc.c | 2 ++
>> mm/page_ext.c | 12 ++++++++++++
>> 5 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index d7f30902fda0..7b5726828ac0 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1471,6 +1471,12 @@
>> Permit 'security.evm' to be updated regardless of
>> current integrity status.
>>
>> + early_page_ext [KNL] Boot-time early page_ext initializing option.
>> + This boot parameter disables the deferred initialization
>> + of struct page and move up function page_ext_init() in
>> + order to catch early page allocations. Available with
>> + CONFIG_PAGE_EXTENSION=y.
>
>For admins it would likely be more easier to understand something like
>following
> early_page_ext [KNL] Enforces page_ext initialization to earlier
> stages so cover more early boot allocations.
> Please note that as side effect some
> optimizations might be disabled to achieve that
> (e.g. parallelized memory initialization is
> disabled) so the boot process might take longer,
> especially on systems with a lot of memory.
> Available with CONFIG_PAGE_EXTENSION=y
Great, I will use this description in my v4 patch. It is much more easier
for us to understand. Thanks!
>[...]
>> diff --git a/mm/page_ext.c b/mm/page_ext.c
>> index 3dc715d7ac29..bf4f2a12d7dc 100644
>> --- a/mm/page_ext.c
>> +++ b/mm/page_ext.c
>> @@ -85,6 +85,18 @@ unsigned long page_ext_size = sizeof(struct page_ext);
>>
>> static unsigned long total_usage;
>>
>> +#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
>> +bool early_page_ext __meminitdata;
>> +#else
>> +bool early_page_ext __meminitdata = true;
>> +#endif
>
>Why should default depend on DEFERRED_STRUCT_PAGE_INIT at all. This is
>just confusing and I do not see how it serves a purpose. We might grow
>more optimizations which would prefent early page_ext init.
>
>Let's just have default false and only enforce with the parameter. This
>is more predictable and easier to understand.
Yes, this is confusing. Without depending on DEFERRED_STRUCT_PAGE_INIT, the
logic here will be more clear. I will remove it from my v4 patch. Thanks!
