Re: [Linaro-mm-sig] [PATCH 1/2] dma-buf: Require VM_PFNMAP vma for mmap

From: Daniel Vetter <daniel@ffwll.ch>
To: "Christian König" <christian.koenig@amd.com>
Cc: "Thomas Hellström (Intel)" <thomas_os@shipmail.org>,
	"Christian König" <ckoenig.leichtzumerken@gmail.com>,
	"Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
	"Matthew Wilcox" <willy@infradead.org>,
	"moderated list:DMA BUFFER SHARING FRAMEWORK"
	<linaro-mm-sig@lists.linaro.org>,
	"Jason Gunthorpe" <jgg@ziepe.ca>,
	"John Stultz" <john.stultz@linaro.org>,
	"DRI Development" <dri-devel@lists.freedesktop.org>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Suren Baghdasaryan" <surenb@google.com>,
	"open list:DMA BUFFER SHARING FRAMEWORK"
	<linux-media@vger.kernel.org>
Subject: Re: [Linaro-mm-sig] [PATCH 1/2] dma-buf: Require VM_PFNMAP vma for mmap
Date: Mon, 1 Mar 2021 15:09:10 +0100	[thread overview]
Message-ID: <CAKMK7uHOe=LacUkvGC75dyWAt9TRm7ce8vgxasXOXn-6wJTVnA@mail.gmail.com> (raw)
In-Reply-To: <648556e6-2d99-950d-c940-706eb5a8f6cc@amd.com>

On Mon, Mar 1, 2021 at 11:17 AM Christian König
<christian.koenig@amd.com> wrote:
>
>
>
> Am 01.03.21 um 10:21 schrieb Thomas Hellström (Intel):
> >
> > On 3/1/21 10:05 AM, Daniel Vetter wrote:
> >> On Mon, Mar 01, 2021 at 09:39:53AM +0100, Thomas Hellström (Intel)
> >> wrote:
> >>> Hi,
> >>>
> >>> On 3/1/21 9:28 AM, Daniel Vetter wrote:
> >>>> On Sat, Feb 27, 2021 at 9:06 AM Thomas Hellström (Intel)
> >>>> <thomas_os@shipmail.org> wrote:
> >>>>> On 2/26/21 2:28 PM, Daniel Vetter wrote:
> >>>>>> So I think it stops gup. But I haven't verified at all. Would be
> >>>>>> good
> >>>>>> if Christian can check this with some direct io to a buffer in
> >>>>>> system
> >>>>>> memory.
> >>>>> Hmm,
> >>>>>
> >>>>> Docs (again vm_normal_page() say)
> >>>>>
> >>>>>     * VM_MIXEDMAP mappings can likewise contain memory with or
> >>>>> without "struct
> >>>>>     * page" backing, however the difference is that _all_ pages
> >>>>> with a struct
> >>>>>     * page (that is, those where pfn_valid is true) are refcounted
> >>>>> and
> >>>>> considered
> >>>>>     * normal pages by the VM. The disadvantage is that pages are
> >>>>> refcounted
> >>>>>     * (which can be slower and simply not an option for some PFNMAP
> >>>>> users). The
> >>>>>     * advantage is that we don't have to follow the strict
> >>>>> linearity rule of
> >>>>>     * PFNMAP mappings in order to support COWable mappings.
> >>>>>
> >>>>> but it's true __vm_insert_mixed() ends up in the insert_pfn()
> >>>>> path, so
> >>>>> the above isn't really true, which makes me wonder if and in that
> >>>>> case
> >>>>> why there could any longer ever be a significant performance
> >>>>> difference
> >>>>> between MIXEDMAP and PFNMAP.
> >>>> Yeah it's definitely confusing. I guess I'll hack up a patch and see
> >>>> what sticks.
> >>>>
> >>>>> BTW regarding the TTM hugeptes, I don't think we ever landed that
> >>>>> devmap
> >>>>> hack, so they are (for the non-gup case) relying on
> >>>>> vma_is_special_huge(). For the gup case, I think the bug is still
> >>>>> there.
> >>>> Maybe there's another devmap hack, but the ttm_vm_insert functions do
> >>>> use PFN_DEV and all that. And I think that stops gup_fast from trying
> >>>> to find the underlying page.
> >>>> -Daniel
> >>> Hmm perhaps it might, but I don't think so. The fix I tried out was
> >>> to set
> >>>
> >>> PFN_DEV | PFN_MAP for huge PTEs which causes pfn_devmap() to be
> >>> true, and
> >>> then
> >>>
> >>> follow_devmap_pmd()->get_dev_pagemap() which returns NULL and
> >>> gup_fast()
> >>> backs off,
> >>>
> >>> in the end that would mean setting in stone that "if there is a huge
> >>> devmap
> >>> page table entry for which we haven't registered any devmap struct
> >>> pages
> >>> (get_dev_pagemap returns NULL), we should treat that as a "special"
> >>> huge
> >>> page table entry".
> >>>
> >>>  From what I can tell, all code calling get_dev_pagemap() already
> >>> does that,
> >>> it's just a question of getting it accepted and formalizing it.
> >> Oh I thought that's already how it works, since I didn't spot anything
> >> else that would block gup_fast from falling over. I guess really would
> >> need some testcases to make sure direct i/o (that's the easiest to test)
> >> fails like we expect.
> >
> > Yeah, IIRC the "| PFN_MAP" is the missing piece for TTM huge ptes.
> > Otherwise pmd_devmap() will not return true and since there is no
> > pmd_special() things break.
>
> Is that maybe the issue we have seen with amdgpu and huge pages?

Yeah, essentially when you have a hugepte inserted by ttm, and it
happens to point at system memory, then gup will work on that. And
create all kinds of havoc.

> Apart from that I'm lost guys, that devmap and gup stuff is not
> something I have a good knowledge of apart from a one mile high view.

I'm not really better, hence would be good to do a testcase and see.
This should provoke it:
- allocate nicely aligned bo in system memory
- mmap, again nicely aligned to 2M
- do some direct io from a filesystem into that mmap, that should trigger gup
- before the gup completes free the mmap and bo so that ttm recycles
the pages, which should trip up on the elevated refcount. If you wait
until the direct io is completely, then I think nothing bad can be
observed.

Ofc if your amdgpu+hugepte issue is something else, then maybe we have
another issue.

Also usual caveat: I'm not an mm hacker either, so might be completely wrong.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch