search for: get_dev_pagemap

...oing any DMA map that might rely on > > MEMORY_DEVICE_PCI_P2PDMA has to be done in the driver lock, which is > > a bit unfortunate. > > Wouldn't P2PDMA use page pins? Not needing to hold a lock over > ZONE_DEVICE page operations was one of the motivations for plumbing > get_dev_pagemap() with a percpu-ref. hmm_range_fault() doesn't use page pins at all, so if a ZONE_DEVICE page comes out of it then it needs to use another locking pattern. If I follow it all right: We can do a get_dev_pagemap inside the page_walk and touch the pgmap, or we can do the 'device mutex &...

...ong pfn, npages, i; bool fault, write_fault; uint64_t cpu_flags; @@ -490,17 +490,14 @@ static int hmm_vma_handle_pmd(struct mm_walk *walk, pfn = pmd_pfn(pmd) + pte_index(addr); for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) { if (pmd_devmap(pmd)) { - hmm_vma_walk->pgmap = get_dev_pagemap(pfn, - hmm_vma_walk->pgmap); - if (unlikely(!hmm_vma_walk->pgmap)) + pgmap = get_dev_pagemap(pfn, pgmap); + if (unlikely(!pgmap)) return -EBUSY; } pfns[i] = hmm_device_entry_from_pfn(range, pfn) | cpu_flags; } - if (hmm_vma_walk->pgmap) { - put_dev_pagemap(hmm...

...lt, write_fault; > uint64_t cpu_flags; > @@ -490,17 +490,14 @@ static int hmm_vma_handle_pmd(struct mm_walk *walk, > pfn = pmd_pfn(pmd) + pte_index(addr); > for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) { > if (pmd_devmap(pmd)) { > - hmm_vma_walk->pgmap = get_dev_pagemap(pfn, > - hmm_vma_walk->pgmap); > - if (unlikely(!hmm_vma_walk->pgmap)) > + pgmap = get_dev_pagemap(pfn, pgmap); > + if (unlikely(!pgmap)) > return -EBUSY; Unrelated to this patch, but what is the point of getting checking that the pgmap exists for the page...

...> nouveau_dmem_page(struct nouveau_drm *drm, struct page *page) > > { > > return is_device_private_page(page) && drm->dmem == page_to_dmem(page) > > > > > > Which does touch 'page->pgmap' > > > > Is this OK without having a get_dev_pagemap() ? > > > > Noting that the collision-retry scheme doesn't protect anything here > > as we can have a concurrent invalidation while doing the above deref. > > As long take_driver_page_table_lock() in Jerome's flow can replace > percpu_ref_tryget_live() on the pag...

...cking work out here? hmm_range_fault() runs with mmap_sem in read, and does not lock any of the page table levels. So it relies on accessing stale pte data being safe, and here we introduce for the first time a page pointer dereference and a pgmap dereference without any locking/refcounting. The get_dev_pagemap() worked on the PFN and obtained a refcount, so it created safety. Is there some tricky reason this is safe, eg a DEVICE_PRIVATE page cannot be removed from the vma without holding mmap_sem in write or something? Jason

...17 +490,14 @@ static int hmm_vma_handle_pmd(struct mm_walk *walk, > > pfn = pmd_pfn(pmd) + pte_index(addr); > > for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) { > > if (pmd_devmap(pmd)) { > > - hmm_vma_walk->pgmap = get_dev_pagemap(pfn, > > - hmm_vma_walk->pgmap); > > - if (unlikely(!hmm_vma_walk->pgmap)) > > + pgmap = get_dev_pagemap(pfn, pgmap); > > + if (unlikely(!pgmap)) > >...

On Fri, Aug 16, 2019 at 10:21:41AM -0700, Dan Williams wrote: > > We can do a get_dev_pagemap inside the page_walk and touch the pgmap, > > or we can do the 'device mutex && retry' pattern and touch the pgmap > > in the driver, under that lock. > > > > However in all cases the current get_dev_pagemap()'s in the page walk > > are not necessar...

...d does not lock any of > > the page table levels. > > > > So it relies on accessing stale pte data being safe, and here we > > introduce for the first time a page pointer dereference and a pgmap > > dereference without any locking/refcounting. > > > > The get_dev_pagemap() worked on the PFN and obtained a refcount, so it > > created safety. > > > > Is there some tricky reason this is safe, eg a DEVICE_PRIVATE page > > cannot be removed from the vma without holding mmap_sem in write or > > something? > > I don't think there...

...ch, but what is the point of getting checking > > that the pgmap exists for the page and then immediately releasing it? > > This code has this pattern in several places. > > > > It feels racy > > Agree, not sure what the intent is here. The only other reason call > get_dev_pagemap() is to just check in general if the pfn is indeed > owned by some ZONE_DEVICE instance, but if the intent is to make sure > the device is still attached/enabled that check is invalidated at > put_dev_pagemap(). > > If it's the former case, validating ZONE_DEVICE pfns, I imagine...

...pfn_to_page(range, range->pfns[i]); if (!nouveau_dmem_page(drm, page)) { nouveau_dmem_page(struct nouveau_drm *drm, struct page *page) { return is_device_private_page(page) && drm->dmem == page_to_dmem(page) Which does touch 'page->pgmap' Is this OK without having a get_dev_pagemap() ? Noting that the collision-retry scheme doesn't protect anything here as we can have a concurrent invalidation while doing the above deref. Jason

...ely on > > > MEMORY_DEVICE_PCI_P2PDMA has to be done in the driver lock, which is > > > a bit unfortunate. > > > > Wouldn't P2PDMA use page pins? Not needing to hold a lock over > > ZONE_DEVICE page operations was one of the motivations for plumbing > > get_dev_pagemap() with a percpu-ref. > > hmm_range_fault() doesn't use page pins at all, so if a ZONE_DEVICE > page comes out of it then it needs to use another locking pattern. > > If I follow it all right: > > We can do a get_dev_pagemap inside the page_walk and touch the pgmap, > or...

...we map two different devices (e.g with device mapper), will start & end pfn still point to same pgmap? Or there is something else which I am missing here. Note: I tested only EXT4. [1] - if (pgmap && pgmap->type == MEMORY_DEVICE_FS_DAX) + end_pgmap = get_dev_pagemap(pfn_t_to_pfn(end_pfn), NULL); + if (pgmap && pgmap == end_pgmap && pgmap->type == MEMORY_DEVICE_FS_DAX + && pfn_t_to_page(pfn)->pgmap == pgmap + && pfn_t_to_page(end_pfn)->pgmap == pgm...

...we map two different devices (e.g with device mapper), will start & end pfn still point to same pgmap? Or there is something else which I am missing here. Note: I tested only EXT4. [1] - if (pgmap && pgmap->type == MEMORY_DEVICE_FS_DAX) + end_pgmap = get_dev_pagemap(pfn_t_to_pfn(end_pfn), NULL); + if (pgmap && pgmap == end_pgmap && pgmap->type == MEMORY_DEVICE_FS_DAX + && pfn_t_to_page(pfn)->pgmap == pgmap + && pfn_t_to_page(end_pfn)->pgmap == pgm...

Hi Jérôme, Ben, Felix and Jason, below is a series against the hmm tree which cleans up various minor bits and allows HMM_MIRROR to be built on all architectures. Diffstat: 11 files changed, 94 insertions(+), 210 deletions(-) A git tree is also available at: git://git.infradead.org/users/hch/misc.git hmm-cleanups.2 Gitweb:

...ng checking > > > that the pgmap exists for the page and then immediately releasing it? > > > This code has this pattern in several places. > > > > > > It feels racy > > > > Agree, not sure what the intent is here. The only other reason call > > get_dev_pagemap() is to just check in general if the pfn is indeed > > owned by some ZONE_DEVICE instance, but if the intent is to make sure > > the device is still attached/enabled that check is invalidated at > > put_dev_pagemap(). > > > > If it's the former case, validating ZON...

...eau_drm *drm, struct page *page) > > > { > > > return is_device_private_page(page) && drm->dmem == page_to_dmem(page) > > > > > > > > > Which does touch 'page->pgmap' > > > > > > Is this OK without having a get_dev_pagemap() ? > > > > > > Noting that the collision-retry scheme doesn't protect anything here > > > as we can have a concurrent invalidation while doing the above deref. > > > > As long take_driver_page_table_lock() in Jerome's flow can replace > > percpu...

...lt() runs with mmap_sem in read, and does not lock any of > the page table levels. > > So it relies on accessing stale pte data being safe, and here we > introduce for the first time a page pointer dereference and a pgmap > dereference without any locking/refcounting. > > The get_dev_pagemap() worked on the PFN and obtained a refcount, so it > created safety. > > Is there some tricky reason this is safe, eg a DEVICE_PRIVATE page > cannot be removed from the vma without holding mmap_sem in write or > something? I don't think there is any specific protection. Let me...

...d to fault */ return hmm_vma_walk_hole_(addr, end, fault, write_fault, walk); @@ -690,16 +682,6 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, return r; } } - if (hmm_vma_walk->pgmap) { - /* - * We do put_dev_pagemap() here and not in hmm_vma_handle_pte() - * so that we can leverage get_dev_pagemap() optimization which - * will not re-take a reference on a pgmap if we already have - * one. - */ - put_dev_pagemap(hmm_vma_walk->pgmap); - hmm_vma_walk->pgmap = NULL; - } pte_unmap(ptep - 1); hmm_vma_walk->last = addr; @@ -751,10 +733,6 @@ static int hmm_vma_walk_pud(pud_t *...

...gmap->ops->page_free) { + error = dev_pagemap_enable(dev); + if (error) + return ERR_PTR(error); + } + align_start = res->start & ~(SECTION_SIZE - 1); align_size = ALIGN(res->start + resource_size(res), SECTION_SIZE) - align_start; @@ -316,28 +353,6 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn, EXPORT_SYMBOL_GPL(get_dev_pagemap); #ifdef CONFIG_DEV_PAGEMAP_OPS -DEFINE_STATIC_KEY_FALSE(devmap_managed_key); -EXPORT_SYMBOL(devmap_managed_key); -static atomic_t devmap_enable; - -/* - * Toggle the static key for ->page_free() callbacks when dev_pagemap - * pages go idl...

On Tue, Aug 13, 2019 at 06:36:33PM -0700, Dan Williams wrote: > Section alignment constraints somewhat save us here. The only example > I can think of a PMD not containing a uniform pgmap association for > each pte is the case when the pgmap overlaps normal dram, i.e. shares > the same 'struct memory_section' for a given span. Otherwise, distinct > pgmaps arrange to manage