Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   lizhe.67@bytedance.com
To:     mhocko@suse.com
Cc:     Jason@zx2c4.com, akpm@linux-foundation.org, corbet@lwn.net,
        keescook@chromium.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        lizefan.x@bytedance.com, lizhe.67@bytedance.com,
        mark-pk.tsai@mediatek.com, mhiramat@kernel.org,
        rostedt@goodmis.org, vbabka@suse.cz, yuanzhu@bytedance.com
Subject: Re: [PATCH v2] page_ext: introduce boot parameter 'early_page_ext'
Date:   Thu, 25 Aug 2022 14:21:29 +0800
Message-Id: <20220825062129.92091-1-lizhe.67@bytedance.com>
In-Reply-To: <YwXczj8Dh0uI0EA9@dhcp22.suse.cz>
References: <YwXczj8Dh0uI0EA9@dhcp22.suse.cz>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2022-08-24 8:09 UTC, mhocko@suse.com wrote:
>On Wed 24-08-22 14:50:58, lizhe.67@bytedance.com wrote:
>[...]
>> diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
>> index fabb2e1e087f..3e081cf8a1ec 100644
>> --- a/include/linux/page_ext.h
>> +++ b/include/linux/page_ext.h
>> @@ -38,19 +38,22 @@ struct page_ext {
>>  
>>  extern unsigned long page_ext_size;
>>  extern void pgdat_page_ext_init(struct pglist_data *pgdat);
>> +#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
>> +extern bool early_page_ext_enable(void);
>> +#endif
>>  
>>  #ifdef CONFIG_SPARSEMEM
>>  static inline void page_ext_init_flatmem(void)
>>  {
>>  }
>> -extern void page_ext_init(void);
>> +extern void page_ext_init(bool early);
>>  static inline void page_ext_init_flatmem_late(void)
>>  {
>>  }
>>  #else
>>  extern void page_ext_init_flatmem(void);
>>  extern void page_ext_init_flatmem_late(void);
>> -static inline void page_ext_init(void)
>> +static inline void page_ext_init(bool early)
>>  {
>>  }
>>  #endif
>
>Why do you need to make it CONFIG_DEFERRED_STRUCT_PAGE_INIT
>dependant?

Yes it is unnecessary. Thanks!

>[...]
>> diff --git a/init/main.c b/init/main.c
>> index 91642a4e69be..3760c0326525 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -849,6 +849,8 @@ static void __init mm_init(void)
>>  	pgtable_init();
>>  	debug_objects_mem_init();
>>  	vmalloc_init();
>> +	/* Should be run after vmap initialization */
>> +	page_ext_init(true);
>
>you can just 
>	if (early_page_ext)
>		page_ext_init();
>
>>  	/* Should be run before the first non-init thread is created */
>>  	init_espfix_bsp();
>>  	/* Should be run after espfix64 is set up. */
>> @@ -1606,7 +1608,7 @@ static noinline void __init kernel_init_freeable(void)
>>  	padata_init();
>>  	page_alloc_init_late();
>>  	/* Initialize page ext after all struct pages are initialized. */
>> -	page_ext_init();
>> +	page_ext_init(false);
>
>	if (!early_page_ext)
>		page_ext_init();

I think we can use an inline function instead of 'early_page_ext' here. The
reason is that if CONFIG_PAGE_EXTENSION=n, 'early_page_ext' is undefined.
Thought we can #define early_page_ext as false, it is ugly.

>>  
>>  	do_basic_setup();
>>  
>
>and without the ifdefery it all becomes much more simple.

Yes it becomes much more simple. Thanks for all your advices! I will take your
advices and send a v3 patch.