From: Christoph Hellwig <hch@infradead.org> To: John Hubbard <jhubbard@nvidia.com> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Christoph Hellwig" <hch@infradead.org>, "Jan Kara" <jack@suse.cz>, "Jason Gunthorpe" <jgg@ziepe.ca>, "Jens Axboe" <axboe@kernel.dk>, "Jérôme Glisse" <jglisse@redhat.com>, linux-nvdimm@lists.01.org, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, "Christoph Hellwig" <hch@lst.de> Subject: Re: [PATCH 2/2] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages Date: Sun, 17 Nov 2019 23:08:26 -0800 [thread overview] Message-ID: <20191118070826.GB3099@infradead.org> (raw) In-Reply-To: <20191115001134.2489505-3-jhubbard@nvidia.com> On Thu, Nov 14, 2019 at 04:11:34PM -0800, John Hubbard wrote: > An upcoming patch changes and complicates the refcounting and > especially the "put page" aspects of it. In order to keep > everything clean, refactor the devmap page release routines: > > * Rename put_devmap_managed_page() to page_is_devmap_managed(), > and limit the functionality to "read only": return a bool, > with no side effects. > > * Add a new routine, put_devmap_managed_page(), to handle checking > what kind of page it is, and what kind of refcount handling it > requires. > > * Rename __put_devmap_managed_page() to free_devmap_managed_page(), > and limit the functionality to unconditionally freeing a devmap > page. > > This is originally based on a separate patch by Ira Weiny, which > applied to an early version of the put_user_page() experiments. > Since then, Jérôme Glisse suggested the refactoring described above. I can't say I'm a big fan of this as it adds a lot more inlined code to put_page, which has a lot of callsites. Can't we instead try to figure out a way to move away from the off by one refcounting? > > Cc: Jan Kara <jack@suse.cz> > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Dan Williams <dan.j.williams@intel.com> > Suggested-by: Jérôme Glisse <jglisse@redhat.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > include/linux/mm.h | 27 ++++++++++++++++++++++++--- > mm/memremap.c | 16 ++-------------- > 2 files changed, 26 insertions(+), 17 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index a2adf95b3f9c..96228376139c 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -967,9 +967,10 @@ static inline bool is_zone_device_page(const struct page *page) > #endif > > #ifdef CONFIG_DEV_PAGEMAP_OPS > -void __put_devmap_managed_page(struct page *page); > +void free_devmap_managed_page(struct page *page); > DECLARE_STATIC_KEY_FALSE(devmap_managed_key); > -static inline bool put_devmap_managed_page(struct page *page) > + > +static inline bool page_is_devmap_managed(struct page *page) > { > if (!static_branch_unlikely(&devmap_managed_key)) > return false; > @@ -978,7 +979,6 @@ static inline bool put_devmap_managed_page(struct page *page) > switch (page->pgmap->type) { > case MEMORY_DEVICE_PRIVATE: > case MEMORY_DEVICE_FS_DAX: > - __put_devmap_managed_page(page); > return true; > default: > break; > @@ -986,6 +986,27 @@ static inline bool put_devmap_managed_page(struct page *page) > return false; > } > > +static inline bool put_devmap_managed_page(struct page *page) > +{ > + bool is_devmap = page_is_devmap_managed(page); > + > + if (is_devmap) { > + int count = page_ref_dec_return(page); > + > + /* > + * devmap page refcounts are 1-based, rather than 0-based: if > + * refcount is 1, then the page is free and the refcount is > + * stable because nobody holds a reference on the page. > + */ > + if (count == 1) > + free_devmap_managed_page(page); > + else if (!count) > + __put_page(page); > + } > + > + return is_devmap; > +} > + > #else /* CONFIG_DEV_PAGEMAP_OPS */ > static inline bool put_devmap_managed_page(struct page *page) > { > diff --git a/mm/memremap.c b/mm/memremap.c > index e899fa876a62..2ba773859031 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -411,20 +411,8 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn, > EXPORT_SYMBOL_GPL(get_dev_pagemap); > > #ifdef CONFIG_DEV_PAGEMAP_OPS > -void __put_devmap_managed_page(struct page *page) > +void free_devmap_managed_page(struct page *page) > { > - int count = page_ref_dec_return(page); > - > - /* still busy */ > - if (count > 1) > - return; > - > - /* only triggered by the dev_pagemap shutdown path */ > - if (count == 0) { > - __put_page(page); > - return; > - } > - > /* notify page idle for dax */ > if (!is_device_private_page(page)) { > wake_up_var(&page->_refcount); > @@ -461,5 +449,5 @@ void __put_devmap_managed_page(struct page *page) > page->mapping = NULL; > page->pgmap->ops->page_free(page); > } > -EXPORT_SYMBOL(__put_devmap_managed_page); > +EXPORT_SYMBOL(free_devmap_managed_page); > #endif /* CONFIG_DEV_PAGEMAP_OPS */ > -- > 2.24.0 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ---end quoted text--- _______________________________________________ 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: Christoph Hellwig <hch@infradead.org> To: John Hubbard <jhubbard@nvidia.com> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Christoph Hellwig" <hch@infradead.org>, "Jan Kara" <jack@suse.cz>, "Jason Gunthorpe" <jgg@ziepe.ca>, "Jens Axboe" <axboe@kernel.dk>, "Jérôme Glisse" <jglisse@redhat.com>, linux-nvdimm@lists.01.org, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, "Christoph Hellwig" <hch@lst.de> Subject: Re: [PATCH 2/2] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages Date: Sun, 17 Nov 2019 23:08:26 -0800 [thread overview] Message-ID: <20191118070826.GB3099@infradead.org> (raw) In-Reply-To: <20191115001134.2489505-3-jhubbard@nvidia.com> On Thu, Nov 14, 2019 at 04:11:34PM -0800, John Hubbard wrote: > An upcoming patch changes and complicates the refcounting and > especially the "put page" aspects of it. In order to keep > everything clean, refactor the devmap page release routines: > > * Rename put_devmap_managed_page() to page_is_devmap_managed(), > and limit the functionality to "read only": return a bool, > with no side effects. > > * Add a new routine, put_devmap_managed_page(), to handle checking > what kind of page it is, and what kind of refcount handling it > requires. > > * Rename __put_devmap_managed_page() to free_devmap_managed_page(), > and limit the functionality to unconditionally freeing a devmap > page. > > This is originally based on a separate patch by Ira Weiny, which > applied to an early version of the put_user_page() experiments. > Since then, Jérôme Glisse suggested the refactoring described above. I can't say I'm a big fan of this as it adds a lot more inlined code to put_page, which has a lot of callsites. Can't we instead try to figure out a way to move away from the off by one refcounting? > > Cc: Jan Kara <jack@suse.cz> > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Dan Williams <dan.j.williams@intel.com> > Suggested-by: Jérôme Glisse <jglisse@redhat.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > include/linux/mm.h | 27 ++++++++++++++++++++++++--- > mm/memremap.c | 16 ++-------------- > 2 files changed, 26 insertions(+), 17 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index a2adf95b3f9c..96228376139c 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -967,9 +967,10 @@ static inline bool is_zone_device_page(const struct page *page) > #endif > > #ifdef CONFIG_DEV_PAGEMAP_OPS > -void __put_devmap_managed_page(struct page *page); > +void free_devmap_managed_page(struct page *page); > DECLARE_STATIC_KEY_FALSE(devmap_managed_key); > -static inline bool put_devmap_managed_page(struct page *page) > + > +static inline bool page_is_devmap_managed(struct page *page) > { > if (!static_branch_unlikely(&devmap_managed_key)) > return false; > @@ -978,7 +979,6 @@ static inline bool put_devmap_managed_page(struct page *page) > switch (page->pgmap->type) { > case MEMORY_DEVICE_PRIVATE: > case MEMORY_DEVICE_FS_DAX: > - __put_devmap_managed_page(page); > return true; > default: > break; > @@ -986,6 +986,27 @@ static inline bool put_devmap_managed_page(struct page *page) > return false; > } > > +static inline bool put_devmap_managed_page(struct page *page) > +{ > + bool is_devmap = page_is_devmap_managed(page); > + > + if (is_devmap) { > + int count = page_ref_dec_return(page); > + > + /* > + * devmap page refcounts are 1-based, rather than 0-based: if > + * refcount is 1, then the page is free and the refcount is > + * stable because nobody holds a reference on the page. > + */ > + if (count == 1) > + free_devmap_managed_page(page); > + else if (!count) > + __put_page(page); > + } > + > + return is_devmap; > +} > + > #else /* CONFIG_DEV_PAGEMAP_OPS */ > static inline bool put_devmap_managed_page(struct page *page) > { > diff --git a/mm/memremap.c b/mm/memremap.c > index e899fa876a62..2ba773859031 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -411,20 +411,8 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn, > EXPORT_SYMBOL_GPL(get_dev_pagemap); > > #ifdef CONFIG_DEV_PAGEMAP_OPS > -void __put_devmap_managed_page(struct page *page) > +void free_devmap_managed_page(struct page *page) > { > - int count = page_ref_dec_return(page); > - > - /* still busy */ > - if (count > 1) > - return; > - > - /* only triggered by the dev_pagemap shutdown path */ > - if (count == 0) { > - __put_page(page); > - return; > - } > - > /* notify page idle for dax */ > if (!is_device_private_page(page)) { > wake_up_var(&page->_refcount); > @@ -461,5 +449,5 @@ void __put_devmap_managed_page(struct page *page) > page->mapping = NULL; > page->pgmap->ops->page_free(page); > } > -EXPORT_SYMBOL(__put_devmap_managed_page); > +EXPORT_SYMBOL(free_devmap_managed_page); > #endif /* CONFIG_DEV_PAGEMAP_OPS */ > -- > 2.24.0 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ---end quoted text---
next prev parent reply other threads:[~2019-11-18 7:08 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-15 0:11 [PATCH 0/2] mm: devmap: page-freeing related cleanups John Hubbard 2019-11-15 0:11 ` John Hubbard 2019-11-15 0:11 ` [PATCH 1/2] mm: Cleanup __put_devmap_managed_page() vs ->page_free() John Hubbard 2019-11-15 0:11 ` John Hubbard 2019-11-18 7:04 ` Christoph Hellwig 2019-11-18 7:04 ` Christoph Hellwig 2019-11-15 0:11 ` [PATCH 2/2] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages John Hubbard 2019-11-15 0:11 ` John Hubbard 2019-11-15 9:48 ` Jan Kara 2019-11-15 9:48 ` Jan Kara 2019-11-15 16:41 ` Dan Williams 2019-11-15 16:41 ` Dan Williams 2019-11-18 7:08 ` Christoph Hellwig [this message] 2019-11-18 7:08 ` Christoph Hellwig 2019-11-18 16:57 ` Dan Williams 2019-11-18 16:57 ` 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=20191118070826.GB3099@infradead.org \ --to=hch@infradead.org \ --cc=akpm@linux-foundation.org \ --cc=axboe@kernel.dk \ --cc=hch@lst.de \ --cc=jack@suse.cz \ --cc=jgg@ziepe.ca \ --cc=jglisse@redhat.com \ --cc=jhubbard@nvidia.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nvdimm@lists.01.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.