From: John Hubbard <jhubbard@nvidia.com> To: Joao Martins <joao.m.martins@oracle.com>, <linux-mm@kvack.org> Cc: 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@ml01.01.org Subject: Re: [PATCH RFC 1/9] memremap: add ZONE_DEVICE support for compound pages Date: Tue, 8 Dec 2020 21:59:19 -0800 [thread overview] Message-ID: <7249cfd2-c178-2e6a-6b03-307a05f11785@nvidia.com> (raw) In-Reply-To: <20201208172901.17384-2-joao.m.martins@oracle.com> On 12/8/20 9:28 AM, Joao Martins wrote: > Add a new flag for struct dev_pagemap which designates that a a pagemap a a > is described as a set of compound pages or in other words, that how > pages are grouped together in the page tables are reflected in how we > describe struct pages. This means that rather than initializing > individual struct pages, we also initialize these struct pages, as Let's not say "rather than x, we also do y", because it's self-contradictory. I think you want to just leave out the "also", like this: "This means that rather than initializing> individual struct pages, we initialize these struct pages ..." Is that right? > compound pages (on x86: 2M or 1G compound pages) > > For certain ZONE_DEVICE users, like device-dax, which have a fixed page > size, this creates an opportunity to optimize GUP and GUP-fast walkers, > thus playing the same tricks as hugetlb pages. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > include/linux/memremap.h | 2 ++ > mm/memremap.c | 8 ++++++-- > mm/page_alloc.c | 7 +++++++ > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/include/linux/memremap.h b/include/linux/memremap.h > index 79c49e7f5c30..f8f26b2cc3da 100644 > --- a/include/linux/memremap.h > +++ b/include/linux/memremap.h > @@ -90,6 +90,7 @@ struct dev_pagemap_ops { > }; > > #define PGMAP_ALTMAP_VALID (1 << 0) > +#define PGMAP_COMPOUND (1 << 1) > > /** > * struct dev_pagemap - metadata for ZONE_DEVICE mappings > @@ -114,6 +115,7 @@ struct dev_pagemap { > struct completion done; > enum memory_type type; > unsigned int flags; > + unsigned int align; This also needs an "@aline" entry in the comment block above. > const struct dev_pagemap_ops *ops; > void *owner; > int nr_range; > diff --git a/mm/memremap.c b/mm/memremap.c > index 16b2fb482da1..287a24b7a65a 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -277,8 +277,12 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params, > memmap_init_zone_device(&NODE_DATA(nid)->node_zones[ZONE_DEVICE], > PHYS_PFN(range->start), > PHYS_PFN(range_len(range)), pgmap); > - percpu_ref_get_many(pgmap->ref, pfn_end(pgmap, range_id) > - - pfn_first(pgmap, range_id)); > + if (pgmap->flags & PGMAP_COMPOUND) > + percpu_ref_get_many(pgmap->ref, (pfn_end(pgmap, range_id) > + - pfn_first(pgmap, range_id)) / PHYS_PFN(pgmap->align)); Is there some reason that we cannot use range_len(), instead of pfn_end() minus pfn_first()? (Yes, this more about the pre-existing code than about your change.) And if not, then why are the nearby range_len() uses OK? I realize that range_len() is simpler and skips a case, but it's not clear that it's required here. But I'm new to this area so be warned. :) Also, dividing by PHYS_PFN() feels quite misleading: that function does what you happen to want, but is not named accordingly. Can you use or create something more accurately named? Like "number of pages in this large page"? > + else > + percpu_ref_get_many(pgmap->ref, pfn_end(pgmap, range_id) > + - pfn_first(pgmap, range_id)); > return 0; > > err_add_memory: > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index eaa227a479e4..9716ecd58e29 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6116,6 +6116,8 @@ void __ref memmap_init_zone_device(struct zone *zone, > unsigned long pfn, end_pfn = start_pfn + nr_pages; > struct pglist_data *pgdat = zone->zone_pgdat; > struct vmem_altmap *altmap = pgmap_altmap(pgmap); > + bool compound = pgmap->flags & PGMAP_COMPOUND; > + unsigned int align = PHYS_PFN(pgmap->align); Maybe align_pfn or pfn_align? Don't want the same name for things that are actually different types, in meaning anyway. > unsigned long zone_idx = zone_idx(zone); > unsigned long start = jiffies; > int nid = pgdat->node_id; > @@ -6171,6 +6173,11 @@ void __ref memmap_init_zone_device(struct zone *zone, > } > } > > + if (compound) { > + for (pfn = start_pfn; pfn < end_pfn; pfn += align) > + prep_compound_page(pfn_to_page(pfn), order_base_2(align)); > + } > + > pr_info("%s initialised %lu pages in %ums\n", __func__, > nr_pages, jiffies_to_msecs(jiffies - start)); > } > thanks, -- John Hubbard NVIDIA _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
WARNING: multiple messages have this Message-ID (diff)
From: John Hubbard <jhubbard@nvidia.com> To: Joao Martins <joao.m.martins@oracle.com>, <linux-mm@kvack.org> Cc: Dan Williams <dan.j.williams@intel.com>, Ira Weiny <ira.weiny@intel.com>, <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 1/9] memremap: add ZONE_DEVICE support for compound pages Date: Tue, 8 Dec 2020 21:59:19 -0800 [thread overview] Message-ID: <7249cfd2-c178-2e6a-6b03-307a05f11785@nvidia.com> (raw) In-Reply-To: <20201208172901.17384-2-joao.m.martins@oracle.com> On 12/8/20 9:28 AM, Joao Martins wrote: > Add a new flag for struct dev_pagemap which designates that a a pagemap a a > is described as a set of compound pages or in other words, that how > pages are grouped together in the page tables are reflected in how we > describe struct pages. This means that rather than initializing > individual struct pages, we also initialize these struct pages, as Let's not say "rather than x, we also do y", because it's self-contradictory. I think you want to just leave out the "also", like this: "This means that rather than initializing> individual struct pages, we initialize these struct pages ..." Is that right? > compound pages (on x86: 2M or 1G compound pages) > > For certain ZONE_DEVICE users, like device-dax, which have a fixed page > size, this creates an opportunity to optimize GUP and GUP-fast walkers, > thus playing the same tricks as hugetlb pages. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > include/linux/memremap.h | 2 ++ > mm/memremap.c | 8 ++++++-- > mm/page_alloc.c | 7 +++++++ > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/include/linux/memremap.h b/include/linux/memremap.h > index 79c49e7f5c30..f8f26b2cc3da 100644 > --- a/include/linux/memremap.h > +++ b/include/linux/memremap.h > @@ -90,6 +90,7 @@ struct dev_pagemap_ops { > }; > > #define PGMAP_ALTMAP_VALID (1 << 0) > +#define PGMAP_COMPOUND (1 << 1) > > /** > * struct dev_pagemap - metadata for ZONE_DEVICE mappings > @@ -114,6 +115,7 @@ struct dev_pagemap { > struct completion done; > enum memory_type type; > unsigned int flags; > + unsigned int align; This also needs an "@aline" entry in the comment block above. > const struct dev_pagemap_ops *ops; > void *owner; > int nr_range; > diff --git a/mm/memremap.c b/mm/memremap.c > index 16b2fb482da1..287a24b7a65a 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -277,8 +277,12 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params, > memmap_init_zone_device(&NODE_DATA(nid)->node_zones[ZONE_DEVICE], > PHYS_PFN(range->start), > PHYS_PFN(range_len(range)), pgmap); > - percpu_ref_get_many(pgmap->ref, pfn_end(pgmap, range_id) > - - pfn_first(pgmap, range_id)); > + if (pgmap->flags & PGMAP_COMPOUND) > + percpu_ref_get_many(pgmap->ref, (pfn_end(pgmap, range_id) > + - pfn_first(pgmap, range_id)) / PHYS_PFN(pgmap->align)); Is there some reason that we cannot use range_len(), instead of pfn_end() minus pfn_first()? (Yes, this more about the pre-existing code than about your change.) And if not, then why are the nearby range_len() uses OK? I realize that range_len() is simpler and skips a case, but it's not clear that it's required here. But I'm new to this area so be warned. :) Also, dividing by PHYS_PFN() feels quite misleading: that function does what you happen to want, but is not named accordingly. Can you use or create something more accurately named? Like "number of pages in this large page"? > + else > + percpu_ref_get_many(pgmap->ref, pfn_end(pgmap, range_id) > + - pfn_first(pgmap, range_id)); > return 0; > > err_add_memory: > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index eaa227a479e4..9716ecd58e29 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6116,6 +6116,8 @@ void __ref memmap_init_zone_device(struct zone *zone, > unsigned long pfn, end_pfn = start_pfn + nr_pages; > struct pglist_data *pgdat = zone->zone_pgdat; > struct vmem_altmap *altmap = pgmap_altmap(pgmap); > + bool compound = pgmap->flags & PGMAP_COMPOUND; > + unsigned int align = PHYS_PFN(pgmap->align); Maybe align_pfn or pfn_align? Don't want the same name for things that are actually different types, in meaning anyway. > unsigned long zone_idx = zone_idx(zone); > unsigned long start = jiffies; > int nid = pgdat->node_id; > @@ -6171,6 +6173,11 @@ void __ref memmap_init_zone_device(struct zone *zone, > } > } > > + if (compound) { > + for (pfn = start_pfn; pfn < end_pfn; pfn += align) > + prep_compound_page(pfn_to_page(pfn), order_base_2(align)); > + } > + > pr_info("%s initialised %lu pages in %ums\n", __func__, > nr_pages, jiffies_to_msecs(jiffies - start)); > } > thanks, -- John Hubbard NVIDIA
next prev parent reply other threads:[~2020-12-09 5:59 UTC|newest] Thread overview: 147+ 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 ` Joao Martins 2020-12-08 17:28 ` [PATCH RFC 1/9] memremap: add ZONE_DEVICE support for compound pages Joao Martins 2020-12-08 17:28 ` Joao Martins 2020-12-09 5:59 ` John Hubbard [this message] 2020-12-09 5:59 ` John Hubbard 2020-12-09 6:33 ` Matthew Wilcox 2020-12-09 6:33 ` Matthew Wilcox 2020-12-09 13:12 ` Joao Martins 2020-12-09 13:12 ` Joao Martins 2021-02-20 1:43 ` Dan Williams 2021-02-20 1:43 ` Dan Williams 2021-02-22 11:24 ` Joao Martins 2021-02-22 11:24 ` Joao Martins 2021-02-22 20:37 ` Dan Williams 2021-02-22 20:37 ` Dan Williams 2021-02-23 15:46 ` Joao Martins 2021-02-23 15:46 ` Joao Martins 2021-02-23 16:50 ` Dan Williams 2021-02-23 16:50 ` Dan Williams 2021-02-23 17:18 ` Joao Martins 2021-02-23 17:18 ` Joao Martins 2021-02-23 18:18 ` Dan Williams 2021-02-23 18:18 ` Dan Williams 2021-03-10 18:12 ` Joao Martins 2021-03-10 18:12 ` Joao Martins 2021-03-12 5:54 ` Dan Williams 2021-03-12 5:54 ` Dan Williams 2021-02-20 1:24 ` Dan Williams 2021-02-20 1:24 ` Dan Williams 2021-02-22 11:09 ` Joao Martins 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-08 17:28 ` Joao Martins 2020-12-09 6:16 ` John Hubbard 2020-12-09 6:16 ` John Hubbard 2020-12-09 13:51 ` Joao Martins 2020-12-09 13:51 ` Joao Martins 2021-02-20 1:49 ` Dan Williams 2021-02-20 1:49 ` Dan Williams 2021-02-22 11:26 ` Joao Martins 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:28 ` Joao Martins 2020-12-08 17:38 ` 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 2020-12-08 17:28 ` Joao Martins 2021-02-20 3:34 ` Dan Williams 2021-02-20 3:34 ` Dan Williams 2021-02-22 11:42 ` Joao Martins 2021-02-22 11:42 ` Joao Martins 2021-02-22 22:40 ` Dan Williams 2021-02-22 22:40 ` Dan Williams 2021-02-23 15:46 ` Joao Martins 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 2020-12-08 17:28 ` Joao Martins 2021-02-20 6:17 ` Dan Williams 2021-02-20 6:17 ` Dan Williams 2021-02-22 12:01 ` Joao Martins 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 ` 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 17:28 ` Joao Martins 2020-12-08 19:49 ` Jason Gunthorpe 2020-12-09 11:05 ` Joao Martins 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:02 ` Joao Martins 2020-12-09 16:24 ` Jason Gunthorpe 2020-12-09 17:27 ` Joao Martins 2020-12-09 17:27 ` Joao Martins 2020-12-09 18:14 ` Matthew Wilcox 2020-12-09 18:14 ` Matthew Wilcox 2020-12-09 19:08 ` Jason Gunthorpe 2020-12-10 15:43 ` Joao Martins 2020-12-10 15:43 ` Joao Martins 2020-12-09 4:40 ` John Hubbard 2020-12-09 4:40 ` John Hubbard 2020-12-09 13:44 ` Joao Martins 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 17:28 ` Joao Martins 2020-12-08 19:34 ` Jason Gunthorpe 2020-12-09 5:06 ` John Hubbard 2020-12-09 5:06 ` John Hubbard 2020-12-09 13:43 ` Jason Gunthorpe 2020-12-09 12:17 ` Joao Martins 2020-12-09 12:17 ` Joao Martins 2020-12-17 19:05 ` 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-17 22:34 ` Joao Martins 2020-12-18 14:25 ` Jason Gunthorpe 2020-12-19 2:06 ` John Hubbard 2020-12-19 2:06 ` John Hubbard 2020-12-19 13:10 ` Joao Martins 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 17:29 ` Joao Martins 2020-12-08 19:29 ` Jason Gunthorpe 2020-12-09 10:59 ` Joao Martins 2020-12-09 10:59 ` Joao Martins 2020-12-19 13:15 ` Joao Martins 2020-12-19 13:15 ` Joao Martins 2020-12-09 5:18 ` John Hubbard 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 17:29 ` Joao Martins 2020-12-08 19:57 ` Jason Gunthorpe 2020-12-09 8:05 ` Christoph Hellwig 2020-12-09 8:05 ` Christoph Hellwig 2020-12-09 11:19 ` Joao Martins 2020-12-09 11:19 ` Joao Martins 2020-12-09 5:23 ` John Hubbard 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:38 ` David Hildenbrand 2020-12-09 9:52 ` [External] " Muchun Song 2020-12-09 9:52 ` Muchun Song 2021-02-20 1:18 ` Dan Williams 2021-02-20 1:18 ` Dan Williams 2021-02-22 11:06 ` Joao Martins 2021-02-22 11:06 ` Joao Martins 2021-02-22 14:32 ` Joao Martins 2021-02-22 14:32 ` Joao Martins 2021-02-23 16:28 ` Joao Martins 2021-02-23 16:28 ` Joao Martins 2021-02-23 16:44 ` Dan Williams 2021-02-23 16:44 ` Dan Williams 2021-02-23 17:15 ` Joao Martins 2021-02-23 17:15 ` Joao Martins 2021-02-23 18:15 ` Dan Williams 2021-02-23 18:15 ` Dan Williams 2021-02-23 18:54 ` Jason Gunthorpe 2021-02-23 22:48 ` Dan Williams 2021-02-23 22:48 ` Dan Williams 2021-02-23 23:07 ` Jason Gunthorpe 2021-02-24 0:14 ` Dan Williams 2021-02-24 0:14 ` Dan Williams 2021-02-24 1:00 ` Jason Gunthorpe 2021-02-24 1:32 ` Dan Williams 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=7249cfd2-c178-2e6a-6b03-307a05f11785@nvidia.com \ --to=jhubbard@nvidia.com \ --cc=Andrew@ml01.01.org \ --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 \ /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.