Nouveau - Sep 2020 - [PATCH] mm: remove extra ZONE_DEVICE struct page refcount

On 9/14/20 4:10 PM, Dan Williams wrote:
> On Mon, Sep 14, 2020 at 3:45 PM Ralph Campbell <rcampbell at
nvidia.com> wrote:
>>
>> ZONE_DEVICE struct pages have an extra reference count that complicates
the
>> code for put_page() and several places in the kernel that need to check
the
>> reference count to see that a page is not being used (gup, compaction,
>> migration, etc.). Clean up the code so the reference count doesn't
need to
>> be treated specially for ZONE_DEVICE.
>>
>> Signed-off-by: Ralph Campbell <rcampbell at nvidia.com>
>> ---
>>
>> Matthew Wilcox, Ira Weiny, and others have complained that ZONE_DEVICE
>> struct page reference counting is ugly/broken. This is my attempt to
>> fix it and it works for the HMM migration self tests.
> 
> Can you link to a technical description of what's broken? Or better
> yet, summarize that argument in the changelog?
> 
>> I'm only sending this out as a RFC since I'm not that familiar
with the
>> DAX, PMEM, XEN, and other uses of ZONE_DEVICE struct pages allocated
>> with devm_memremap_pages() or memremap_pages() but my best reading of
>> the code looks like it might be OK. I could use help testing these
>> configurations.
> 
> Back in the 4.15 days I could not convince myself that some code paths
> blindly assumed that pages with refcount==0 were on an lru list. Since
> then, struct page has been reorganized to not collide the ->pgmap back
> pointer with the ->lru list and there have been other cleanups for
> page pinning that might make this incremental cleanup viable.
> 
> You also need to fix up ext4_break_layouts() and
> xfs_break_dax_layouts() to expect ->_refcount is 0 instead of 1. This
> also needs some fstests exposure.
Got it. Thanks!
>> I have a modified THP migration patch series that applies on top of
>> this one and is cleaner since I don't have to add code to handle
the
>> +1 reference count. The link below is for the earlier v2:
>> ("mm/hmm/nouveau: add THP migration to migrate_vma_*")
>> https://lore.kernel.org/linux-mm/20200902165830.5367-1-rcampbell at
nvidia.com
>>
>>
>>   arch/powerpc/kvm/book3s_hv_uvmem.c     |  1 -
>>   drivers/gpu/drm/nouveau/nouveau_dmem.c |  1 -
>>   include/linux/memremap.h               |  6 +--
>>   include/linux/mm.h                     | 39 ---------------
>>   lib/test_hmm.c                         |  1 -
>>   mm/gup.c                               | 44 -----------------
>>   mm/memremap.c                          | 20 ++++----
>>   mm/migrate.c                           |  5 --
>>   mm/swap.c                              | 66
+++++++++++---------------
>>   9 files changed, 41 insertions(+), 142 deletions(-)
> 
> This diffstat is indeed appealing.
> 
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c
b/arch/powerpc/kvm/book3s_hv_uvmem.c
>> index 84e5a2dc8be5..00d97050d7ff 100644
>> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
>> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
>> @@ -711,7 +711,6 @@ static struct page *kvmppc_uvmem_get_page(unsigned
long gpa, struct kvm *kvm)
>>
>>          dpage = pfn_to_page(uvmem_pfn);
>>          dpage->zone_device_data = pvt;
>> -       get_page(dpage);
>>          lock_page(dpage);
>>          return dpage;
>>   out_clear:
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c
b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> index a13c6215bba8..2a4bbe01a455 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
>> @@ -324,7 +324,6 @@ nouveau_dmem_page_alloc_locked(struct nouveau_drm
*drm)
>>                          return NULL;
>>          }
>>
>> -       get_page(page);
>>          lock_page(page);
>>          return page;
>>   }
>> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
>> index 4e9c738f4b31..7dd9802d2612 100644
>> --- a/include/linux/memremap.h
>> +++ b/include/linux/memremap.h
>> @@ -67,9 +67,9 @@ enum memory_type {
>>
>>   struct dev_pagemap_ops {
>>          /*
>> -        * Called once the page refcount reaches 1.  (ZONE_DEVICE pages
never
>> -        * reach 0 refcount unless there is a refcount bug. This allows
the
>> -        * device driver to implement its own memory management.)
>> +        * Called once the page refcount reaches 0. The reference count
is
>> +        * reset to 1 before calling page_free(). This allows the
>> +        * device driver to implement its own memory management.
> 
> I'd clarify the order events / responsibility of the common core
> page_free() and the device specific page_free(). At the same time, why
> not update drivers to expect that the page is already refcount==0 on
> entry? Seems odd to go through all this trouble to make the reference
> count appear to be zero to the wider kernel but expect that drivers
> get a fake reference on entry to their ->page_free() callbacks.
Good point.

Since set_page_refcounted() is defined in mm_interal.h I would have to
move the definition to someplace like page_ref.h or have the drivers
cal init_page_count() or set_page_count() since get_page() calls
VM_BUG_ON_PAGE() if refcount == 0.
I'll move set_page_refcounted() since that is what the page allocator
uses and seems named for the purpose.

On Mon, Sep 14, 2020 at 04:53:25PM -0700, Ralph Campbell
wrote:
> Since set_page_refcounted() is defined in mm_interal.h I would have to
> move the definition to someplace like page_ref.h or have the drivers
> cal init_page_count() or set_page_count() since get_page() calls
> VM_BUG_ON_PAGE() if refcount == 0.
> I'll move set_page_refcounted() since that is what the page allocator
> uses and seems named for the purpose.
I don't think any of the three ->page_free instances even cares about
the page refcount.

On 9/15/20 9:29 AM, Christoph Hellwig wrote:
> On Mon, Sep 14, 2020 at 04:53:25PM -0700, Ralph Campbell wrote:
>> Since set_page_refcounted() is defined in mm_interal.h I would have to
>> move the definition to someplace like page_ref.h or have the drivers
>> cal init_page_count() or set_page_count() since get_page() calls
>> VM_BUG_ON_PAGE() if refcount == 0.
>> I'll move set_page_refcounted() since that is what the page
allocator
>> uses and seems named for the purpose.
> 
> I don't think any of the three ->page_free instances even cares
about
> the page refcount.
> 
Not true. The page_free() callback records the page is free by setting
a bit or putting the page on a free list but when it allocates a free
device private struct page to be used with migrate_vma_setup(), it needs to
increment the refcount.

For the ZONE_DEVICE MEMORY_DEVICE_GENERIC and MEMORY_DEVICE_PCI_P2PDMA
struct pages, I think you are correct because they don't define page_free()
and from what I can see, don't decrement the page refcount to zero.