Re: [Nouveau] [PATCH 09/27] mm: generalize the pgmap based page_free infrastructure
On 2022-02-10 12:28 a.m., Christoph Hellwig wrote:
> Key off on the existence of ->page_free to prepare for adding support for
> more pgmap types that are device managed and thus need the free callback.
>
> Signed-off-by: Christoph Hellwig
Great! This makes my patch simpler.
Reviewed-by: Logan Gunthorpe
> ---
> mm/memremap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memremap.c b/mm/memremap.c
> index fef5734d5e4933..e00ffcdba7b632 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -452,7 +452,7 @@ EXPORT_SYMBOL_GPL(get_dev_pagemap);
>
> void free_zone_device_page(struct page *page)
> {
> - if (WARN_ON_ONCE(!is_device_private_page(page)))
> + if (WARN_ON_ONCE(!page->pgmap->ops || !page->pgmap->ops->page_free))
> return;
>
> __ClearPageWaiters(page);
> @@ -460,7 +460,7 @@ void free_zone_device_page(struct page *page)
> mem_cgroup_uncharge(page_folio(page));
>
> /*
> - * When a device_private page is freed, the page->mapping field
> + * When a device managed page is freed, the page->mapping field
>* may still contain a (stale) mapping value. For example, the
>* lower bits of page->mapping may still identify the page as an
>* anonymous page. Ultimately, this entire field is just stale
>
Re: [Nouveau] start sorting out the ZONE_DEVICE refcount mess
On 2022-02-06 11:32 p.m., Christoph Hellwig wrote: > Hi all, > > this series removes the offset by one refcount for ZONE_DEVICE pages > that are freed back to the driver owning them, which is just device > private ones for now, but also the planned device coherent pages > and the ehanced p2p ones pending. > > It does not address the fsdax pages yet, which will be attacked in a > follow on series. > > Diffstat: > arch/arm64/mm/mmu.c |1 > arch/powerpc/kvm/book3s_hv_uvmem.c |1 > drivers/gpu/drm/amd/amdkfd/kfd_migrate.c |2 > drivers/gpu/drm/amd/amdkfd/kfd_priv.h|1 > drivers/gpu/drm/drm_cache.c |2 > drivers/gpu/drm/nouveau/nouveau_dmem.c |3 - > drivers/gpu/drm/nouveau/nouveau_svm.c|1 > drivers/infiniband/core/rw.c |1 > drivers/nvdimm/pmem.h|1 > drivers/nvme/host/pci.c |1 > drivers/nvme/target/io-cmd-bdev.c|1 > fs/Kconfig |2 > fs/fuse/virtio_fs.c |1 > include/linux/hmm.h |9 > include/linux/memremap.h | 22 +- > include/linux/mm.h | 59 - > lib/test_hmm.c |4 + > mm/Kconfig |4 - > mm/internal.h|2 > mm/memcontrol.c | 11 + > mm/memremap.c| 63 > --- > mm/migrate.c |6 -- > mm/swap.c| 49 ++-- > 23 files changed, 90 insertions(+), 157 deletions(-) Looks good to me. I was wondering about the location of some of this code, so it's nice to see it cleaned up. Except for the one minor issue I noted on patch 6, it all looks good to me. I've reviewed all the patches and tested the series under my p2pdma series. Reviewed-by: Logan Gunthorpe Logan
Re: [Nouveau] [PATCH 6/8] mm: don't include in
On 2022-02-06 11:32 p.m., Christoph Hellwig wrote: > Move the check for the actual pgmap types that need the free at refcount > one behavior into the out of line helper, and thus avoid the need to > pull memremap.h into mm.h. > > Signed-off-by: Christoph Hellwig I've noticed mm/memcontrol.c uses is_device_private_page() and also needs a memremap.h include added to compile with my configuration. Logan
Re: [Nouveau] [PATCH v3 3/6] mm: support THP migration to device private memory
On 2020-12-02 3:14 a.m., Christoph Hellwig wrote:>> MEMORY_DEVICE_PCI_P2PDMA: >> Struct pages are created in pci_p2pdma_add_resource() and represent device >> memory accessible by PCIe bar address space. Memory is allocated with >> pci_alloc_p2pmem() based on a byte length but the gen_pool_alloc_owner() >> call will allocate memory in a minimum of PAGE_SIZE units. >> Reference counting is +1 per *allocation* on the pgmap->ref reference count. >> Note that this is not +1 per page which is what put_page() expects. So >> currently, a get_page()/put_page() works OK because the page reference count >> only goes 1->2 and 2->1. If it went to zero, the pgmap->ref reference count >> would be incorrect if the allocation size was greater than one page. >> >> I see pci_alloc_p2pmem() is called by nvme_alloc_sq_cmds() and >> pci_p2pmem_alloc_sgl() to create a command queue and a struct scatterlist *. >> Looks like sg_page(sg) returns the ZONE_DEVICE struct page of the >> scatterlist. >> There are a huge number of places sg_page() is called so it is hard to tell >> whether or not get_page()/put_page() is ever called on >> MEMORY_DEVICE_PCI_P2PDMA >> pages. > > Nothing should call get_page/put_page on them, as they are not treated > as refcountable memory. More importantly nothing is allowed to keep > a reference longer than the time of the I/O. Yes, right now this is safe, as Christoph notes there are no places where these should be got/put. But eventually we'll need to change how pci_alloc_p2pmem() works to take references on the actual pages and allow freeing individual pages, similar to what you suggest. This is one of the issues Jason pointed out in my last RFC to try to pass these pages through GUP. Logan ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 17/25] PCI/P2PDMA: use the dev_pagemap internal refcount
On 2019-06-26 6:27 a.m., Christoph Hellwig wrote:
> The functionality is identical to the one currently open coded in
> p2pdma.c.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Logan Gunthorpe
Also, for the P2PDMA changes in this series:
Tested-by: Logan Gunthorpe
I've ran this series through my simple P2PDMA tests.
Logan
> ---
> drivers/pci/p2pdma.c | 57
> 1 file changed, 4 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index ebd8ce3bba2e..608f84df604a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -24,12 +24,6 @@ struct pci_p2pdma {
> bool p2pmem_published;
> };
>
> -struct p2pdma_pagemap {
> - struct dev_pagemap pgmap;
> - struct percpu_ref ref;
> - struct completion ref_done;
> -};
> -
> static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>char *buf)
> {
> @@ -78,32 +72,6 @@ static const struct attribute_group p2pmem_group = {
> .name = "p2pmem",
> };
>
> -static struct p2pdma_pagemap *to_p2p_pgmap(struct percpu_ref *ref)
> -{
> - return container_of(ref, struct p2pdma_pagemap, ref);
> -}
> -
> -static void pci_p2pdma_percpu_release(struct percpu_ref *ref)
> -{
> - struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
> -
> - complete(_pgmap->ref_done);
> -}
> -
> -static void pci_p2pdma_percpu_kill(struct dev_pagemap *pgmap)
> -{
> - percpu_ref_kill(pgmap->ref);
> -}
> -
> -static void pci_p2pdma_percpu_cleanup(struct dev_pagemap *pgmap)
> -{
> - struct p2pdma_pagemap *p2p_pgmap =
> - container_of(pgmap, struct p2pdma_pagemap, pgmap);
> -
> - wait_for_completion(_pgmap->ref_done);
> - percpu_ref_exit(_pgmap->ref);
> -}
> -
> static void pci_p2pdma_release(void *data)
> {
> struct pci_dev *pdev = data;
> @@ -153,11 +121,6 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
> return error;
> }
>
> -static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
> - .kill = pci_p2pdma_percpu_kill,
> - .cleanup= pci_p2pdma_percpu_cleanup,
> -};
> -
> /**
> * pci_p2pdma_add_resource - add memory for use as p2p memory
> * @pdev: the device to add the memory to
> @@ -171,7 +134,6 @@ static const struct dev_pagemap_ops
> pci_p2pdma_pagemap_ops = {
> int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
> u64 offset)
> {
> - struct p2pdma_pagemap *p2p_pgmap;
> struct dev_pagemap *pgmap;
> void *addr;
> int error;
> @@ -194,26 +156,15 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> return error;
> }
>
> - p2p_pgmap = devm_kzalloc(>dev, sizeof(*p2p_pgmap), GFP_KERNEL);
> - if (!p2p_pgmap)
> + pgmap = devm_kzalloc(>dev, sizeof(*pgmap), GFP_KERNEL);
> + if (!pgmap)
> return -ENOMEM;
> -
> - init_completion(_pgmap->ref_done);
> - error = percpu_ref_init(_pgmap->ref,
> - pci_p2pdma_percpu_release, 0, GFP_KERNEL);
> - if (error)
> - goto pgmap_free;
> -
> - pgmap = _pgmap->pgmap;
> -
> pgmap->res.start = pci_resource_start(pdev, bar) + offset;
> pgmap->res.end = pgmap->res.start + size - 1;
> pgmap->res.flags = pci_resource_flags(pdev, bar);
> - pgmap->ref = _pgmap->ref;
> pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
> pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
> pci_resource_start(pdev, bar);
> - pgmap->ops = _p2pdma_pagemap_ops;
>
> addr = devm_memremap_pages(>dev, pgmap);
> if (IS_ERR(addr)) {
> @@ -224,7 +175,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> error = gen_pool_add_owner(pdev->p2pdma->pool, (unsigned long)addr,
> pci_bus_address(pdev, bar) + offset,
> resource_size(>res), dev_to_node(>dev),
> - _pgmap->ref);
> + pgmap->ref);
> if (error)
> goto pages_free;
>
> @@ -236,7 +187,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> pages_free:
> devm_memunmap_pages(>dev, pgmap);
> pgmap_free:
> - devm_kfree(>dev, p2p_pgmap);
> + devm_kfree(>dev, pgmap);
> return error;
> }
> EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
>
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 08/25] memremap: move dev_pagemap callbacks into a separate structure
On 2019-06-17 6:27 a.m., Christoph Hellwig wrote:
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index a98126ad9c3a..e083567d26ef 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -100,7 +100,7 @@ static void pci_p2pdma_percpu_cleanup(struct percpu_ref
> *ref)
> struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
>
> wait_for_completion(_pgmap->ref_done);
> - percpu_ref_exit(_pgmap->ref);
> + percpu_ref_exit(ref);
> }
>
> static void pci_p2pdma_release(void *data)
> @@ -152,6 +152,11 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
> return error;
> }
>
> +static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
> + .kill = pci_p2pdma_percpu_kill,
> + .cleanup= pci_p2pdma_percpu_cleanup,
> +};
> +
> /**
> * pci_p2pdma_add_resource - add memory for use as p2p memory
> * @pdev: the device to add the memory to
> @@ -207,8 +212,6 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
> pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
> pci_resource_start(pdev, bar);
> - pgmap->kill = pci_p2pdma_percpu_kill;
> - pgmap->cleanup = pci_p2pdma_percpu_cleanup;
I just noticed this is missing a line to set pgmap->ops to
pci_p2pdma_pagemap_ops. I must have gotten confused by the other users
in my original review. Though I'm not sure how this compiles as the new
struct is static and unused. However, it is rendered moot in Patch 16
when this is all removed.
Logan
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [PATCH 16/25] PCI/P2PDMA: use the dev_pagemap internal refcount
On 2019-06-17 6:27 a.m., Christoph Hellwig wrote:
> The functionality is identical to the one currently open coded in
> p2pdma.c.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Logan Gunthorpe
I also did a quick test with the full patch-set to ensure that the setup
and tear down paths for p2pdma still work correctly and it all does.
Thanks,
Logan
> ---
> drivers/pci/p2pdma.c | 56
> 1 file changed, 4 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 48a88158e46a..608f84df604a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -24,12 +24,6 @@ struct pci_p2pdma {
> bool p2pmem_published;
> };
>
> -struct p2pdma_pagemap {
> - struct dev_pagemap pgmap;
> - struct percpu_ref ref;
> - struct completion ref_done;
> -};
> -
> static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>char *buf)
> {
> @@ -78,32 +72,6 @@ static const struct attribute_group p2pmem_group = {
> .name = "p2pmem",
> };
>
> -static struct p2pdma_pagemap *to_p2p_pgmap(struct percpu_ref *ref)
> -{
> - return container_of(ref, struct p2pdma_pagemap, ref);
> -}
> -
> -static void pci_p2pdma_percpu_release(struct percpu_ref *ref)
> -{
> - struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
> -
> - complete(_pgmap->ref_done);
> -}
> -
> -static void pci_p2pdma_percpu_kill(struct dev_pagemap *pgmap)
> -{
> - percpu_ref_kill(pgmap->ref);
> -}
> -
> -static void pci_p2pdma_percpu_cleanup(struct dev_pagemap *pgmap)
> -{
> - struct p2pdma_pagemap *p2p_pgmap =
> - container_of(pgmap, struct p2pdma_pagemap, pgmap);
> -
> - wait_for_completion(_pgmap->ref_done);
> - percpu_ref_exit(_pgmap->ref);
> -}
> -
> static void pci_p2pdma_release(void *data)
> {
> struct pci_dev *pdev = data;
> @@ -153,11 +121,6 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
> return error;
> }
>
> -static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
> - .kill = pci_p2pdma_percpu_kill,
> - .cleanup= pci_p2pdma_percpu_cleanup,
> -};
> -
> /**
> * pci_p2pdma_add_resource - add memory for use as p2p memory
> * @pdev: the device to add the memory to
> @@ -171,7 +134,6 @@ static const struct dev_pagemap_ops
> pci_p2pdma_pagemap_ops = {
> int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
> u64 offset)
> {
> - struct p2pdma_pagemap *p2p_pgmap;
> struct dev_pagemap *pgmap;
> void *addr;
> int error;
> @@ -194,22 +156,12 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> return error;
> }
>
> - p2p_pgmap = devm_kzalloc(>dev, sizeof(*p2p_pgmap), GFP_KERNEL);
> - if (!p2p_pgmap)
> + pgmap = devm_kzalloc(>dev, sizeof(*pgmap), GFP_KERNEL);
> + if (!pgmap)
> return -ENOMEM;
> -
> - init_completion(_pgmap->ref_done);
> - error = percpu_ref_init(_pgmap->ref,
> - pci_p2pdma_percpu_release, 0, GFP_KERNEL);
> - if (error)
> - goto pgmap_free;
> -
> - pgmap = _pgmap->pgmap;
> -
> pgmap->res.start = pci_resource_start(pdev, bar) + offset;
> pgmap->res.end = pgmap->res.start + size - 1;
> pgmap->res.flags = pci_resource_flags(pdev, bar);
> - pgmap->ref = _pgmap->ref;
> pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
> pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
> pci_resource_start(pdev, bar);
> @@ -223,7 +175,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> error = gen_pool_add_owner(pdev->p2pdma->pool, (unsigned long)addr,
> pci_bus_address(pdev, bar) + offset,
> resource_size(>res), dev_to_node(>dev),
> - _pgmap->ref);
> + pgmap->ref);
> if (error)
> goto pages_free;
>
> @@ -235,7 +187,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int
> bar, size_t size,
> pages_free:
> devm_memunmap_pages(>dev, pgmap);
> pgmap_free:
> - devm_kfree(>dev, p2p_pgmap);
> + devm_kfree(>dev, pgmap);
> return error;
> }
> EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
>
Re: dev_pagemap related cleanups
On 2019-06-13 2:21 p.m., Dan Williams wrote: > On Thu, Jun 13, 2019 at 1:18 PM Logan Gunthorpe wrote: >> >> >> >> On 2019-06-13 12:27 p.m., Dan Williams wrote: >>> On Thu, Jun 13, 2019 at 2:43 AM Christoph Hellwig wrote: >>>> >>>> Hi Dan, Jérôme and Jason, >>>> >>>> below is a series that cleans up the dev_pagemap interface so that >>>> it is more easily usable, which removes the need to wrap it in hmm >>>> and thus allowing to kill a lot of code >>>> >>>> Diffstat: >>>> >>>> 22 files changed, 245 insertions(+), 802 deletions(-) >>> >>> Hooray! >>> >>>> Git tree: >>>> >>>> git://git.infradead.org/users/hch/misc.git hmm-devmem-cleanup >>> >>> I just realized this collides with the dev_pagemap release rework in >>> Andrew's tree (commit ids below are from next.git and are not stable) >>> >>> 4422ee8476f0 mm/devm_memremap_pages: fix final page put race >>> 771f0714d0dc PCI/P2PDMA: track pgmap references per resource, not globally >>> af37085de906 lib/genalloc: introduce chunk owners >>> e0047ff8aa77 PCI/P2PDMA: fix the gen_pool_add_virt() failure path >>> 0315d47d6ae9 mm/devm_memremap_pages: introduce devm_memunmap_pages >>> 216475c7eaa8 drivers/base/devres: introduce devm_release_action() >>> >>> CONFLICT (content): Merge conflict in tools/testing/nvdimm/test/iomap.c >>> CONFLICT (content): Merge conflict in mm/hmm.c >>> CONFLICT (content): Merge conflict in kernel/memremap.c >>> CONFLICT (content): Merge conflict in include/linux/memremap.h >>> CONFLICT (content): Merge conflict in drivers/pci/p2pdma.c >>> CONFLICT (content): Merge conflict in drivers/nvdimm/pmem.c >>> CONFLICT (content): Merge conflict in drivers/dax/device.c >>> CONFLICT (content): Merge conflict in drivers/dax/dax-private.h >>> >>> Perhaps we should pull those out and resend them through hmm.git? >> >> Hmm, I've been waiting for those patches to get in for a little while now ;( > > Unless Andrew was going to submit as v5.2-rc fixes I think I should > rebase / submit them on current hmm.git and then throw these cleanups > from Christoph on top? Whatever you feel is best. I'm just hoping they get in sooner rather than later. They do fix a bug after all. Let me know if you want me to retest the P2PDMA stuff after the rebase. Thanks, Logan
Re: dev_pagemap related cleanups
On 2019-06-13 12:27 p.m., Dan Williams wrote: > On Thu, Jun 13, 2019 at 2:43 AM Christoph Hellwig wrote: >> >> Hi Dan, Jérôme and Jason, >> >> below is a series that cleans up the dev_pagemap interface so that >> it is more easily usable, which removes the need to wrap it in hmm >> and thus allowing to kill a lot of code >> >> Diffstat: >> >> 22 files changed, 245 insertions(+), 802 deletions(-) > > Hooray! > >> Git tree: >> >> git://git.infradead.org/users/hch/misc.git hmm-devmem-cleanup > > I just realized this collides with the dev_pagemap release rework in > Andrew's tree (commit ids below are from next.git and are not stable) > > 4422ee8476f0 mm/devm_memremap_pages: fix final page put race > 771f0714d0dc PCI/P2PDMA: track pgmap references per resource, not globally > af37085de906 lib/genalloc: introduce chunk owners > e0047ff8aa77 PCI/P2PDMA: fix the gen_pool_add_virt() failure path > 0315d47d6ae9 mm/devm_memremap_pages: introduce devm_memunmap_pages > 216475c7eaa8 drivers/base/devres: introduce devm_release_action() > > CONFLICT (content): Merge conflict in tools/testing/nvdimm/test/iomap.c > CONFLICT (content): Merge conflict in mm/hmm.c > CONFLICT (content): Merge conflict in kernel/memremap.c > CONFLICT (content): Merge conflict in include/linux/memremap.h > CONFLICT (content): Merge conflict in drivers/pci/p2pdma.c > CONFLICT (content): Merge conflict in drivers/nvdimm/pmem.c > CONFLICT (content): Merge conflict in drivers/dax/device.c > CONFLICT (content): Merge conflict in drivers/dax/dax-private.h > > Perhaps we should pull those out and resend them through hmm.git? Hmm, I've been waiting for those patches to get in for a little while now ;( Logan
Re: [PATCH 07/22] memremap: move dev_pagemap callbacks into a separate structure
On 2019-06-13 3:43 a.m., Christoph Hellwig wrote: > The dev_pagemap is a growing too many callbacks. Move them into a > separate ops structure so that they are not duplicated for multiple > instances, and an attacker can't easily overwrite them. > > Signed-off-by: Christoph Hellwig > --- > drivers/dax/device.c | 6 +- > drivers/nvdimm/pmem.c | 18 +- > drivers/pci/p2pdma.c | 5 - > include/linux/memremap.h | 29 +++-- > kernel/memremap.c | 12 ++-- > mm/hmm.c | 8 ++-- > tools/testing/nvdimm/test/iomap.c | 2 +- > 7 files changed, 50 insertions(+), 30 deletions(-) Looks good to me, Reviewed-by: Logan Gunthorpe Logan
Re: [PATCH 08/22] memremap: pass a struct dev_pagemap to ->kill
On 2019-06-13 3:43 a.m., Christoph Hellwig wrote: > Passing the actual typed structure leads to more understandable code > vs the actual references. Ha, ok, I originally suggested this to Dan when he introduced the callback[1]. Reviewed-by: Logan Gunthorpe Logan [1] https://lore.kernel.org/lkml/8f0cae82-130f-8a64-cfbd-fda5fd76b...@deltatee.com/T/#u
