| Message ID | 3d74bb439723c7e46cbe47d1711795308aee4ae3.1664171943.git-series.apopple@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix several device private page reference counting issues | expand |
On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote: > Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page > refcount") device private pages have no longer had an extra reference > count when the page is in use. However before handing them back to the > owning device driver we add an extra reference count such that free > pages have a reference count of one. > > This makes it difficult to tell if a page is free or not because both > free and in use pages will have a non-zero refcount. Instead we should > return pages to the drivers page allocator with a zero reference count. > Kernel code can then safely use kernel functions such as > get_page_unless_zero(). > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > --- > arch/powerpc/kvm/book3s_hv_uvmem.c | 1 + > drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 + > drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 + > lib/test_hmm.c | 1 + > mm/memremap.c | 5 ----- > mm/page_alloc.c | 6 ++++++ > 6 files changed, 10 insertions(+), 5 deletions(-) I think this is a great idea, but I'm surprised no dax stuff is touched here? Jason
Jason Gunthorpe <jgg@nvidia.com> writes: > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote: >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page >> refcount") device private pages have no longer had an extra reference >> count when the page is in use. However before handing them back to the >> owning device driver we add an extra reference count such that free >> pages have a reference count of one. >> >> This makes it difficult to tell if a page is free or not because both >> free and in use pages will have a non-zero refcount. Instead we should >> return pages to the drivers page allocator with a zero reference count. >> Kernel code can then safely use kernel functions such as >> get_page_unless_zero(). >> >> Signed-off-by: Alistair Popple <apopple@nvidia.com> >> --- >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 + >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 + >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 + >> lib/test_hmm.c | 1 + >> mm/memremap.c | 5 ----- >> mm/page_alloc.c | 6 ++++++ >> 6 files changed, 10 insertions(+), 5 deletions(-) > > I think this is a great idea, but I'm surprised no dax stuff is > touched here? free_zone_device_page() shouldn't be called for pgmap->type == MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX there. Except that the folio code looks like it might have introduced a bug. AFAICT put_page() always calls put_devmap_managed_page(&folio->page) but folio_put() does not (although folios_put() does!). So it seems folio_put() won't end up calling __put_devmap_managed_page_refs() as I think it should. I think you're right about the change to __init_zone_device_page() - I should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to look at Dan's patch series more closely as I suspect it might be better to rebase this patch on top of that. > Jason
Alistair Popple wrote: > > Jason Gunthorpe <jgg@nvidia.com> writes: > > > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote: > >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page > >> refcount") device private pages have no longer had an extra reference > >> count when the page is in use. However before handing them back to the > >> owning device driver we add an extra reference count such that free > >> pages have a reference count of one. > >> > >> This makes it difficult to tell if a page is free or not because both > >> free and in use pages will have a non-zero refcount. Instead we should > >> return pages to the drivers page allocator with a zero reference count. > >> Kernel code can then safely use kernel functions such as > >> get_page_unless_zero(). > >> > >> Signed-off-by: Alistair Popple <apopple@nvidia.com> > >> --- > >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 + > >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 + > >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 + > >> lib/test_hmm.c | 1 + > >> mm/memremap.c | 5 ----- > >> mm/page_alloc.c | 6 ++++++ > >> 6 files changed, 10 insertions(+), 5 deletions(-) > > > > I think this is a great idea, but I'm surprised no dax stuff is > > touched here? > > free_zone_device_page() shouldn't be called for pgmap->type == > MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX > there. Except that the folio code looks like it might have introduced a > bug. AFAICT put_page() always calls > put_devmap_managed_page(&folio->page) but folio_put() does not (although > folios_put() does!). So it seems folio_put() won't end up calling > __put_devmap_managed_page_refs() as I think it should. > > I think you're right about the change to __init_zone_device_page() - I > should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to > look at Dan's patch series more closely as I suspect it might be better > to rebase this patch on top of that. Apologies for the delay I was travelling the past few days. Yes, I think this patch slots in nicely to avoid the introduction of an init_mode [1]: https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/ Mind if I steal it into my series?
Dan Williams <dan.j.williams@intel.com> writes: > Alistair Popple wrote: >> >> Jason Gunthorpe <jgg@nvidia.com> writes: >> >> > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote: >> >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page >> >> refcount") device private pages have no longer had an extra reference >> >> count when the page is in use. However before handing them back to the >> >> owning device driver we add an extra reference count such that free >> >> pages have a reference count of one. >> >> >> >> This makes it difficult to tell if a page is free or not because both >> >> free and in use pages will have a non-zero refcount. Instead we should >> >> return pages to the drivers page allocator with a zero reference count. >> >> Kernel code can then safely use kernel functions such as >> >> get_page_unless_zero(). >> >> >> >> Signed-off-by: Alistair Popple <apopple@nvidia.com> >> >> --- >> >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 + >> >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 + >> >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 + >> >> lib/test_hmm.c | 1 + >> >> mm/memremap.c | 5 ----- >> >> mm/page_alloc.c | 6 ++++++ >> >> 6 files changed, 10 insertions(+), 5 deletions(-) >> > >> > I think this is a great idea, but I'm surprised no dax stuff is >> > touched here? >> >> free_zone_device_page() shouldn't be called for pgmap->type == >> MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX >> there. Except that the folio code looks like it might have introduced a >> bug. AFAICT put_page() always calls >> put_devmap_managed_page(&folio->page) but folio_put() does not (although >> folios_put() does!). So it seems folio_put() won't end up calling >> __put_devmap_managed_page_refs() as I think it should. >> >> I think you're right about the change to __init_zone_device_page() - I >> should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to >> look at Dan's patch series more closely as I suspect it might be better >> to rebase this patch on top of that. > > Apologies for the delay I was travelling the past few days. Yes, I think > this patch slots in nicely to avoid the introduction of an init_mode > [1]: > > https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/ > > Mind if I steal it into my series? No problem, although I notice Andrew has already merged it into mm-unstable. If you end up rebasing your series on top of mine I think all that's needed is a patch somewhere in your series to drop the various `if (pgmap->type == MEMORY_DEVICE_*)` I added to (hopefully) avoid breaking DAX. Assuming DAX takes a pagemap reference on struct page allocation something like below. --- diff --git a/mm/memremap.c b/mm/memremap.c index 421bec3a29ee..da1a0e0abb8b 100644 --- a/mm/memremap.c +++ b/mm/memremap.c @@ -507,15 +507,7 @@ void free_zone_device_page(struct page *page) page->mapping = NULL; page->pgmap->ops->page_free(page); - if (page->pgmap->type != MEMORY_DEVICE_PRIVATE && - page->pgmap->type != MEMORY_DEVICE_COHERENT) - /* - * Reset the page count to 1 to prepare for handing out the page - * again. - */ - set_page_count(page, 1); - else - put_dev_pagemap(page->pgmap); + put_dev_pagemap(page->pgmap); } void zone_device_page_init(struct page *page) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 014dbdf54d62..3e5ff06700ca 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6816,9 +6816,7 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn, * ZONE_DEVICE pages are released directly to the driver page allocator * which will set the page count to 1 when allocating the page. */ - if (pgmap->type == MEMORY_DEVICE_PRIVATE || - pgmap->type == MEMORY_DEVICE_COHERENT) - set_page_count(page, 0); + set_page_count(page, 0); } /*
Alistair Popple wrote: > > Dan Williams <dan.j.williams@intel.com> writes: > > > Alistair Popple wrote: > >> > >> Jason Gunthorpe <jgg@nvidia.com> writes: > >> > >> > On Mon, Sep 26, 2022 at 04:03:06PM +1000, Alistair Popple wrote: > >> >> Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page > >> >> refcount") device private pages have no longer had an extra reference > >> >> count when the page is in use. However before handing them back to the > >> >> owning device driver we add an extra reference count such that free > >> >> pages have a reference count of one. > >> >> > >> >> This makes it difficult to tell if a page is free or not because both > >> >> free and in use pages will have a non-zero refcount. Instead we should > >> >> return pages to the drivers page allocator with a zero reference count. > >> >> Kernel code can then safely use kernel functions such as > >> >> get_page_unless_zero(). > >> >> > >> >> Signed-off-by: Alistair Popple <apopple@nvidia.com> > >> >> --- > >> >> arch/powerpc/kvm/book3s_hv_uvmem.c | 1 + > >> >> drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 + > >> >> drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 + > >> >> lib/test_hmm.c | 1 + > >> >> mm/memremap.c | 5 ----- > >> >> mm/page_alloc.c | 6 ++++++ > >> >> 6 files changed, 10 insertions(+), 5 deletions(-) > >> > > >> > I think this is a great idea, but I'm surprised no dax stuff is > >> > touched here? > >> > >> free_zone_device_page() shouldn't be called for pgmap->type == > >> MEMORY_DEVICE_FS_DAX so I don't think we should have to worry about DAX > >> there. Except that the folio code looks like it might have introduced a > >> bug. AFAICT put_page() always calls > >> put_devmap_managed_page(&folio->page) but folio_put() does not (although > >> folios_put() does!). So it seems folio_put() won't end up calling > >> __put_devmap_managed_page_refs() as I think it should. > >> > >> I think you're right about the change to __init_zone_device_page() - I > >> should limit it to DEVICE_PRIVATE/COHERENT pages only. But I need to > >> look at Dan's patch series more closely as I suspect it might be better > >> to rebase this patch on top of that. > > > > Apologies for the delay I was travelling the past few days. Yes, I think > > this patch slots in nicely to avoid the introduction of an init_mode > > [1]: > > > > https://lore.kernel.org/nvdimm/166329940343.2786261.6047770378829215962.stgit@dwillia2-xfh.jf.intel.com/ > > > > Mind if I steal it into my series? > > No problem, although I notice Andrew has already merged it into > mm-unstable. If you end up rebasing your series on top of mine I think > all that's needed is a patch somewhere in your series to drop the > various `if (pgmap->type == MEMORY_DEVICE_*)` I added to (hopefully) > avoid breaking DAX. Assuming DAX takes a pagemap reference on struct > page allocation something like below. Yeah, I'll go that route and rebase on top of -mm. Thanks again.
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c index d4eacf4..08d2f7d 100644 --- a/arch/powerpc/kvm/book3s_hv_uvmem.c +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c @@ -718,6 +718,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; + set_page_count(dpage, 1); lock_page(dpage); return dpage; out_clear: diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c index 776448b..05c2f4d 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c @@ -223,6 +223,7 @@ svm_migrate_get_vram_page(struct svm_range *prange, unsigned long pfn) page = pfn_to_page(pfn); svm_range_bo_ref(prange->svm_bo); page->zone_device_data = prange->svm_bo; + set_page_count(page, 1); lock_page(page); } diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c index 1635661..f9234ed 100644 --- a/drivers/gpu/drm/nouveau/nouveau_dmem.c +++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c @@ -326,6 +326,7 @@ nouveau_dmem_page_alloc_locked(struct nouveau_drm *drm) return NULL; } + set_page_count(page, 1); lock_page(page); return page; } diff --git a/lib/test_hmm.c b/lib/test_hmm.c index 89463ff..2bd3a67 100644 --- a/lib/test_hmm.c +++ b/lib/test_hmm.c @@ -627,6 +627,7 @@ static struct page *dmirror_devmem_alloc_page(struct dmirror_device *mdevice) goto error; } + set_page_count(dpage, 1); dpage->zone_device_data = rpage; lock_page(dpage); return dpage; diff --git a/mm/memremap.c b/mm/memremap.c index 25029a4..e065171 100644 --- a/mm/memremap.c +++ b/mm/memremap.c @@ -501,11 +501,6 @@ void free_zone_device_page(struct page *page) */ page->mapping = NULL; page->pgmap->ops->page_free(page); - - /* - * Reset the page count to 1 to prepare for handing out the page again. - */ - set_page_count(page, 1); } #ifdef CONFIG_FS_DAX diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 9d49803..67eaab5 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6744,6 +6744,12 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn, set_pageblock_migratetype(page, MIGRATE_MOVABLE); cond_resched(); } + + /* + * ZONE_DEVICE pages are released directly to the driver page allocator + * which will set the page count to 1 when allocating the page. + */ + set_page_count(page, 0); } /*
Since 27674ef6c73f ("mm: remove the extra ZONE_DEVICE struct page refcount") device private pages have no longer had an extra reference count when the page is in use. However before handing them back to the owning device driver we add an extra reference count such that free pages have a reference count of one. This makes it difficult to tell if a page is free or not because both free and in use pages will have a non-zero refcount. Instead we should return pages to the drivers page allocator with a zero reference count. Kernel code can then safely use kernel functions such as get_page_unless_zero(). Signed-off-by: Alistair Popple <apopple@nvidia.com> --- arch/powerpc/kvm/book3s_hv_uvmem.c | 1 + drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 1 + drivers/gpu/drm/nouveau/nouveau_dmem.c | 1 + lib/test_hmm.c | 1 + mm/memremap.c | 5 ----- mm/page_alloc.c | 6 ++++++ 6 files changed, 10 insertions(+), 5 deletions(-)