Message-ID: <248221a5-5ff6-479a-ab23-96ce357d1b16@quicinc.com>
Date: Tue, 7 Jan 2025 15:38:03 +0800
From: Zhenhua Huang <quic_zhenhuah@...cinc.com>
To: Anshuman Khandual <anshuman.khandual@....com>, <catalin.marinas@....com>
CC: <will@...nel.org>, <ardb@...nel.org>, <ryan.roberts@....com>,
        <mark.rutland@....com>, <joey.gouly@....com>,
        <dave.hansen@...ux.intel.com>, <akpm@...ux-foundation.org>,
        <chenfeiyang@...ngson.cn>, <chenhuacai@...nel.org>,
        <linux-mm@...ck.org>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <quic_tingweiz@...cinc.com>,
        <stable@...r.kernel.org>
Subject: Re: [PATCH v3] arm64: mm: Populate vmemmap at the page level for
 hotplugged sections

Thanks Anshuman for your detailed review!

On 2025/1/6 23:11, Anshuman Khandual wrote:
> Hello Zhenhua,
> 
> On 1/3/25 14:20, Zhenhua Huang wrote:
>> Commit c1cc1552616d ("arm64: MMU initialisation") optimizes the
>> vmemmap to populate at the PMD section level which was suitable
>> initially since hotplugging granule is always 128M. However,
> 
> A small nit s/hotplugging/hot plug/
> 
> Also please do mention that 128M is SECTION_SIZE_BITS == 27 on arm64
> platform for 4K base pages which is the page size in context here.
> 
> 
>> commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
>> which added 2M hotplugging granule disrupted the arm64 assumptions.
> 
> A small nit s/hotplugging/hot plug/
> 
> Also please do mention that 2M is SUB_SECTION_SIZE.
> 
>>
>> Considering the vmemmap_free -> unmap_hotplug_pmd_range path, when
>> pmd_sect() is true, the entire PMD section is cleared, even if there is
>> other effective subsection. For example pagemap1 and pagemap2 are part
>> of a single PMD entry and they are hot-added sequentially. Then pagemap1
>> is removed, vmemmap_free() will clear the entire PMD entry freeing the
>> struct page metadata for the whole section, even though pagemap2 is still
>> active.
> 
> I guess pagemap1/2 here indicates the struct pages virtual regions for two
> different vmemmap mapped sub sections covered via a single PMD entry ? But
> please do update the above paragraph appropriately because pagemap<N> might
> be confused with /proc/<pid>/pagemap mechanism.
> 
> Also please do mention that similar problems exist with linear mapping for
> 16K (PMD = 32M) and 64K (PMD = 512M) base pages as their block mappings
> exceed SUBSECTION_SIZE. Hence tearing down the entire PMD mapping too will
> leave other subsections unmapped in the linear mapping.

Make sense to me, will update comments.

> 
>>
>> To address the issue, we need to prevent PMD/PUD/CONT mappings for both
>> linear and vmemmap for non-boot sections if the size exceeds 2MB
> 
> s/if the size/if corresponding size on the given base page/
> 
>> (considering sub-section is 2MB). We only permit 2MB blocks in a 4KB page
>> configuration.
> 
> PMD block in 4K page size config as it's PMD_SIZE matches the SUBSECTION_SIZE
> but only for linear mapping.
> 
> 
>>
>> Cc: stable@...r.kernel.org # v5.4+
>> Fixes: ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@...cinc.com>
>> ---
>> Hi Catalin and Anshuman,
>> Based on your review comments, I concluded below patch and tested with my setup.
>> I have not folded patchset #2 since this patch seems to be enough for backporting..
>> Please see if you have further suggestions.
>>
>>   arch/arm64/mm/mmu.c | 33 +++++++++++++++++++++++++++++----
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index e2739b69e11b..2b4d23f01d85 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -42,9 +42,11 @@
>>   #include <asm/pgalloc.h>
>>   #include <asm/kfence.h>
>>   
>> -#define NO_BLOCK_MAPPINGS	BIT(0)
>> +#define NO_PMD_BLOCK_MAPPINGS	BIT(0)
>>   #define NO_CONT_MAPPINGS	BIT(1)
>>   #define NO_EXEC_MAPPINGS	BIT(2)	/* assumes FEAT_HPDS is not used */
>> +#define NO_PUD_BLOCK_MAPPINGS	BIT(3)  /* Hotplug case: do not want block mapping for PUD */
>> +#define NO_BLOCK_MAPPINGS (NO_PMD_BLOCK_MAPPINGS | NO_PUD_BLOCK_MAPPINGS)
> 
> 
> Should not NO_PMD_BLOCK_MAPPINGS and NO_PUD_BLOCK_MAPPINGS be adjacent bits
> for better readability ? But that will also cause some additional churn.

Agree, I see these macros are only used in this file, it's safe to update.

