From: Dan Williams <dan.j.williams@intel.com> To: Joao Martins <joao.m.martins@oracle.com> Cc: Linux MM <linux-mm@kvack.org>, Ira Weiny <ira.weiny@intel.com>, linux-nvdimm <linux-nvdimm@lists.01.org>, Matthew Wilcox <willy@infradead.org>, Jason Gunthorpe <jgg@ziepe.ca>, Jane Chu <jane.chu@oracle.com>, Muchun Song <songmuchun@bytedance.com>, Mike Kravetz <mike.kravetz@oracle.com>, Andrew Morton <akpm@linux-foundation.org> Subject: Re: [PATCH RFC 3/9] sparse-vmemmap: Reuse vmemmap areas for a given page size Date: Fri, 19 Feb 2021 19:34:05 -0800 [thread overview] Message-ID: <CAPcyv4ixD6_KEpuXkpeH6JNLvoahch8rm8J55o1B97ghrtcbjQ@mail.gmail.com> (raw) In-Reply-To: <20201208172901.17384-5-joao.m.martins@oracle.com> On Tue, Dec 8, 2020 at 9:32 AM Joao Martins <joao.m.martins@oracle.com> wrote: > > Introduce a new flag, MEMHP_REUSE_VMEMMAP, which signals that > struct pages are onlined with a given alignment, and should reuse the > tail pages vmemmap areas. On that circunstamce we reuse the PFN backing s/On that circunstamce we reuse/Reuse/ Kills a "we" and switches to imperative tense. I noticed a couple other "we"s in the previous patches, but this crossed my threshold to make a comment. > only the tail pages subsections, while letting the head page PFN remain > different. This presumes that the backing page structs are compound > pages, such as the case for compound pagemaps (i.e. ZONE_DEVICE with > PGMAP_COMPOUND set) > > On 2M compound pagemaps, it lets us save 6 pages out of the 8 necessary s/lets us save/saves/ > PFNs necessary s/8 necessary PFNs necessary/8 PFNs necessary/ > to describe the subsection's 32K struct pages we are > onlining. s/we are onlining/being mapped/ ...because ZONE_DEVICE pages are never "onlined". > On a 1G compound pagemap it let us save 4096 pages. s/lets us save/saves/ > > Sections are 128M (or bigger/smaller), Huh? > and such when initializing a > compound memory map where we are initializing compound struct pages, we > need to preserve the tail page to be reused across the rest of the areas > for pagesizes which bigger than a section. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > I wonder, rather than separating vmem_context and mhp_params, that > one would just pick the latter. Albeit semantically the ctx aren't > necessarily paramters, context passed from multiple sections onlining > (i.e. multiple calls to populate_section_memmap). Also provided that > this is internal state, which isn't passed to external modules, except > @align and @flags for page size and requesting whether to reuse tail > page areas. > --- > include/linux/memory_hotplug.h | 10 ++++ > include/linux/mm.h | 2 +- > mm/memory_hotplug.c | 12 ++++- > mm/memremap.c | 3 ++ > mm/sparse-vmemmap.c | 93 ++++++++++++++++++++++++++++------ > 5 files changed, 103 insertions(+), 17 deletions(-) > > diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h > index 73f8bcbb58a4..e15bb82805a3 100644 > --- a/include/linux/memory_hotplug.h > +++ b/include/linux/memory_hotplug.h > @@ -70,6 +70,10 @@ typedef int __bitwise mhp_t; > */ > #define MEMHP_MERGE_RESOURCE ((__force mhp_t)BIT(0)) > > +/* > + */ > +#define MEMHP_REUSE_VMEMMAP ((__force mhp_t)BIT(1)) > + > /* > * Extended parameters for memory hotplug: > * altmap: alternative allocator for memmap array (optional) > @@ -79,10 +83,16 @@ typedef int __bitwise mhp_t; > struct mhp_params { > struct vmem_altmap *altmap; > pgprot_t pgprot; > + unsigned int align; > + mhp_t flags; > }; > > struct vmem_context { > struct vmem_altmap *altmap; > + mhp_t flags; > + unsigned int align; > + void *block; > + unsigned long block_page; > }; > > /* > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 2eb44318bb2d..8b0155441835 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3006,7 +3006,7 @@ p4d_t *vmemmap_p4d_populate(pgd_t *pgd, unsigned long addr, int node); > pud_t *vmemmap_pud_populate(p4d_t *p4d, unsigned long addr, int node); > pmd_t *vmemmap_pmd_populate(pud_t *pud, unsigned long addr, int node); > pte_t *vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, int node, > - struct vmem_altmap *altmap); > + struct vmem_altmap *altmap, void *block); > void *vmemmap_alloc_block(unsigned long size, int node); > struct vmem_altmap; > void *vmemmap_alloc_block_buf(unsigned long size, int node, > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index f8870c53fe5e..56121dfcc44b 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -300,6 +300,14 @@ static int check_hotplug_memory_addressable(unsigned long pfn, > return 0; > } > > +static void vmem_context_init(struct vmem_context *ctx, struct mhp_params *params) > +{ > + memset(ctx, 0, sizeof(*ctx)); > + ctx->align = params->align; > + ctx->altmap = params->altmap; > + ctx->flags = params->flags; > +} > + > /* > * Reasonably generic function for adding memory. It is > * expected that archs that support memory hotplug will > @@ -313,7 +321,7 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, > unsigned long cur_nr_pages; > int err; > struct vmem_altmap *altmap = params->altmap; > - struct vmem_context ctx = { .altmap = params->altmap }; > + struct vmem_context ctx; > > if (WARN_ON_ONCE(!params->pgprot.pgprot)) > return -EINVAL; > @@ -338,6 +346,8 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, > if (err) > return err; > > + vmem_context_init(&ctx, params); > + > for (; pfn < end_pfn; pfn += cur_nr_pages) { > /* Select all remaining pages up to the next section boundary */ > cur_nr_pages = min(end_pfn - pfn, > diff --git a/mm/memremap.c b/mm/memremap.c > index 287a24b7a65a..ecfa74848ac6 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -253,6 +253,9 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params, > goto err_kasan; > } > > + if (pgmap->flags & PGMAP_COMPOUND) > + params->align = pgmap->align; > + > error = arch_add_memory(nid, range->start, range_len(range), > params); > } > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c > index bcda68ba1381..364c071350e8 100644 > --- a/mm/sparse-vmemmap.c > +++ b/mm/sparse-vmemmap.c > @@ -141,16 +141,20 @@ void __meminit vmemmap_verify(pte_t *pte, int node, > } > > pte_t * __meminit vmemmap_pte_populate(pmd_t *pmd, unsigned long addr, int node, > - struct vmem_altmap *altmap) > + struct vmem_altmap *altmap, void *block) > { > pte_t *pte = pte_offset_kernel(pmd, addr); > if (pte_none(*pte)) { > pte_t entry; > - void *p; > - > - p = vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap); > - if (!p) > - return NULL; > + void *p = block; > + > + if (!block) { > + p = vmemmap_alloc_block_buf(PAGE_SIZE, node, altmap); > + if (!p) > + return NULL; > + } else { > + get_page(virt_to_page(block)); > + } > entry = pfn_pte(__pa(p) >> PAGE_SHIFT, PAGE_KERNEL); > set_pte_at(&init_mm, addr, pte, entry); > } > @@ -216,8 +220,10 @@ pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node) > return pgd; > } > > -int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, > - int node, struct vmem_altmap *altmap) > +static void *__meminit __vmemmap_populate_basepages(unsigned long start, > + unsigned long end, int node, > + struct vmem_altmap *altmap, > + void *block) > { > unsigned long addr = start; > pgd_t *pgd; > @@ -229,38 +235,95 @@ int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, > for (; addr < end; addr += PAGE_SIZE) { > pgd = vmemmap_pgd_populate(addr, node); > if (!pgd) > - return -ENOMEM; > + return NULL; > p4d = vmemmap_p4d_populate(pgd, addr, node); > if (!p4d) > - return -ENOMEM; > + return NULL; > pud = vmemmap_pud_populate(p4d, addr, node); > if (!pud) > - return -ENOMEM; > + return NULL; > pmd = vmemmap_pmd_populate(pud, addr, node); > if (!pmd) > - return -ENOMEM; > - pte = vmemmap_pte_populate(pmd, addr, node, altmap); > + return NULL; > + pte = vmemmap_pte_populate(pmd, addr, node, altmap, block); > if (!pte) > - return -ENOMEM; > + return NULL; > vmemmap_verify(pte, node, addr, addr + PAGE_SIZE); > } > > + return __va(__pfn_to_phys(pte_pfn(*pte))); > +} > + > +int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, > + int node, struct vmem_altmap *altmap) > +{ > + if (!__vmemmap_populate_basepages(start, end, node, altmap, NULL)) > + return -ENOMEM; > return 0; > } > > +static struct page * __meminit vmemmap_populate_reuse(unsigned long start, > + unsigned long end, int node, > + struct vmem_context *ctx) > +{ > + unsigned long size, addr = start; > + unsigned long psize = PHYS_PFN(ctx->align) * sizeof(struct page); > + > + size = min(psize, end - start); > + > + for (; addr < end; addr += size) { > + unsigned long head = addr + PAGE_SIZE; > + unsigned long tail = addr; > + unsigned long last = addr + size; > + void *area; > + > + if (ctx->block_page && > + IS_ALIGNED((addr - ctx->block_page), psize)) > + ctx->block = NULL; > + > + area = ctx->block; > + if (!area) { > + if (!__vmemmap_populate_basepages(addr, head, node, > + ctx->altmap, NULL)) > + return NULL; > + > + tail = head + PAGE_SIZE; > + area = __vmemmap_populate_basepages(head, tail, node, > + ctx->altmap, NULL); > + if (!area) > + return NULL; > + > + ctx->block = area; > + ctx->block_page = addr; > + } > + > + if (!__vmemmap_populate_basepages(tail, last, node, > + ctx->altmap, area)) > + return NULL; > + } I think that compound page accounting and combined altmap accounting makes this difficult to read, and I think the compound page case deserves it's own first class loop rather than reusing vmemmap_populate_basepages(). With the suggestion to drop altmap support I'd expect a vmmemap_populate_compound that takes a compound page size and goes the right think with respect to mapping all the tail pages to the same pfn.
next prev parent reply other threads:[~2021-02-20 3:34 UTC|newest] Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-08 17:28 [PATCH RFC 0/9] mm, sparse-vmemmap: Introduce compound pagemaps Joao Martins 2020-12-08 17:28 ` [PATCH RFC 1/9] memremap: add ZONE_DEVICE support for compound pages Joao Martins 2020-12-09 5:59 ` John Hubbard 2020-12-09 6:33 ` Matthew Wilcox 2020-12-09 13:12 ` Joao Martins 2021-02-20 1:43 ` Dan Williams 2021-02-22 11:24 ` Joao Martins 2021-02-22 20:37 ` Dan Williams 2021-02-23 15:46 ` Joao Martins 2021-02-23 16:50 ` Dan Williams 2021-02-23 17:18 ` Joao Martins 2021-02-23 18:18 ` Dan Williams 2021-03-10 18:12 ` Joao Martins 2021-03-12 5:54 ` Dan Williams 2021-02-20 1:24 ` Dan Williams 2021-02-22 11:09 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 2/9] sparse-vmemmap: Consolidate arguments in vmemmap section populate Joao Martins 2020-12-09 6:16 ` John Hubbard 2020-12-09 13:51 ` Joao Martins 2021-02-20 1:49 ` Dan Williams 2021-02-22 11:26 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 3/9] sparse-vmemmap: Reuse vmemmap areas for a given mhp_params::align Joao Martins 2020-12-08 17:38 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 3/9] sparse-vmemmap: Reuse vmemmap areas for a given page size Joao Martins 2021-02-20 3:34 ` Dan Williams [this message] 2021-02-22 11:42 ` Joao Martins 2021-02-22 22:40 ` Dan Williams 2021-02-23 15:46 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 4/9] mm/page_alloc: Reuse tail struct pages for compound pagemaps Joao Martins 2021-02-20 6:17 ` Dan Williams 2021-02-22 12:01 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 5/9] device-dax: Compound pagemap support Joao Martins 2020-12-08 17:28 ` [PATCH RFC 6/9] mm/gup: Grab head page refcount once for group of subpages Joao Martins 2020-12-08 19:49 ` Jason Gunthorpe 2020-12-09 11:05 ` Joao Martins 2020-12-09 15:15 ` Jason Gunthorpe 2020-12-09 16:02 ` Joao Martins 2020-12-09 16:24 ` Jason Gunthorpe 2020-12-09 17:27 ` Joao Martins 2020-12-09 18:14 ` Matthew Wilcox 2020-12-09 19:08 ` Jason Gunthorpe 2020-12-10 15:43 ` Joao Martins 2020-12-09 4:40 ` John Hubbard 2020-12-09 13:44 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 7/9] mm/gup: Decrement head page " Joao Martins 2020-12-08 19:34 ` Jason Gunthorpe 2020-12-09 5:06 ` John Hubbard 2020-12-09 13:43 ` Jason Gunthorpe 2020-12-09 12:17 ` Joao Martins 2020-12-17 19:05 ` Joao Martins 2020-12-17 20:05 ` Jason Gunthorpe 2020-12-17 22:34 ` Joao Martins 2020-12-18 14:25 ` Jason Gunthorpe 2020-12-19 2:06 ` John Hubbard 2020-12-19 13:10 ` Joao Martins 2020-12-08 17:29 ` [PATCH RFC 8/9] RDMA/umem: batch page unpin in __ib_mem_release() Joao Martins 2020-12-08 19:29 ` Jason Gunthorpe 2020-12-09 10:59 ` Joao Martins 2020-12-19 13:15 ` Joao Martins 2020-12-09 5:18 ` John Hubbard 2020-12-08 17:29 ` [PATCH RFC 9/9] mm: Add follow_devmap_page() for devdax vmas Joao Martins 2020-12-08 19:57 ` Jason Gunthorpe 2020-12-09 8:05 ` Christoph Hellwig 2020-12-09 11:19 ` Joao Martins 2020-12-09 5:23 ` John Hubbard 2020-12-09 9:38 ` [PATCH RFC 0/9] mm, sparse-vmemmap: Introduce compound pagemaps David Hildenbrand 2020-12-09 9:52 ` [External] " Muchun Song 2021-02-20 1:18 ` Dan Williams 2021-02-22 11:06 ` Joao Martins 2021-02-22 14:32 ` Joao Martins 2021-02-23 16:28 ` Joao Martins 2021-02-23 16:44 ` Dan Williams 2021-02-23 17:15 ` Joao Martins 2021-02-23 18:15 ` Dan Williams 2021-02-23 18:54 ` Jason Gunthorpe 2021-02-23 22:48 ` Dan Williams 2021-02-23 23:07 ` Jason Gunthorpe 2021-02-24 0:14 ` Dan Williams 2021-02-24 1:00 ` Jason Gunthorpe 2021-02-24 1:32 ` Dan Williams
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=CAPcyv4ixD6_KEpuXkpeH6JNLvoahch8rm8J55o1B97ghrtcbjQ@mail.gmail.com \ --to=dan.j.williams@intel.com \ --cc=akpm@linux-foundation.org \ --cc=ira.weiny@intel.com \ --cc=jane.chu@oracle.com \ --cc=jgg@ziepe.ca \ --cc=joao.m.martins@oracle.com \ --cc=linux-mm@kvack.org \ --cc=linux-nvdimm@lists.01.org \ --cc=mike.kravetz@oracle.com \ --cc=songmuchun@bytedance.com \ --cc=willy@infradead.org \ --subject='Re: [PATCH RFC 3/9] sparse-vmemmap: Reuse vmemmap areas for a given page size' \ /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: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).