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

[Jerome Glisse]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> > ---
> >  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