> 
>>   
>>   u64 kimage_voffset __ro_after_init;
>>   EXPORT_SYMBOL(kimage_voffset);
>> @@ -254,7 +256,7 @@ static void init_pmd(pmd_t *pmdp, unsigned long addr, unsigned long end,
>>   
>>   		/* try section mapping first */
>>   		if (((addr | next | phys) & ~PMD_MASK) == 0 &&
>> -		    (flags & NO_BLOCK_MAPPINGS) == 0) {
>> +		    (flags & NO_PMD_BLOCK_MAPPINGS) == 0) {
> 
> Behavior will remain unchanged for all existing users of NO_BLOCK_MAPPINGS
> as it will now contain NO_PMD_BLOCK_MAPPINGS.

Hmm... do you want me to include these in comments? The code appears to 
be clear as it is.

> 
>>   			pmd_set_huge(pmdp, phys, prot);
>>   
>>   			/*
>> @@ -356,10 +358,11 @@ static void alloc_init_pud(p4d_t *p4dp, unsigned long addr, unsigned long end,
>>   
>>   		/*
>>   		 * For 4K granule only, attempt to put down a 1GB block
>> +		 * Hotplug case: do not attempt 1GB block
>>   		 */
>>   		if (pud_sect_supported() &&
>>   		   ((addr | next | phys) & ~PUD_MASK) == 0 &&
>> -		    (flags & NO_BLOCK_MAPPINGS) == 0) {
>> +		   (flags & NO_PUD_BLOCK_MAPPINGS) == 0) {
> 
> Behavior will remain unchanged for all existing users of NO_BLOCK_MAPPINGS
> as it will now contain NO_PUD_BLOCK_MAPPINGS.
> 

Ditto.

>>   			pud_set_huge(pudp, phys, prot);
>>   
>>   			/*
>> @@ -1175,9 +1178,16 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, int node,
>>   int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>>   		struct vmem_altmap *altmap)
>>   {
>> +	unsigned long start_pfn;
>> +	struct mem_section *ms;
>> +
>>   	WARN_ON((start < VMEMMAP_START) || (end > VMEMMAP_END));
>>   
>> -	if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>> +	start_pfn = page_to_pfn((struct page *)start);
>> +	ms = __pfn_to_section(start_pfn);
>> +
>> +	/* Hotplugged section not support hugepages */
> 
> Please update the comment as
> 
> 	/*
> 	 * Hotplugged section does not support hugepages as
> 	 * PMD_SIZE (hence PUD_SIZE) section mapping covers
> 	 * struct page range that exceeds a SUBSECTION_SIZE
> 	 * i.e 2MB - for all available base page sizes.
> 	 */
> 
>> +	if (!IS_ENABLED(CONFIG_ARM64_4K_PAGES) || !early_section(ms))
>>   		return vmemmap_populate_basepages(start, end, node, altmap);
>>   	else
>>   		return vmemmap_populate_hugepages(start, end, node, altmap);
>> @@ -1339,9 +1349,24 @@ int arch_add_memory(int nid, u64 start, u64 size,
>>   		    struct mhp_params *params)
>>   {
>>   	int ret, flags = NO_EXEC_MAPPINGS;
>> +	unsigned long start_pfn = page_to_pfn((struct page *)start);
>> +	struct mem_section *ms = __pfn_to_section(start_pfn);
>>   
>>   	VM_BUG_ON(!mhp_range_allowed(start, size, true));
>>   
>> +	/* Should not be invoked by early section */
>> +	WARN_ON(early_section(ms));
>> +
>> +	if (IS_ENABLED(CONFIG_ARM64_4K_PAGES))
>> +	/*
>> +	 * As per subsection granule is 2M, allow PMD block mapping in
>> +	 * case 4K PAGES.
>> +	 * Other cases forbid section mapping.
>> +	 */
> 
> IIUC subsection size is 2M regardless of the page size. But with 4K pages
> on arm64, PMD_SIZE happen to be 2M. Hence there is no problem in creating
> linear mappings at PMD block level, which will not be the case with other
> page sizes i.e 16K and 64K.

Yes.

> 
> include/linux/mmzone.h
> 
> #define SUBSECTION_SHIFT 21
> #define SUBSECTION_SIZE (1UL << SUBSECTION_SHIFT)
> 
> Please update the comment with following changes but above IS_ENABLED()
> statement as it talks about all page size configs.
> 
> 	/*
> 	 * 4K base page's PMD_SIZE matches SUBSECTION_SIZE i.e 2MB. Hence
> 	 * PMD section mapping can be allowed, but only for 4K base pages.
> 	 * Where as PMD_SIZE (hence PUD_SIZE) for other page sizes exceed
> 	 * SUBSECTION_SIZE.
> 	 */

Nice comments! Thanks.

>> +		flags |= NO_PUD_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>> +	else
>> +		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>> +
>>   	if (can_set_direct_map())
>>   		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>   
> 

Powered by blists - more mailing lists