From: Dan Williams <dan.j.williams@intel.com> To: John Hubbard <jhubbard@nvidia.com> Cc: Joao Martins <joao.m.martins@oracle.com>, Linux MM <linux-mm@kvack.org>, linux-nvdimm <linux-nvdimm@lists.01.org>, Matthew Wilcox <willy@infradead.org>, Jason Gunthorpe <jgg@ziepe.ca>, 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: Fri, 19 Feb 2021 17:43:18 -0800 [thread overview] Message-ID: <CAPcyv4iMzaAqKe6C1Q_feHL5SwppPdBem0i6XfYX4sL1U6sHgg@mail.gmail.com> (raw) In-Reply-To: <7249cfd2-c178-2e6a-6b03-307a05f11785@nvidia.com> On Tue, Dec 8, 2020 at 9:59 PM John Hubbard <jhubbard@nvidia.com> wrote: > > 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. :) There's a subtle distinction between the range that was passed in and the pfns that are activated inside of it. See the offset trickery in pfn_first(). > 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"? It's not the number of pages in a large page it's converting bytes to pages. Other place in the kernel write it as (x >> PAGE_SHIFT), but my though process was if I'm going to add () might as well use a macro that already does this. That said I think this calculation is broken precisely because pfn_first() makes the result unaligned. Rather than fix the unaligned pfn_first() problem I would use this support as an opportunity to revisit the option of storing pages in the vmem_altmap reserve soace. The altmap's whole reason for existence was that 1.5% of large PMEM might completely swamp DRAM. However if that overhead is reduced by an order (or orders) of magnitude the primary need for vmem_altmap vanishes. Now, we'll still need to keep it around for the ->align == PAGE_SIZE case, but for most part existing deployments that are specifying page map on PMEM and an align > PAGE_SIZE can instead just transparently be upgraded to page map on a smaller amount of DRAM. > > > + 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. Good catch. _______________________________________________ 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: Dan Williams <dan.j.williams@intel.com> To: John Hubbard <jhubbard@nvidia.com> Cc: Joao Martins <joao.m.martins@oracle.com>, 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 1/9] memremap: add ZONE_DEVICE support for compound pages Date: Fri, 19 Feb 2021 17:43:18 -0800 [thread overview] Message-ID: <CAPcyv4iMzaAqKe6C1Q_feHL5SwppPdBem0i6XfYX4sL1U6sHgg@mail.gmail.com> (raw) In-Reply-To: <7249cfd2-c178-2e6a-6b03-307a05f11785@nvidia.com> On Tue, Dec 8, 2020 at 9:59 PM John Hubbard <jhubbard@nvidia.com> wrote: > > 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. :) There's a subtle distinction between the range that was passed in and the pfns that are activated inside of it. See the offset trickery in pfn_first(). > 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"? It's not the number of pages in a large page it's converting bytes to pages. Other place in the kernel write it as (x >> PAGE_SHIFT), but my though process was if I'm going to add () might as well use a macro that already does this. That said I think this calculation is broken precisely because pfn_first() makes the result unaligned. Rather than fix the unaligned pfn_first() problem I would use this support as an opportunity to revisit the option of storing pages in the vmem_altmap reserve soace. The altmap's whole reason for existence was that 1.5% of large PMEM might completely swamp DRAM. However if that overhead is reduced by an order (or orders) of magnitude the primary need for vmem_altmap vanishes. Now, we'll still need to keep it around for the ->align == PAGE_SIZE case, but for most part existing deployments that are specifying page map on PMEM and an align > PAGE_SIZE can instead just transparently be upgraded to page map on a smaller amount of DRAM. > > > + 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. Good catch.
next prev parent reply other threads:[~2021-02-20 1:43 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 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 [this message] 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=CAPcyv4iMzaAqKe6C1Q_feHL5SwppPdBem0i6XfYX4sL1U6sHgg@mail.gmail.com \ --to=dan.j.williams@intel.com \ --cc=akpm@linux-foundation.org \ --cc=jgg@ziepe.ca \ --cc=jhubbard@nvidia.com \ --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.