Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1647152 > unrolled thread

[HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages Jérôme Glisse <jglisse@redhat.com> - 2017-05-22 19:00 +0200
    Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages Dan Williams <dan.j.williams@intel.com> - 2017-05-22 21:40 +0200
      Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages Jerome Glisse <jglisse@redhat.com> - 2017-05-22 22:20 +0200
        Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages Dan Williams <dan.j.williams@intel.com> - 2017-05-22 22:20 +0200
          Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages Jerome Glisse <jglisse@redhat.com> - 2017-05-22 23:20 +0200
        Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages Hugh Dickins <hughd@google.com> - 2017-05-22 22:30 +0200
          Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages Jerome Glisse <jglisse@redhat.com> - 2017-05-22 23:20 +0200
    Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device  private pages "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-05-23 15:30 +0200
      [HMM 08/18] mm/ZONE_DEVICE: special case put_page() for device private pages v2 Jérôme Glisse <jglisse@redhat.com> - 2017-05-23 23:40 +0200

#1647152 — [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
have any user. For device private pages this is important to catch
and thus we need to special case put_page() for this.

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
 kernel/memremap.c  |  1 -
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a825dab..11f7bac 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -23,6 +23,7 @@
 #include <linux/page_ext.h>
 #include <linux/err.h>
 #include <linux/page_ref.h>
+#include <linux/memremap.h>
 
 struct mempolicy;
 struct anon_vma;
@@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
 	return ((page_zonenum(page) == ZONE_DEVICE) &&
 		(page->pgmap->type == MEMORY_DEVICE_PRIVATE));
 }
