Nouveau - Sep 2020 - [PATCH] mm: remove extra ZONE_DEVICE struct 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.

On Tue, Sep 15, 2020 at 09:39:47AM -0700, Ralph Campbell
wrote:
>> 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.
Umm, the whole point of ZONE_DEVICE is to have a struct page for
something that is not system memory.  For both the ppc kvm case (magic
hypervisor pool) and Noveau (device internal) memory that clear is the
case.  But looks like test_hmm uses normal pages to fake this up, so
I was wrong about the third caller.  But I think we can just call
set_page_count just before freeing the page there with a comment
explaining what is goin on.

On 9/15/20 10:36 PM, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 09:39:47AM -0700, Ralph Campbell wrote:
>>> 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.
> 
> Umm, the whole point of ZONE_DEVICE is to have a struct page for
> something that is not system memory.  For both the ppc kvm case (magic
> hypervisor pool) and Noveau (device internal) memory that clear is the
> case.  But looks like test_hmm uses normal pages to fake this up, so
> I was wrong about the third caller.  But I think we can just call
> set_page_count just before freeing the page there with a comment
> explaining what is goin on.
Dan Williams thought that having the ZONE_DEVICE struct pages
be on a free list with a refcount of one was a bit strange and
that the driver should handle the zero to one transition.
But, that would mean a bit more invasive change to the 3 drivers
to set the reference count to zero after calling memremap_pages()
and setting the reference count to one when allocating a struct
page. What you are suggesting is what I also proposed in v1.