mm: make device private reference counts zero based
Subsystems (11)
PATCH 1/1 mm: make device private reference counts zero based
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 device private pages, leaving DAX as still being a special case.
---
I'm sending this as a separate patch since I think it is ready to merge. Originally, this was part of an RFC:
https://lore.kernel.org/linux-mm/[email protected]
and is changed to only make device private struct page reference counts be zero based since DAX needs to detect when a page is not referenced by GUP and not no references at all.
It applies cleanly to linux-5.9.0-rc8-mm1 plus my patch ("ext4/xfs: add page refcount helper")
https://lore.kernel.org/linux-mm/[email protected]
but doesn't really depend on it, just a simple merge conflict without it.
This is for Andrew Morton after the 5.10.0-rc1 merge window closes.
arch/powerpc/kvm/book3s_hv_uvmem.c | 13 +++++++- drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +- include/linux/memremap.h | 6 ++-- include/linux/mm.h | 9 +----- lib/test_hmm.c | 7 ++++- mm/internal.h | 8 +++++ mm/memremap.c | 42 ++++++++++---------------- mm/migrate.c | 5 --- mm/swap.c | 6 ++-- 9 files changed, 51 insertions(+), 47 deletions(-)
arch/powerpc/kvm/book3s_hv_uvmem.c +12 -1
@@ -711,7 +711,7 @@ 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); +init_page_count (dpage);lock_page (dpage);return dpage; out_clear:
@@ -1151,6 +1151,7 @@ int kvmppc_uvmem_init(void)struct resource *res;void *addr;unsigned long pfn_last, pfn_first; +unsigned long pfn; size =kvmppc_get_secmem_size ();if (!size) {
@@ -1191,6 +1192,16 @@ int kvmppc_uvmem_init(void) goto out_unmap; } +/* +* Pages are created with an initial reference count of one but should +* have a reference count of zero while in the free state. +*/ +for (pfn = pfn_first; pfn < pfn_last; pfn++) { +struct page *dpage =pfn_to_page (pfn); + +set_page_count (dpage,0 ); + } +pr_info ("KVMPPC-UVMEM: Secure Memory size 0x%lx\n" , size);return ret; out_unmap:
drivers/gpu/drm/nouveau/nouveau_dmem.c +1 -1
@@ -324,7 +324,7 @@ nouveau_dmem_page_alloc_locked(struct nouveau_drm *drm)return NULL ; } -get_page (page); +init_page_count (page);lock_page (page);return page; }
include/linux/memremap.h +3 -3
@@ -66,9 +66,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 should +* be reset to one with init_page_count(page) before reusing the page. +* This allows device drivers to implement their own memory management. */ void (*page_free)(struct page *page);
include/linux/mm.h +1 -8
@@ -1110,14 +1110,7 @@ static inline bool page_is_devmap_managed(struct page *page)return false;if (!is_zone_device_page (page))return false; -switch (page->pgmap->type) { -case MEMORY_DEVICE_PRIVATE : -case MEMORY_DEVICE_FS_DAX : -return true; -default : -break ; - } -return false; +return page->pgmap->type ==MEMORY_DEVICE_FS_DAX ; }void put_devmap_managed_page (struct page *page);
lib/test_hmm.c +6 -1
@@ -509,10 +509,15 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice, mdevice->devmem_count * (DEVMEM_CHUNK_SIZE / (1024 *1024 )), pfn_first, pfn_last); +/* +* Pages are created with an initial reference count of one but should +* have a reference count of zero while in the free state. +*/ spin_lock (&mdevice->lock);for (pfn = pfn_first; pfn < pfn_last; pfn++) {struct page *page =pfn_to_page (pfn); +set_page_count (page,0 ); page->zone_device_data = mdevice->free_pages; mdevice->free_pages = page; }
@@ -561,7 +566,7 @@ static struct page *dmirror_devmem_alloc_page(struct dmirror_device *mdevice) } dpage->zone_device_data = rpage; -get_page (dpage); +init_page_count (dpage);lock_page (dpage);return dpage;
mm/internal.h +8 -0
@@ -618,4 +618,12 @@ struct migration_target_control { gfp_t gfp_mask; }; +#ifdef CONFIG_DEV_PAGEMAP_OPS +void free_zone_device_page (struct page *page); +#else +static inline void free_zone_device_page (struct page *page) +{ +} +#endif +#endif /* __MM_INTERNAL_H */
mm/memremap.c +16 -26
106 lines not on this page · raw message
mm/migrate.c +0 -5
@@ -380,11 +380,6 @@ static int expected_page_refs(struct address_space *mapping, struct page *page) {int expected_count =1 ; -/* -* Device private pages have an extra refcount as they are -* ZONE_DEVICE pages. -*/ - expected_count +=is_device_private_page (page);if (mapping) expected_count +=thp_nr_pages (page) +page_has_private (page);
mm/swap.c +4 -2
@@ -116,12 +116,11 @@ static void __put_compound_page(struct page *page)void __put_page (struct page *page) {if (is_zone_device_page (page)) { -put_dev_pagemap (page->pgmap); -/* * The page belongs to the device that created pgmap. Do * not return it to page allocator. */ +free_zone_device_page (page);return ; }
@@ -907,6 +906,9 @@ void release_pages(struct page **pages, int nr)put_devmap_managed_page (page);continue ; } +if (put_page_testzero (page)) +__put_page (page); +continue ; }if (!put_page_testzero (page))
Signature · 1 line
2.20.1
Quoted 6 lines · Ralph Campbell
ZONE_DEVICEstruct pages have an extra reference count that complicates the code forput_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 device private pages, leaving DAX as still being a special case.
What about the check in mc_handle_swap_pte()?
mm/memcontrol.c:
5513 /*
5514 * MEMORY_DEVICE_PRIVATE means ZONE_DEVICE page and which have 5515 * a refcount of 1 when free (unlike normal page) 5516 */
5517 if (!page_ref_add_unless(page, 1, 1))
5518 return NULL;
... does that need to change? Perhaps just the comment?
Quoted 4 lines
Signed-off-by Ralph Campbell <[email protected]>---
[snip]
Quoted 9 lines
void
put_devmap_managed_page(struct page *page);lib/test_hmm.c +0
-0@@ -509,10 +509,15 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice, mdevice->devmem_count * (DEVMEM_CHUNK_SIZE / (1024 *1024 )), pfn_first, pfn_last);
lib/test_hmm.c static bool dmirror_allocate_chunk(struct dmirror_device *mdevice, · line 520
Quoted 10 lines
lib/test_hmm.c +5
-0+/* +* Pages are created with an initial reference count of one but should +* have a reference count of zero while in the free state. +*/ spin_lock (&mdevice->lock);for (pfn = pfn_first; pfn < pfn_last; pfn++) {struct page *page =pfn_to_page (pfn); +set_page_count (page,0 );
This confuses me. How does this and init_page_count() not confuse the buddy allocator? Don't you have to reset the refcount somewhere after the test?
Quoted 17 lines
lib/test_hmm.c +1
-1page->zone_device_data = mdevice->free_pages; mdevice->free_pages = page; }@@ -561,7 +566,7 @@ static struct page *dmirror_devmem_alloc_page(struct dmirror_device *mdevice) } dpage->zone_device_data = rpage; -get_page (dpage); +init_page_count (dpage);lock_page (dpage);return dpage;mm/internal.h +0
-0
[snip]
Quoted 5 lines
mm/swap.c +0
-0
mm/swap.c static void __put_compound_page(struct page *page) · line 123
Quoted 10 lines
mm/swap.c +1
-2@@ -116,12 +116,11 @@ static void __put_compound_page(struct page *page)void __put_page (struct page *page) {if (is_zone_device_page (page)) { -put_dev_pagemap (page->pgmap); -/* * The page belongs to the device that created pgmap. Do * not return it to page allocator. */ +free_zone_device_page (page);
I really like this.
Ira
Quoted 12 lines · Ira Weiny
Quoted 6 lines · Ralph Campbell
ZONE_DEVICEstruct pages have an extra reference count that complicates the code forput_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 device private pages, leaving DAX as still being a special case.What about the check in
mc_handle_swap_pte()?
mm/memcontrol.c:
Quoted 8 lines
5513 /*
5514 * MEMORY_DEVICE_PRIVATE means ZONE_DEVICE page and which have 5515 * a refcount of 1 when free (unlike normal page) 5516 */5517 if (!page_ref_add_unless(page, 1, 1))
5518 return NULL;... does that need to change? Perhaps just the comment?
Thanks for spotting this.
Actually, this whole bit of code is never reached because the
if (non_swap_entry(ent))
return NULL;
covers device private pages and returns.
The device private pages are accounted for in mem_cgroup so this probably needs fixing. I'll probably move the code before the non_swap_entry() check and change the refcount increment to page_ref_add_unless(page, 1, 0).
Quoted 25 lines
Quoted 4 lines
Signed-off-by Ralph Campbell <[email protected]>---
[snip]
Quoted 18 lines
void put_devmap_managed_page(struct page *page);lib/test_hmm.c +4
-0@@ -509,10 +509,15 @@ static bool dmirror_allocate_chunk(struct dmirror_device *mdevice, mdevice->devmem_count * (DEVMEM_CHUNK_SIZE / (1024 *1024 )), pfn_first, pfn_last); +/* +* Pages are created with an initial reference count of one but should +* have a reference count of zero while in the free state. +*/ spin_lock (&mdevice->lock);for (pfn = pfn_first; pfn < pfn_last; pfn++) {struct page *page =pfn_to_page (pfn);
Quoted 4 lines
lib/test_hmm.c static bool dmirror_allocate_chunk(struct dmirror_device *mdevice, · line 520
Quoted 1 line
lib/test_hmm.c +1
-0+set_page_count (page,0 );This confuses me. How does this and
init_page_count()not confuse the buddy allocator? Don't you have to reset the refcount somewhere after the test?
Device private struct pages are created with memmap_pages() and destroyed with memunmap_pages(). They are never put on the LRU and never freed to the page allocator. The refcount is set to zero by put_page() which triggers the call to pgmap->page_free() instead. So only the driver handles the free pages it creates.
Quoted 40 lines
Quoted 17 lines
lib/test_hmm.c +1
-1page->zone_device_data = mdevice->free_pages; mdevice->free_pages = page; }@@ -561,7 +566,7 @@ static struct page *dmirror_devmem_alloc_page(struct dmirror_device *mdevice) } dpage->zone_device_data = rpage; -get_page (dpage); +init_page_count (dpage);lock_page (dpage);return dpage;mm/internal.h +0
-0[snip]
Quoted 5 lines
mm/swap.c +0
-0mm/swap.c static void __put_compound_page(struct page *page) · line 123
Quoted 10 lines
mm/swap.c +1
-2@@ -116,12 +116,11 @@ static void __put_compound_page(struct page *page)void __put_page (struct page *page) {if (is_zone_device_page (page)) { -put_dev_pagemap (page->pgmap); -/* * The page belongs to the device that created pgmap. Do * not return it to page allocator. */ +free_zone_device_page (page);I really like this.
Ira