From: Ralph Campbell <rcampbell@nvidia.com>
To: Jason Gunthorpe <jgg@nvidia.com>, Alex Sierra <alex.sierra@amd.com>
Cc: <akpm@linux-foundation.org>, <Felix.Kuehling@amd.com>,
<linux-mm@kvack.org>, <linux-ext4@vger.kernel.org>,
<linux-xfs@vger.kernel.org>, <amd-gfx@lists.freedesktop.org>,
<dri-devel@lists.freedesktop.org>, <hch@lst.de>,
<jglisse@redhat.com>, <apopple@nvidia.com>, <willy@infradead.org>
Subject: Re: [PATCH v1 2/2] mm: remove extra ZONE_DEVICE struct page refcount
Date: Thu, 14 Oct 2021 10:35:27 -0700 [thread overview]
Message-ID: <eafbccb5-f94b-0ddd-bb46-7ee92ed36ee8@nvidia.com> (raw)
In-Reply-To: <20211014170634.GV2744544@nvidia.com>
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@nvidia.com>
>>
>> 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@nvidia.com>
>> Signed-off-by: Alex Sierra <alex.sierra@amd.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>> 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@redhat.com
>> This condition was removed after rebase over patch series
>> https://lore.kernel.org/r/20210813044133.1536842-4-jhubbard@nvidia.com
>> ---
>> 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
next prev parent reply other threads:[~2021-10-14 17:35 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-14 15:39 [PATCH v1 0/2] mm: remove extra ZONE_DEVICE struct page refcount Alex Sierra
2021-10-14 15:39 ` [PATCH v1 1/2] ext4/xfs: add page refcount helper Alex Sierra
2021-10-14 16:25 ` Jason Gunthorpe
2021-10-14 16:40 ` Matthew Wilcox
2021-10-14 15:39 ` [PATCH v1 2/2] mm: remove extra ZONE_DEVICE struct page refcount Alex Sierra
2021-10-14 16:52 ` Matthew Wilcox
2021-10-14 17:06 ` Jason Gunthorpe
2021-10-14 17:35 ` Ralph Campbell [this message]
2021-10-14 18:01 ` Jason Gunthorpe
2021-10-14 20:57 ` Ralph Campbell
2021-10-15 3:45 ` Sierra Guiza, Alejandro (Alex)
2021-10-15 11:06 ` Jason Gunthorpe
2021-10-14 18:43 ` Matthew Wilcox
2021-10-14 19:01 ` Dan Williams
2021-10-14 23:06 ` Jason Gunthorpe
2021-10-15 1:37 ` Dan Williams
2021-10-16 15:44 ` Jason Gunthorpe
2021-10-16 16:39 ` Matthew Wilcox
2021-10-17 18:20 ` Dan Williams
2021-10-17 18:35 ` Dan Williams
2021-10-18 18:25 ` Jason Gunthorpe
2021-10-18 19:37 ` Dan Williams
2021-10-18 23:06 ` Jason Gunthorpe
2021-10-19 15:13 ` Joao Martins
2021-10-19 16:01 ` Jason Gunthorpe
2021-10-19 19:21 ` Dan Williams
2021-10-20 17:06 ` Joao Martins
2021-10-20 17:12 ` Dan Williams
2021-10-20 18:51 ` Joao Martins
2021-11-15 19:33 [PATCH v1 0/2] Remove extra ZONE_DEVICE " Alex Sierra
2021-11-15 19:33 ` [PATCH v1 2/2] mm: remove extra ZONE_DEVICE struct " Alex Sierra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eafbccb5-f94b-0ddd-bb46-7ee92ed36ee8@nvidia.com \
--to=rcampbell@nvidia.com \
--cc=Felix.Kuehling@amd.com \
--cc=akpm@linux-foundation.org \
--cc=alex.sierra@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=apopple@nvidia.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hch@lst.de \
--cc=jgg@nvidia.com \
--cc=jglisse@redhat.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-xfs@vger.kernel.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).