From: Steven Price <steven.price@arm.com> To: Sudarshan Rajagopalan <sudaraja@codeaurora.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Anshuman Khandual <anshuman.khandual@arm.com>, Mark Rutland <mark.rutland@arm.com>, Logan Gunthorpe <logang@deltatee.com>, David Hildenbrand <david@redhat.com>, Andrew Morton <akpm@linux-foundation.org> Subject: Re: [PATCH] arm64/mm: add fallback option to allocate virtually contiguous memory Date: Thu, 10 Sep 2020 09:27:43 +0100 [thread overview] Message-ID: <b8c6e11f-00f3-a89c-6ebc-eef55f92298b@arm.com> (raw) In-Reply-To: <01010174769e2b68-a6f3768e-aef8-43c7-b357-a8cb1e17d3eb-000000@us-west-2.amazonses.com> On 10/09/2020 07:05, Sudarshan Rajagopalan wrote: > When section mappings are enabled, we allocate vmemmap pages from physically > continuous memory of size PMD_SZIE using vmemmap_alloc_block_buf(). Section > mappings are good to reduce TLB pressure. But when system is highly fragmented > and memory blocks are being hot-added at runtime, its possible that such > physically continuous memory allocations can fail. Rather than failing the > memory hot-add procedure, add a fallback option to allocate vmemmap pages from > discontinuous pages using vmemmap_populate_basepages(). > > Signed-off-by: Sudarshan Rajagopalan <sudaraja@codeaurora.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Logan Gunthorpe <logang@deltatee.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Steven Price <steven.price@arm.com> > --- > arch/arm64/mm/mmu.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 75df62f..a46c7d4 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -1100,6 +1100,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, > p4d_t *p4dp; > pud_t *pudp; > pmd_t *pmdp; > + int ret = 0; > > do { > next = pmd_addr_end(addr, end); > @@ -1121,15 +1122,23 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, > void *p = NULL; > > p = vmemmap_alloc_block_buf(PMD_SIZE, node, altmap); > - if (!p) > - return -ENOMEM; > + if (!p) { > +#ifdef CONFIG_MEMORY_HOTPLUG > + vmemmap_free(start, end, altmap); > +#endif > + ret = -ENOMEM; > + break; > + } > > pmd_set_huge(pmdp, __pa(p), __pgprot(PROT_SECT_NORMAL)); > } else > vmemmap_verify((pte_t *)pmdp, node, addr, next); > } while (addr = next, addr != end); > > - return 0; > + if (ret) > + return vmemmap_populate_basepages(start, end, node, altmap); > + else > + return ret; Style comment: I find this usage of 'ret' confusing. When we assign -ENOMEM above that is never actually the return value of the function (in that case vmemmap_populate_basepages() provides the actual return value). Also the "return ret" is misleading since we know by that point that ret==0 (and the 'else' is redundant). Can you not just move the call to vmemmap_populate_basepages() up to just after the (possible) vmemmap_free() call and remove the 'ret' variable? AFAICT the call to vmemmap_free() also doesn't need the #ifdef as the function is a no-op if CONFIG_MEMORY_HOTPLUG isn't set. I also feel you need at least a comment to explain Anshuman's point that it looks like you're freeing an unmapped area. Although if I'm reading the code correctly it seems like the unmapped area will just be skipped. Steve > } > #endif /* !ARM64_SWAPPER_USES_SECTION_MAPS */ > void vmemmap_free(unsigned long start, unsigned long end, >
WARNING: multiple messages have this Message-ID (diff)
From: Steven Price <steven.price@arm.com> To: Sudarshan Rajagopalan <sudaraja@codeaurora.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Mark Rutland <mark.rutland@arm.com>, Will Deacon <will@kernel.org>, David Hildenbrand <david@redhat.com>, Catalin Marinas <catalin.marinas@arm.com>, Anshuman Khandual <anshuman.khandual@arm.com>, Andrew Morton <akpm@linux-foundation.org>, Logan Gunthorpe <logang@deltatee.com> Subject: Re: [PATCH] arm64/mm: add fallback option to allocate virtually contiguous memory Date: Thu, 10 Sep 2020 09:27:43 +0100 [thread overview] Message-ID: <b8c6e11f-00f3-a89c-6ebc-eef55f92298b@arm.com> (raw) In-Reply-To: <01010174769e2b68-a6f3768e-aef8-43c7-b357-a8cb1e17d3eb-000000@us-west-2.amazonses.com> On 10/09/2020 07:05, Sudarshan Rajagopalan wrote: > When section mappings are enabled, we allocate vmemmap pages from physically > continuous memory of size PMD_SZIE using vmemmap_alloc_block_buf(). Section > mappings are good to reduce TLB pressure. But when system is highly fragmented > and memory blocks are being hot-added at runtime, its possible that such > physically continuous memory allocations can fail. Rather than failing the > memory hot-add procedure, add a fallback option to allocate vmemmap pages from > discontinuous pages using vmemmap_populate_basepages(). > > Signed-off-by: Sudarshan Rajagopalan <sudaraja@codeaurora.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Anshuman Khandual <anshuman.khandual@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Logan Gunthorpe <logang@deltatee.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Steven Price <steven.price@arm.com> > --- > arch/arm64/mm/mmu.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 75df62f..a46c7d4 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -1100,6 +1100,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, > p4d_t *p4dp; > pud_t *pudp; > pmd_t *pmdp; > + int ret = 0; > > do { > next = pmd_addr_end(addr, end); > @@ -1121,15 +1122,23 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, > void *p = NULL; > > p = vmemmap_alloc_block_buf(PMD_SIZE, node, altmap); > - if (!p) > - return -ENOMEM; > + if (!p) { > +#ifdef CONFIG_MEMORY_HOTPLUG > + vmemmap_free(start, end, altmap); > +#endif > + ret = -ENOMEM; > + break; > + } > > pmd_set_huge(pmdp, __pa(p), __pgprot(PROT_SECT_NORMAL)); > } else > vmemmap_verify((pte_t *)pmdp, node, addr, next); > } while (addr = next, addr != end); > > - return 0; > + if (ret) > + return vmemmap_populate_basepages(start, end, node, altmap); > + else > + return ret; Style comment: I find this usage of 'ret' confusing. When we assign -ENOMEM above that is never actually the return value of the function (in that case vmemmap_populate_basepages() provides the actual return value). Also the "return ret" is misleading since we know by that point that ret==0 (and the 'else' is redundant). Can you not just move the call to vmemmap_populate_basepages() up to just after the (possible) vmemmap_free() call and remove the 'ret' variable? AFAICT the call to vmemmap_free() also doesn't need the #ifdef as the function is a no-op if CONFIG_MEMORY_HOTPLUG isn't set. I also feel you need at least a comment to explain Anshuman's point that it looks like you're freeing an unmapped area. Although if I'm reading the code correctly it seems like the unmapped area will just be skipped. Steve > } > #endif /* !ARM64_SWAPPER_USES_SECTION_MAPS */ > void vmemmap_free(unsigned long start, unsigned long end, > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-10 8:30 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <01010174769e2b68-a6f3768e-aef8-43c7-b357-a8cb1e17d3eb-000000@us-west-2.amazonses.com> 2020-09-10 6:45 ` [PATCH] arm64/mm: add fallback option to allocate virtually contiguous memory Anshuman Khandual 2020-09-10 6:45 ` Anshuman Khandual 2020-09-10 8:08 ` David Hildenbrand 2020-09-10 8:08 ` David Hildenbrand 2020-09-10 10:58 ` Anshuman Khandual 2020-09-10 10:58 ` Anshuman Khandual 2020-09-10 8:27 ` sudaraja 2020-09-10 11:16 ` Anshuman Khandual 2020-09-10 11:16 ` Anshuman Khandual 2020-09-10 8:27 ` sudaraja 2020-09-10 8:27 ` Steven Price [this message] 2020-09-10 8:27 ` Steven Price 2020-09-10 10:50 ` Anshuman Khandual 2020-09-10 10:50 ` Anshuman Khandual 2020-09-10 20:48 ` sudaraja 2020-09-21 17:43 ` Will Deacon 2020-09-21 17:43 ` Will Deacon 2020-09-10 20:48 ` sudaraja 2020-09-10 6:05 Sudarshan Rajagopalan -- strict thread matches above, loose matches on Subject: below -- 2020-09-10 6:05 Sudarshan Rajagopalan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b8c6e11f-00f3-a89c-6ebc-eef55f92298b@arm.com \ --to=steven.price@arm.com \ --cc=akpm@linux-foundation.org \ --cc=anshuman.khandual@arm.com \ --cc=catalin.marinas@arm.com \ --cc=david@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=logang@deltatee.com \ --cc=mark.rutland@arm.com \ --cc=sudaraja@codeaurora.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.