Re: [PATCH 02/13] pci/p2pdma: Don't initialise page refcount to one
From: Christoph Hellwig <hch@lst.de>
Date: 2024-06-27 05:31:07
Also in:
linux-arm-kernel, linux-cxl, linux-doc, linux-ext4, linux-fsdevel, linux-mm, linux-xfs, lkml, nvdimm
On Thu, Jun 27, 2024 at 10:54:17AM +1000, Alistair Popple wrote:
quoted hunk ↗ jump to hunk
The reference counts for ZONE_DEVICE private pages should be initialised by the driver when the page is actually allocated by the driver allocator, not when they are first created. This is currently the case for MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_COHERENT pages but not MEMORY_DEVICE_PCI_P2PDMA pages so fix that up. Signed-off-by: Alistair Popple <apopple@nvidia.com> --- drivers/pci/p2pdma.c | 2 ++ mm/memremap.c | 8 ++++---- mm/mm_init.c | 4 +++- 3 files changed, 9 insertions(+), 5 deletions(-)diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index 4f47a13..1e9ea32 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c@@ -128,6 +128,8 @@ static int p2pmem_alloc_mmap(struct file *filp, struct kobject *kobj, goto out; } + set_page_count(virt_to_page(kaddr), 1);
Can we have a comment here? Without that it feels a bit too much like black magic when reading the code.
+ if (folio->page.pgmap->type == MEMORY_DEVICE_PRIVATE || + folio->page.pgmap->type == MEMORY_DEVICE_COHERENT) + put_dev_pagemap(folio->page.pgmap); + else if (folio->page.pgmap->type != MEMORY_DEVICE_PCI_P2PDMA) /* * Reset the refcount to 1 to prepare for handing out the page * again. */ folio_set_count(folio, 1);
Where the else if evaluates to MEMORY_DEVICE_FS_DAX || MEMORY_DEVICE_GENERIC. Maybe make this a switch statement handling all cases of the enum to make it clear and have the compiler generate a warning when a new type is added without being handled here?
quoted hunk ↗ jump to hunk
@@ -1014,7 +1015,8 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn, * which will set the page count to 1 when allocating the page. */ if (pgmap->type == MEMORY_DEVICE_PRIVATE || + pgmap->type == MEMORY_DEVICE_COHERENT || + pgmap->type == MEMORY_DEVICE_PCI_P2PDMA) set_page_count(page, 0);
Similarly here a switch with explanation of what will be handled and what not would be nice.