search for: memory_device_generic

...s 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.

...ue. 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 n...

On Fri, Nov 06, 2020 at 01:26:50PM -0800, Ralph Campbell wrote: > > On 11/6/20 12:03 AM, Christoph Hellwig wrote: >> I hate the extra pin count magic here. IMHO we really need to finish >> off the series to get rid of the extra references on the ZONE_DEVICE >> pages first. > > First, thanks for the review comments. > > I don't like the extra refcount

...mean time. > > I think P2P is pretty trivial, given that ZONE_DEVICE pages are used like > a normal memory allocator. DAX is the interesting case, any specific > help that you need with that? There are 4 types of ZONE_DEVICE struct pages: MEMORY_DEVICE_PRIVATE, MEMORY_DEVICE_FS_DAX, MEMORY_DEVICE_GENERIC, and MEMORY_DEVICE_PCI_P2PDMA. Currently, memremap_pages() allocates struct pages for a physical address range with a page_ref_count(page) of one and increments the pgmap->ref per CPU reference count by the number of pages created since each ZONE_DEVICE struct page has a pointer to the pgmap....

...freeing the page in hmm_dmirror_exit? > int error, is_ram; > - bool need_devmap_managed = true; > > switch (pgmap->type) { > case MEMORY_DEVICE_PRIVATE: > @@ -217,11 +171,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) > } > break; > case MEMORY_DEVICE_GENERIC: The MEMORY_DEVICE_PRIVATE cases loses the sanity check that the page_free method is set. Otherwise this looks good.

...e_add_file_rmap() set_pte_at() else insert_pfn() pte_mkdevmap() set_pte_at() Should pmem set the page reference count to one before inserting the pfn into the page tables (and decrement when removing devmap PTEs)? What about MEMORY_DEVICE_GENERIC and MEMORY_DEVICE_PCI_P2PDMA use cases? Where should they icrement/decrement the page reference count? I don't know enough about how these are used to really know what to do at this point. If people want me to continue to work on this series, I will need some guidance. --- Matthew Wilcox, Ira...

...>> > 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...

...if (!pgmap->ops || + !pgmap->ops->migrate_to_ram || !pgmap->ops->page_free) { + WARN(1, "Missing ops\n"); return ERR_PTR(-EINVAL); } if (!pgmap->owner) { @@ -348,11 +318,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) } break; case MEMORY_DEVICE_GENERIC: - need_devmap_managed = false; break; case MEMORY_DEVICE_PCI_P2PDMA: params.pgprot = pgprot_noncached(params.pgprot); - need_devmap_managed = false; break; default: WARN(1, "Invalid pgmap type %d\n", pgmap->type); @@ -376,12 +344,6 @@ void *memremap_pages(struct dev_...

...gt;migrate_to_ram || !pgmap->ops->page_free) { > + WARN(1, "Missing ops\n"); > return ERR_PTR(-EINVAL); > } > if (!pgmap->owner) { > @@ -348,11 +318,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) > } > break; > case MEMORY_DEVICE_GENERIC: > - need_devmap_managed = false; > break; > case MEMORY_DEVICE_PCI_P2PDMA: > params.pgprot = pgprot_noncached(params.pgprot); > - need_devmap_managed = false; > break; > default: > WARN(1, "Invalid pgmap type %d\n", pgmap->type); > @@...

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, >>

Matthew Wilcox, Ira Weiny, and others have complained that ZONE_DEVICE struct page reference counting is ugly because they are "free" when the reference count is one instead of zero. This leads to explicit checks for ZONE_DEVICE pages in places like put_page(), GUP, THP splitting, and page migration which have to adjust the expected reference count when determining if the page is

...;owner) { WARN(1, "Missing owner\n"); return ERR_PTR(-EINVAL); @@ -355,13 +343,12 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) WARN(1, "File system DAX not supported\n"); return ERR_PTR(-EINVAL); } + need_devmap_managed = true; break; case MEMORY_DEVICE_GENERIC: - need_devmap_managed = false; break; case MEMORY_DEVICE_PCI_P2PDMA: params.pgprot = pgprot_noncached(params.pgprot); - need_devmap_managed = false; break; default: WARN(1, "Invalid pgmap type %d\n", pgmap->type); @@ -508,10 +495,13 @@ EXPORT_SYMBOL_GPL(get_dev_pagem...

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

...*memremap_pages(struct dev_pagemap *pgmap, int nid) .pgprot = PAGE_KERNEL, }; int error, is_ram; - bool need_devmap_managed = true; switch (pgmap->type) { case MEMORY_DEVICE_PRIVATE: @@ -217,11 +171,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) } break; case MEMORY_DEVICE_GENERIC: - need_devmap_managed = false; break; case MEMORY_DEVICE_PCI_P2PDMA: params.pgprot = pgprot_noncached(params.pgprot); - need_devmap_managed = false; break; default: WARN(1, "Invalid pgmap type %d\n", pgmap->type); @@ -245,12 +197,6 @@ void *memremap_pages(struct dev_...

...t;page_free) { + WARN(1, "Missing page_free method\n"); + return ERR_PTR(-EINVAL); + } if (!pgmap->owner) { WARN(1, "Missing owner\n"); return ERR_PTR(-EINVAL); @@ -356,11 +318,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) } break; case MEMORY_DEVICE_GENERIC: - need_devmap_managed = false; break; case MEMORY_DEVICE_PCI_P2PDMA: params.pgprot = pgprot_noncached(params.pgprot); - need_devmap_managed = false; break; default: WARN(1, "Invalid pgmap type %d\n", pgmap->type); @@ -384,12 +344,6 @@ void *memremap_pages(struct dev_...

...;owner) { WARN(1, "Missing owner\n"); return ERR_PTR(-EINVAL); @@ -355,13 +343,12 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) WARN(1, "File system DAX not supported\n"); return ERR_PTR(-EINVAL); } + need_devmap_managed = true; break; case MEMORY_DEVICE_GENERIC: - need_devmap_managed = false; break; case MEMORY_DEVICE_PCI_P2PDMA: params.pgprot = pgprot_noncached(params.pgprot); - need_devmap_managed = false; break; default: WARN(1, "Invalid pgmap type %d\n", pgmap->type); @@ -508,10 +495,13 @@ EXPORT_SYMBOL_GPL(get_dev_pagem...