lkml.org 
[lkml]   [2019]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Subject[PATCH v3] mm: Cleanup __put_devmap_managed_page() vs ->page_free()
    From
    Date
    After the removal of the device-public infrastructure there are only 2
    ->page_free() call backs in the kernel. One of those is a device-private
    callback in the nouveau driver, the other is a generic wakeup needed in
    the DAX case. In the hopes that all ->page_free() callbacks can be
    migrated to common core kernel functionality, move the device-private
    specific actions in __put_devmap_managed_page() under the
    is_device_private_page() conditional, including the ->page_free()
    callback. For the other page types just open-code the generic wakeup.

    Yes, the wakeup is only needed in the MEMORY_DEVICE_FSDAX case, but it
    does no harm in the MEMORY_DEVICE_DEVDAX and MEMORY_DEVICE_PCI_P2PDMA
    case.

    Cc: Jan Kara <jack@suse.cz>
    Cc: Christoph Hellwig <hch@lst.de>
    Cc: Ira Weiny <ira.weiny@intel.com>
    Cc: John Hubbard <jhubbard@nvidia.com>
    Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
    Signed-off-by: Dan Williams <dan.j.williams@intel.com>
    ---
    Changes since v2:
    - Drop 'else' after return. (Christoph)

    drivers/nvdimm/pmem.c | 6 ----
    mm/memremap.c | 80 +++++++++++++++++++++++++++----------------------
    2 files changed, 44 insertions(+), 42 deletions(-)

    diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
    index f9f76f6ba07b..21db1ce8c0ae 100644
    --- a/drivers/nvdimm/pmem.c
    +++ b/drivers/nvdimm/pmem.c
    @@ -338,13 +338,7 @@ static void pmem_release_disk(void *__pmem)
    put_disk(pmem->disk);
    }

    -static void pmem_pagemap_page_free(struct page *page)
    -{
    - wake_up_var(&page->_refcount);
    -}
    -
    static const struct dev_pagemap_ops fsdax_pagemap_ops = {
    - .page_free = pmem_pagemap_page_free,
    .kill = pmem_pagemap_kill,
    .cleanup = pmem_pagemap_cleanup,
    };
    diff --git a/mm/memremap.c b/mm/memremap.c
    index 022e78e68ea0..e1678e575d9f 100644
    --- a/mm/memremap.c
    +++ b/mm/memremap.c
    @@ -27,7 +27,8 @@ static void devmap_managed_enable_put(void)

    static int devmap_managed_enable_get(struct dev_pagemap *pgmap)
    {
    - if (!pgmap->ops || !pgmap->ops->page_free) {
    + if (pgmap->type == MEMORY_DEVICE_PRIVATE &&
    + (!pgmap->ops || !pgmap->ops->page_free)) {
    WARN(1, "Missing page_free method\n");
    return -EINVAL;
    }
    @@ -444,44 +445,51 @@ void __put_devmap_managed_page(struct page *page)
    {
    int count = page_ref_dec_return(page);

    - /*
    - * If refcount is 1 then page is freed and refcount is stable as nobody
    - * holds a reference on the page.
    - */
    - if (count == 1) {
    - /* Clear Active bit in case of parallel mark_page_accessed */
    - __ClearPageActive(page);
    - __ClearPageWaiters(page);
    + /* still busy */
    + if (count > 1)
    + return;

    - mem_cgroup_uncharge(page);
    + /* only triggered by the dev_pagemap shutdown path */
    + if (count == 0) {
    + __put_page(page);
    + return;
    + }

    - /*
    - * When a device_private 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 and wrong, and it will cause errors if not cleared.
    - * One example is:
    - *
    - * migrate_vma_pages()
    - * migrate_vma_insert_page()
    - * page_add_new_anon_rmap()
    - * __page_set_anon_rmap()
    - * ...checks page->mapping, via PageAnon(page) call,
    - * and incorrectly concludes that the page is an
    - * anonymous page. Therefore, it incorrectly,
    - * silently fails to set up the new anon rmap.
    - *
    - * For other types of ZONE_DEVICE pages, migration is either
    - * handled differently or not done at all, so there is no need
    - * to clear page->mapping.
    - */
    - if (is_device_private_page(page))
    - page->mapping = NULL;
    + /* notify page idle for dax */
    + if (!is_device_private_page(page)) {
    + wake_up_var(&page->_refcount);
    + return;
    + }

    - page->pgmap->ops->page_free(page);
    - } else if (!count)
    - __put_page(page);
    + /* Clear Active bit in case of parallel mark_page_accessed */
    + __ClearPageActive(page);
    + __ClearPageWaiters(page);
    +
    + mem_cgroup_uncharge(page);
    +
    + /*
    + * When a device_private 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
    + * and wrong, and it will cause errors if not cleared. One
    + * example is:
    + *
    + * migrate_vma_pages()
    + * migrate_vma_insert_page()
    + * page_add_new_anon_rmap()
    + * __page_set_anon_rmap()
    + * ...checks page->mapping, via PageAnon(page) call,
    + * and incorrectly concludes that the page is an
    + * anonymous page. Therefore, it incorrectly,
    + * silently fails to set up the new anon rmap.
    + *
    + * For other types of ZONE_DEVICE pages, migration is either
    + * handled differently or not done at all, so there is no need
    + * to clear page->mapping.
    + */
    + page->mapping = NULL;
    + page->pgmap->ops->page_free(page);
    }
    EXPORT_SYMBOL(__put_devmap_managed_page);
    #endif /* CONFIG_DEV_PAGEMAP_OPS */
    \
     
     \ /
      Last update: 2019-11-14 17:26    [W:4.096 / U:1.764 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site