lkml.org 
[lkml]   [2020]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 3/6] mm: support THP migration to device private memory
    [adding a few of the usual suspects]

    On Wed, Nov 11, 2020 at 03:38:42PM -0800, Ralph Campbell wrote:
    > There are 4 types of ZONE_DEVICE struct pages:
    > MEMORY_DEVICE_PRIVATE, MEMORY_DEVICE_FS_DAX, MEMORY_DEVICE_GENERIC, and
    > MEMORY_DEVICE_PCI_P2PDMA.
    >
    > Currently, memremap_pages() allocates struct pages for a physical address range
    > with a page_ref_count(page) of one and increments the pgmap->ref per CPU
    > reference count by the number of pages created since each ZONE_DEVICE struct
    > page has a pointer to the pgmap.
    >
    > The struct pages are not freed until memunmap_pages() is called which
    > calls put_page() which calls put_dev_pagemap() which releases a reference to
    > pgmap->ref. memunmap_pages() blocks waiting for pgmap->ref reference count
    > to be zero. As far as I can tell, the put_page() in memunmap_pages() has to
    > be the *last* put_page() (see MEMORY_DEVICE_PCI_P2PDMA).
    > My RFC [1] breaks this put_page() -> put_dev_pagemap() connection so that
    > the struct page reference count can go to zero and back to non-zero without
    > changing the pgmap->ref reference count.
    >
    > Q1: Is that safe? Is there some code that depends on put_page() dropping
    > the pgmap->ref reference count as part of memunmap_pages()?
    > My testing of [1] seems OK but I'm sure there are lots of cases I didn't test.

    It should be safe, but the audit you've done is important to make sure
    we do not miss anything important.

    > 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.

    > pci_p2pmem_virt_to_bus() will return the physical address and I guess
    > pfn_to_page(physaddr >> PAGE_SHIFT) could return the struct page.
    >
    > Since there is a clear allocation/free, pci_alloc_p2pmem() can probably be
    > modified to increment/decrement the MEMORY_DEVICE_PCI_P2PDMA struct page
    > reference count. Or maybe just leave it at one like it is now.

    And yes, doing that is probably a sensible safe guard.

    > MEMORY_DEVICE_FS_DAX:
    > Struct pages are created in pmem_attach_disk() and virtio_fs_setup_dax() with
    > an initial reference count of one.
    > The problem I see is that there are 3 states that are important:
    > a) memory is free and not allocated to any file (page_ref_count() == 0).
    > b) memory is allocated to a file and in the page cache (page_ref_count() == 1).
    > c) some gup() or I/O has a reference even after calling unmap_mapping_pages()
    > (page_ref_count() > 1). ext4_break_layouts() basically waits until the
    > page_ref_count() == 1 with put_page() calling wake_up_var(&page->_refcount)
    > to wake up ext4_break_layouts().
    > The current code doesn't seem to distinguish (a) and (b). If we want to use
    > the 0->1 reference count to signal (c), then the page cache would have hold
    > entries with a page_ref_count() == 0 which doesn't match the general page cache

    I think the sensible model here is to grab a reference when it is
    added to the page cache. That is exactly how normal system memory pages
    work.

    \
     
     \ /
      Last update: 2020-12-02 11:17    [W:3.146 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site