On 10/14/21 10:06 AM, Jason Gunthorpe wrote:
On Thu, Oct 14, 2021 at 10:39:28AM -0500, Alex Sierra wrote:
From: Ralph Campbell <rcampbell@xxxxxxxxxx>
ZONE_DEVICE struct pages have an extra reference count that complicates the
code for put_page() and several places in the kernel that need to check the
reference count to see that a page is not being used (gup, compaction,
migration, etc.). Clean up the code so the reference count doesn't need to
be treated specially for ZONE_DEVICE.
Signed-off-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
Signed-off-by: Alex Sierra <alex.sierra@xxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
---
v2:
AS: merged this patch in linux 5.11 version
v5:
AS: add condition at try_grab_page to check for the zone device type, while
page ref counter is checked less/equal to zero. In case of device zone, pages
ref counter are initialized to zero.
v7:
AS: fix condition at try_grab_page added at v5, is invalid. It supposed
to fix xfstests/generic/413 test, however, there's a known issue on
this test where DAX mapped area DIO to non-DAX expect to fail.
https://patchwork.kernel.org/project/fstests/patch/1489463960-3579-1-git-send-email-xzhou@xxxxxxxxxx
This condition was removed after rebase over patch series
https://lore.kernel.org/r/20210813044133.1536842-4-jhubbard@xxxxxxxxxx
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_dmem.c | 2 +-
fs/dax.c | 4 +-
include/linux/dax.h | 2 +-
include/linux/memremap.h | 7 +--
include/linux/mm.h | 11 ----
lib/test_hmm.c | 2 +-
mm/internal.h | 8 +++
mm/memcontrol.c | 6 +--
mm/memremap.c | 69 +++++++-------------------
mm/migrate.c | 5 --
mm/page_alloc.c | 3 ++
mm/swap.c | 45 ++---------------
13 files changed, 46 insertions(+), 120 deletions(-)
Has anyone tested this with FSDAX? Does get_user_pages() on fsdax
backed memory still work?
I ran xfstests-dev using the kernel boot option to "fake" a pmem device
when I first posted this patch. The tests ran OK (or at least the same
tests passed with and without my patch). However, I could never really
convince myself the changes were "OK" for fsdax since I didn't understand
the code that well. I would still like to see a xfsdax maintainer or
expert ACK this change.
https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
What refcount value does the struct pages have when they are installed
in the PTEs? Remember a 0 refcount will make all the get_user_pages()
fail.
I'm looking at the call path starting in ext4_punch_hole() and I would
expect to see something manipulating the page ref count before
the ext4_break_layouts() call path gets to the dax_page_unused() test.
All I see is we go into unmap_mapping_pages() - that would normally
put back the page references held by PTEs but insert_pfn() has this:
if (pfn_t_devmap(pfn))
entry = pte_mkdevmap(pfn_t_pte(pfn, prot));
And:
static inline pte_t pte_mkdevmap(pte_t pte)
{
return pte_set_flags(pte, _PAGE_SPECIAL|_PAGE_DEVMAP);
}
Which interacts with vm_normal_page():
if (pte_devmap(pte))
return NULL;
To disable that refcounting?
So... I have a feeling this will have PTEs pointing to 0 refcount
pages? Unless FSDAX is !pte_devmap which is not the case, right?
This seems further confirmed by this comment:
/*
* If we race get_user_pages_fast() here either we'll see the
* elevated page count in the iteration and wait, or
* get_user_pages_fast() will see that the page it took a reference
* against is no longer mapped in the page tables and bail to the
* get_user_pages() slow path. The slow path is protected by
* pte_lock() and pmd_lock(). New references are not taken without
* holding those locks, and unmap_mapping_pages() will not zero the
* pte or pmd without holding the respective lock, so we are
* guaranteed to either see new references or prevent new
* references from being established.
*/
Which seems to explain this scheme relies on unmap_mapping_pages() to
fence GUP_fast, not on GUP_fast observing 0 refcounts when it should
stop.
This seems like it would be properly fixed by using normal page
refcounting for PTEs - ie stop using special for these pages?
Does anyone know why devmap is pte_special anyhow?
+void free_zone_device_page(struct page *page)
+{
+ switch (page->pgmap->type) {
+ case MEMORY_DEVICE_PRIVATE:
+ free_device_page(page);
+ return;
+ case MEMORY_DEVICE_FS_DAX:
+ /* notify page idle */
+ wake_up_var(&page->_refcount);
+ return;
It is not for this series, but I wonder if we should just always call
ops->page_free and have free_device_page() logic in that callback for
the non-fs-dax cases?
For instance where is the mem_cgroup_charge() call to pair with the
mem_cgroup_uncharge() in free_device_page()?
Isn't cgroup charging (or not) the responsibility of the "allocator"
eg the pgmap_ops owner?
Jason