+
+static inline void put_zone_device_private_page(struct page *page)
+{
+	int count = page_ref_dec_return(page);
+
+	/*
+	 * If refcount is 1 then page is freed and refcount is stable as nobody
+	 * holds a reference on the page.
+	 */
+	if (count == 1)
+		page->pgmap->page_free(page, page->pgmap->data);
+	else if (!count)
+		__put_page(page);
+}
 #else
 static inline bool is_zone_device_page(const struct page *page)
 {
@@ -805,6 +820,10 @@ static inline bool is_device_private_page(const struct page *page)
 {
 	return false;
 }
+
+static inline void put_zone_device_private_page(struct page *page)
+{
+}
 #endif
 
 static inline void get_page(struct page *page)
@@ -822,6 +841,17 @@ static inline void put_page(struct page *page)
 {
 	page = compound_head(page);
 
+	/*
+	 * For private device pages we need to catch refcount transition from
+	 * 2 to 1, when refcount reach one it means the private device page is
+	 * free and we need to inform the device driver through callback. See
+	 * include/linux/memremap.h and HMM for details.
+	 */
+	if (unlikely(is_device_private_page(page))) {
+		put_zone_device_private_page(page);
+		return;
+	}
+
 	if (put_page_testzero(page))
 		__put_page(page);
 }
diff --git a/kernel/memremap.c b/kernel/memremap.c
index dbdb656..71f6f28 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -11,7 +11,6 @@
  * General Public License for more details.
  */
 #include <linux/radix-tree.h>
-#include <linux/memremap.h>
 #include <linux/device.h>
 #include <linux/types.h>
 #include <linux/pfn_t.h>
-- 
2.9.3

[toc] | [next] | [standalone]

#1647300 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, May 22, 2017 at 9:51 AM, Jérôme Glisse <jglisse@redhat.com> wrote:
> A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
> have any user. For device private pages this is important to catch
> and thus we need to special case put_page() for this.
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
>  kernel/memremap.c  |  1 -
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a825dab..11f7bac 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -23,6 +23,7 @@
>  #include <linux/page_ext.h>
>  #include <linux/err.h>
>  #include <linux/page_ref.h>
> +#include <linux/memremap.h>
>
>  struct mempolicy;
>  struct anon_vma;
> @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
>         return ((page_zonenum(page) == ZONE_DEVICE) &&
>                 (page->pgmap->type == MEMORY_DEVICE_PRIVATE));
>  }
> +
> +static inline void put_zone_device_private_page(struct page *page)
> +{
> +       int count = page_ref_dec_return(page);
> +
> +       /*
> +        * If refcount is 1 then page is freed and refcount is stable as nobody
> +        * holds a reference on the page.
> +        */
> +       if (count == 1)
> +               page->pgmap->page_free(page, page->pgmap->data);
> +       else if (!count)
> +               __put_page(page);
> +}
>  #else
>  static inline bool is_zone_device_page(const struct page *page)
>  {
> @@ -805,6 +820,10 @@ static inline bool is_device_private_page(const struct page *page)
>  {
>         return false;
>  }
> +
> +static inline void put_zone_device_private_page(struct page *page)
> +{
> +}
>  #endif
>
>  static inline void get_page(struct page *page)
> @@ -822,6 +841,17 @@ static inline void put_page(struct page *page)
>  {
>         page = compound_head(page);
>
> +       /*
> +        * For private device pages we need to catch refcount transition from
> +        * 2 to 1, when refcount reach one it means the private device page is
> +        * free and we need to inform the device driver through callback. See
> +        * include/linux/memremap.h and HMM for details.
> +        */
> +       if (unlikely(is_device_private_page(page))) {

Since I presume HMM is a niche use case can we make this a
"static_branch_unlikely(&hmm_key) && is_device_private_page(page))"?
That way non-hmm platforms see minimal overhead.

[toc] | [prev] | [next] | [standalone]

#1647335 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, May 22, 2017 at 12:29:53PM -0700, Dan Williams wrote:
> On Mon, May 22, 2017 at 9:51 AM, Jérôme Glisse <jglisse@redhat.com> wrote:
> > A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
> > have any user. For device private pages this is important to catch
> > and thus we need to special case put_page() for this.
> >
> > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > ---
> >  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
> >  kernel/memremap.c  |  1 -
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index a825dab..11f7bac 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -23,6 +23,7 @@
> >  #include <linux/page_ext.h>
> >  #include <linux/err.h>
> >  #include <linux/page_ref.h>
> > +#include <linux/memremap.h>
> >
> >  struct mempolicy;
> >  struct anon_vma;
> > @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
> >         return ((page_zonenum(page) == ZONE_DEVICE) &&
> >                 (page->pgmap->type == MEMORY_DEVICE_PRIVATE));
> >  }
> > +
> > +static inline void put_zone_device_private_page(struct page *page)
> > +{
> > +       int count = page_ref_dec_return(page);
> > +
> > +       /*
> > +        * If refcount is 1 then page is freed and refcount is stable as nobody
> > +        * holds a reference on the page.
> > +        */
> > +       if (count == 1)
> > +               page->pgmap->page_free(page, page->pgmap->data);
> > +       else if (!count)
> > +               __put_page(page);
> > +}
> >  #else
> >  static inline bool is_zone_device_page(const struct page *page)
> >  {
> > @@ -805,6 +820,10 @@ static inline bool is_device_private_page(const struct page *page)
> >  {
> >         return false;
> >  }
> > +
> > +static inline void put_zone_device_private_page(struct page *page)
> > +{
> > +}
> >  #endif
> >
> >  static inline void get_page(struct page *page)
> > @@ -822,6 +841,17 @@ static inline void put_page(struct page *page)
> >  {
> >         page = compound_head(page);
> >
> > +       /*
> > +        * For private device pages we need to catch refcount transition from
> > +        * 2 to 1, when refcount reach one it means the private device page is
> > +        * free and we need to inform the device driver through callback. See
> > +        * include/linux/memremap.h and HMM for details.
> > +        */
> > +       if (unlikely(is_device_private_page(page))) {
> 
> Since I presume HMM is a niche use case can we make this a
> "static_branch_unlikely(&hmm_key) && is_device_private_page(page))"?
> That way non-hmm platforms see minimal overhead.

Like i said in the cover letter i am bit anxious about doing for
an inline function. I don't see any existing case for inline
function and static key. Is that suppose to work ?

How widespread HMM use will be is hard to guess. Usual chicken
and egg plus adoption thing. If GPGPU compte keeps growing and
it seems it does then HMM likely gonna be enable and actively
use for large chunk of those computer that have GPGPU workload.

I will test a static key of that branch and see if it explodes
because put_page() is an inline function.

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1647336 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, May 22, 2017 at 1:14 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> On Mon, May 22, 2017 at 12:29:53PM -0700, Dan Williams wrote:
>> On Mon, May 22, 2017 at 9:51 AM, Jérôme Glisse <jglisse@redhat.com> wrote:
>> > A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
>> > have any user. For device private pages this is important to catch
>> > and thus we need to special case put_page() for this.
>> >
>> > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
>> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > ---
>> >  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
>> >  kernel/memremap.c  |  1 -
>> >  2 files changed, 30 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> > index a825dab..11f7bac 100644
>> > --- a/include/linux/mm.h
>> > +++ b/include/linux/mm.h
>> > @@ -23,6 +23,7 @@
>> >  #include <linux/page_ext.h>
>> >  #include <linux/err.h>
>> >  #include <linux/page_ref.h>
>> > +#include <linux/memremap.h>
>> >
>> >  struct mempolicy;
>> >  struct anon_vma;
>> > @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
>> >         return ((page_zonenum(page) == ZONE_DEVICE) &&
>> >                 (page->pgmap->type == MEMORY_DEVICE_PRIVATE));
>> >  }
>> > +
>> > +static inline void put_zone_device_private_page(struct page *page)
>> > +{
>> > +       int count = page_ref_dec_return(page);
>> > +
>> > +       /*
>> > +        * If refcount is 1 then page is freed and refcount is stable as nobody
>> > +        * holds a reference on the page.
>> > +        */
>> > +       if (count == 1)
>> > +               page->pgmap->page_free(page, page->pgmap->data);
>> > +       else if (!count)
>> > +               __put_page(page);
>> > +}
>> >  #else
>> >  static inline bool is_zone_device_page(const struct page *page)
>> >  {
>> > @@ -805,6 +820,10 @@ static inline bool is_device_private_page(const struct page *page)
>> >  {
>> >         return false;
>> >  }
>> > +
>> > +static inline void put_zone_device_private_page(struct page *page)
>> > +{
>> > +}
>> >  #endif
>> >
>> >  static inline void get_page(struct page *page)
>> > @@ -822,6 +841,17 @@ static inline void put_page(struct page *page)
>> >  {
>> >         page = compound_head(page);
>> >
>> > +       /*
>> > +        * For private device pages we need to catch refcount transition from
>> > +        * 2 to 1, when refcount reach one it means the private device page is
>> > +        * free and we need to inform the device driver through callback. See
>> > +        * include/linux/memremap.h and HMM for details.
>> > +        */
>> > +       if (unlikely(is_device_private_page(page))) {
>>
>> Since I presume HMM is a niche use case can we make this a
>> "static_branch_unlikely(&hmm_key) && is_device_private_page(page))"?
>> That way non-hmm platforms see minimal overhead.
>
> Like i said in the cover letter i am bit anxious about doing for

I don't think you copied me on the cover letter.

> an inline function. I don't see any existing case for inline
> function and static key. Is that suppose to work ?
>
> How widespread HMM use will be is hard to guess. Usual chicken
> and egg plus adoption thing. If GPGPU compte keeps growing and
> it seems it does then HMM likely gonna be enable and actively
> use for large chunk of those computer that have GPGPU workload.
>
> I will test a static key of that branch and see if it explodes
> because put_page() is an inline function.

memcpy_mcsafe() is an existing example of a static inline with a
static branch. Hasn't seemed to have caused any problems to date.

[toc] | [prev] | [next] | [standalone]

#1647408 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, May 22, 2017 at 01:19:29PM -0700, Dan Williams wrote:
> On Mon, May 22, 2017 at 1:14 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> > On Mon, May 22, 2017 at 12:29:53PM -0700, Dan Williams wrote:
> >> On Mon, May 22, 2017 at 9:51 AM, Jérôme Glisse <jglisse@redhat.com> wrote:
> >> > A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
> >> > have any user. For device private pages this is important to catch
> >> > and thus we need to special case put_page() for this.
> >> >
> >> > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> >> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> >> > ---
> >> >  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
> >> >  kernel/memremap.c  |  1 -
> >> >  2 files changed, 30 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> > index a825dab..11f7bac 100644
> >> > --- a/include/linux/mm.h
> >> > +++ b/include/linux/mm.h
> >> > @@ -23,6 +23,7 @@
> >> >  #include <linux/page_ext.h>
> >> >  #include <linux/err.h>
> >> >  #include <linux/page_ref.h>
> >> > +#include <linux/memremap.h>
> >> >
> >> >  struct mempolicy;
> >> >  struct anon_vma;
> >> > @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
> >> >         return ((page_zonenum(page) == ZONE_DEVICE) &&
> >> >                 (page->pgmap->type == MEMORY_DEVICE_PRIVATE));
> >> >  }
> >> > +
> >> > +static inline void put_zone_device_private_page(struct page *page)
> >> > +{
> >> > +       int count = page_ref_dec_return(page);
> >> > +
> >> > +       /*
> >> > +        * If refcount is 1 then page is freed and refcount is stable as nobody
> >> > +        * holds a reference on the page.
> >> > +        */
> >> > +       if (count == 1)
> >> > +               page->pgmap->page_free(page, page->pgmap->data);
> >> > +       else if (!count)
> >> > +               __put_page(page);
> >> > +}
> >> >  #else
> >> >  static inline bool is_zone_device_page(const struct page *page)
> >> >  {
> >> > @@ -805,6 +820,10 @@ static inline bool is_device_private_page(const struct page *page)
> >> >  {
> >> >         return false;
> >> >  }
> >> > +
> >> > +static inline void put_zone_device_private_page(struct page *page)
> >> > +{
> >> > +}
> >> >  #endif
> >> >
> >> >  static inline void get_page(struct page *page)
> >> > @@ -822,6 +841,17 @@ static inline void put_page(struct page *page)
> >> >  {
> >> >         page = compound_head(page);
> >> >
> >> > +       /*
> >> > +        * For private device pages we need to catch refcount transition from
> >> > +        * 2 to 1, when refcount reach one it means the private device page is
> >> > +        * free and we need to inform the device driver through callback. See
> >> > +        * include/linux/memremap.h and HMM for details.
> >> > +        */
> >> > +       if (unlikely(is_device_private_page(page))) {
> >>
> >> Since I presume HMM is a niche use case can we make this a
> >> "static_branch_unlikely(&hmm_key) && is_device_private_page(page))"?
> >> That way non-hmm platforms see minimal overhead.
> >
> > Like i said in the cover letter i am bit anxious about doing for
> 
> I don't think you copied me on the cover letter.
> 
> > an inline function. I don't see any existing case for inline
> > function and static key. Is that suppose to work ?
> >
> > How widespread HMM use will be is hard to guess. Usual chicken
> > and egg plus adoption thing. If GPGPU compte keeps growing and
> > it seems it does then HMM likely gonna be enable and actively
> > use for large chunk of those computer that have GPGPU workload.
> >
> > I will test a static key of that branch and see if it explodes
> > because put_page() is an inline function.
> 
> memcpy_mcsafe() is an existing example of a static inline with a
> static branch. Hasn't seemed to have caused any problems to date.

Ok i will post a new version of 08 with static keys, i shouldn't
need to repost any other patches for that. Andrew that's fine
with you or do you prefer me to repost a full updated patchset ?

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1647346 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, 22 May 2017, Jerome Glisse wrote:
> On Mon, May 22, 2017 at 12:29:53PM -0700, Dan Williams wrote:
> > On Mon, May 22, 2017 at 9:51 AM, Jerome Glisse <jglisse@redhat.com> wrote:
> > > A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
> > > have any user. For device private pages this is important to catch
> > > and thus we need to special case put_page() for this.
> > >
> > > Signed-off-by: Jerome Glisse <jglisse@redhat.com>
> > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > ---
> > >  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
> > >  kernel/memremap.c  |  1 -
> > >  2 files changed, 30 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index a825dab..11f7bac 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -23,6 +23,7 @@
> > >  #include <linux/page_ext.h>
> > >  #include <linux/err.h>
> > >  #include <linux/page_ref.h>
> > > +#include <linux/memremap.h>
> > >
> > >  struct mempolicy;
> > >  struct anon_vma;
> > > @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
> > >         return ((page_zonenum(page) == ZONE_DEVICE) &&
> > >                 (page->pgmap->type == MEMORY_DEVICE_PRIVATE));
> > >  }
> > > +
> > > +static inline void put_zone_device_private_page(struct page *page)
> > > +{
> > > +       int count = page_ref_dec_return(page);
> > > +
> > > +       /*
> > > +        * If refcount is 1 then page is freed and refcount is stable as nobody
> > > +        * holds a reference on the page.
> > > +        */
> > > +       if (count == 1)
> > > +               page->pgmap->page_free(page, page->pgmap->data);
> > > +       else if (!count)
> > > +               __put_page(page);
> > > +}

Is there something else in this patchset that guarantees
that get_page_unless_zero() is never used on thse pages?
We have plenty of code that knows that refcount 0 is special:
having to know that refcount 1 may be special is worrying.

Hugh

> > >  #else
> > >  static inline bool is_zone_device_page(const struct page *page)
> > >  {
> > > @@ -805,6 +820,10 @@ static inline bool is_device_private_page(const struct page *page)
> > >  {
> > >         return false;
> > >  }
> > > +
> > > +static inline void put_zone_device_private_page(struct page *page)
> > > +{
> > > +}
> > >  #endif
> > >
> > >  static inline void get_page(struct page *page)
> > > @@ -822,6 +841,17 @@ static inline void put_page(struct page *page)
> > >  {
> > >         page = compound_head(page);
> > >
> > > +       /*
> > > +        * For private device pages we need to catch refcount transition from
> > > +        * 2 to 1, when refcount reach one it means the private device page is
> > > +        * free and we need to inform the device driver through callback. See
> > > +        * include/linux/memremap.h and HMM for details.
> > > +        */
> > > +       if (unlikely(is_device_private_page(page))) {
> > 
> > Since I presume HMM is a niche use case can we make this a
> > "static_branch_unlikely(&hmm_key) && is_device_private_page(page))"?
> > That way non-hmm platforms see minimal overhead.
> 
> Like i said in the cover letter i am bit anxious about doing for
> an inline function. I don't see any existing case for inline
> function and static key. Is that suppose to work ?
> 
> How widespread HMM use will be is hard to guess. Usual chicken
> and egg plus adoption thing. If GPGPU compte keeps growing and
> it seems it does then HMM likely gonna be enable and actively
> use for large chunk of those computer that have GPGPU workload.
> 
> I will test a static key of that branch and see if it explodes
> because put_page() is an inline function.
> 
> Cheers,
> Jerome

[toc] | [prev] | [next] | [standalone]

#1647394 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, May 22, 2017 at 01:22:22PM -0700, Hugh Dickins wrote:
> On Mon, 22 May 2017, Jerome Glisse wrote:
> > On Mon, May 22, 2017 at 12:29:53PM -0700, Dan Williams wrote:
> > > On Mon, May 22, 2017 at 9:51 AM, Jerome Glisse <jglisse@redhat.com> wrote:
> > > > A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
> > > > have any user. For device private pages this is important to catch
> > > > and thus we need to special case put_page() for this.
> > > >
> > > > Signed-off-by: Jerome Glisse <jglisse@redhat.com>
> > > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > > ---
> > > >  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
> > > >  kernel/memremap.c  |  1 -
> > > >  2 files changed, 30 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > index a825dab..11f7bac 100644
> > > > --- a/include/linux/mm.h
> > > > +++ b/include/linux/mm.h
> > > > @@ -23,6 +23,7 @@
> > > >  #include <linux/page_ext.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/page_ref.h>
> > > > +#include <linux/memremap.h>
> > > >
> > > >  struct mempolicy;
> > > >  struct anon_vma;
> > > > @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
> > > >         return ((page_zonenum(page) == ZONE_DEVICE) &&
> > > >                 (page->pgmap->type == MEMORY_DEVICE_PRIVATE));
> > > >  }
> > > > +
> > > > +static inline void put_zone_device_private_page(struct page *page)
> > > > +{
> > > > +       int count = page_ref_dec_return(page);
> > > > +
> > > > +       /*
> > > > +        * If refcount is 1 then page is freed and refcount is stable as nobody
> > > > +        * holds a reference on the page.
> > > > +        */
> > > > +       if (count == 1)
> > > > +               page->pgmap->page_free(page, page->pgmap->data);
> > > > +       else if (!count)
> > > > +               __put_page(page);
> > > > +}
> 
> Is there something else in this patchset that guarantees
> that get_page_unless_zero() is never used on thse pages?
> We have plenty of code that knows that refcount 0 is special:
> having to know that refcount 1 may be special is worrying.
> 
> Hugh

ZONE_DEVICE pages always had this extra refcount since their
inception. All the place that use get_page_unless_zero() should
be unreachable by a ZONE_DEVICE pages (hwpoison, lru, isolate,
ksm, ...). So if that happens it is a bug.

Jérôme

[toc] | [prev] | [next] | [standalone]

#1648045 — Re: [HMM 08/15] mm/ZONE_DEVICE: special case put_page() for device private pages

On Mon, May 22, 2017 at 12:51:59PM -0400, Jérôme Glisse wrote:
> A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
> have any user. For device private pages this is important to catch
> and thus we need to special case put_page() for this.
> 
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  include/linux/mm.h | 30 ++++++++++++++++++++++++++++++
>  kernel/memremap.c  |  1 -
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a825dab..11f7bac 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -23,6 +23,7 @@
>  #include <linux/page_ext.h>
>  #include <linux/err.h>
>  #include <linux/page_ref.h>
> +#include <linux/memremap.h>
>  
>  struct mempolicy;
>  struct anon_vma;
> @@ -795,6 +796,20 @@ static inline bool is_device_private_page(const struct page *page)
>  	return ((page_zonenum(page) == ZONE_DEVICE) &&
>  		(page->pgmap->type == MEMORY_DEVICE_PRIVATE));
>  }
> +
> +static inline void put_zone_device_private_page(struct page *page)

Could you measure how much bloat this change produce?

I would rather make put_zone_device_private_page() non-inline. put_page()
is inlined everewhere. It's beneficial to keep it skinny.

(And I guess it would help solving 0-day reporeted build issue).

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1648812 — [HMM 08/18] mm/ZONE_DEVICE: special case put_page() for device private pages v2

A ZONE_DEVICE page that reach a refcount of 1 is free ie no longer
have any user. For device private pages this is important to catch
and thus we need to special case put_page() for this.

Changed since v1:
  - use static key to disable special code path in put_page() by
    default
  - uninline put_zone_device_private_page()
  - fix build issues with some kernel config related to header
    inter-dependency

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 include/linux/memremap.h | 13 +++++++++++++
 include/linux/mm.h       | 31 ++++++++++++++++++++++---------
 kernel/memremap.c        | 19 ++++++++++++++++++-
 mm/page_alloc.c          |  6 ++++++
 4 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 0fcf840..0e0d2e6 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -125,6 +125,14 @@ struct dev_pagemap {
 void *devm_memremap_pages(struct device *dev, struct resource *res,
 		struct percpu_ref *ref, struct vmem_altmap *altmap);
 struct dev_pagemap *find_dev_pagemap(resource_size_t phys);
+
+static inline bool is_zone_device_page(const struct page *page);
+
+static inline bool is_device_private_page(const struct page *page)
+{
+	return is_zone_device_page(page) &&
+		page->pgmap->type == MEMORY_DEVICE_PRIVATE;
+}
 #else
 static inline void *devm_memremap_pages(struct device *dev,
 		struct resource *res, struct percpu_ref *ref,
@@ -143,6 +151,11 @@ static inline struct dev_pagemap *find_dev_pagemap(resource_size_t phys)
 {
 	return NULL;
 }
+
+static inline bool is_device_private_page(const struct page *page)
+{
+	return false;
+}
 #endif
 
 /**
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a825dab..7f0656f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -23,6 +23,7 @@
 #include <linux/page_ext.h>
 #include <linux/err.h>
 #include <linux/page_ref.h>
+#include <linux/memremap.h>
 
 struct mempolicy;
 struct anon_vma;
@@ -788,25 +789,25 @@ static inline bool is_zone_device_page(const struct page *page)
 {
 	return page_zonenum(page) == ZONE_DEVICE;
 }
-
-static inline bool is_device_private_page(const struct page *page)
-{
-	/* See MEMORY_DEVICE_PRIVATE in include/linux/memory_hotplug.h */
-	return ((page_zonenum(page) == ZONE_DEVICE) &&
-		(page->pgmap->type == MEMORY_DEVICE_PRIVATE));
-}
 #else
 static inline bool is_zone_device_page(const struct page *page)
 {
 	return false;
 }
+#endif
 
-static inline bool is_device_private_page(const struct page *page)
+#ifdef CONFIG_DEVICE_PRIVATE
+void put_zone_device_private_page(struct page *page);
+#else
+static inline void put_zone_device_private_page(struct page *page)
 {
-	return false;
 }
 #endif
 
+static inline bool is_device_private_page(const struct page *page);
+
+DECLARE_STATIC_KEY_FALSE(device_private_key);
+
 static inline void get_page(struct page *page)
 {
 	page = compound_head(page);
@@ -822,6 +823,18 @@ static inline void put_page(struct page *page)
 {
 	page = compound_head(page);
 
+	/*
+	 * For private device pages we need to catch refcount transition from
+	 * 2 to 1, when refcount reach one it means the private device page is
+	 * free and we need to inform the device driver through callback. See
+	 * include/linux/memremap.h and HMM for details.
+	 */
+	if (static_branch_unlikely(&device_private_key) &&
+	    unlikely(is_device_private_page(page))) {
+		put_zone_device_private_page(page);
+		return;
+	}
+
 	if (put_page_testzero(page))
 		__put_page(page);
 }
diff --git a/kernel/memremap.c b/kernel/memremap.c
index cd596d4..b9baa6c 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -11,7 +11,6 @@
  * General Public License for more details.
  */
 #include <linux/radix-tree.h>
-#include <linux/memremap.h>
 #include <linux/device.h>
 #include <linux/types.h>
 #include <linux/pfn_t.h>
@@ -464,3 +463,21 @@ struct vmem_altmap *to_vmem_altmap(unsigned long memmap_start)
 	return pgmap ? pgmap->altmap : NULL;
 }
 #endif /* CONFIG_ZONE_DEVICE */
+
+
+#ifdef CONFIG_DEVICE_PRIVATE
+void put_zone_device_private_page(struct page *page)
+{
+	int count = page_ref_dec_return(page);
+
+	/*
+	 * If refcount is 1 then page is freed and refcount is stable as nobody
+	 * holds a reference on the page.
+	 */
+	if (count == 1)
+		page->pgmap->page_free(page, page->pgmap->data);
+	else if (!count)
+		__put_page(page);
+}
+EXPORT_SYMBOL(put_zone_device_private_page);
+#endif /* CONFIG_DEVICE_PRIVATE */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e7521ec..2a9dce5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -72,6 +72,12 @@
 #include <asm/div64.h>
 #include "internal.h"
 
+/*
+ * Device private memory see HMM (Documentation/vm/hmm.txt) or hmm.h
+ */
+DEFINE_STATIC_KEY_FALSE(device_private_key);
+EXPORT_SYMBOL(device_private_key);
+
 /* prevent >1 _updater_ of zone percpu pageset ->high and ->batch fields */
 static DEFINE_MUTEX(pcp_batch_high_lock);
 #define MIN_PERCPU_PAGELIST_FRACTION	(8)
-- 
2.9.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web