Message-ID: <60191c97-dce2-4a92-8b47-c402478ba336@lucifer.local>
Date: Fri, 29 Nov 2024 13:38:47 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH] perf: map pages in advance

On Fri, Nov 29, 2024 at 02:24:24PM +0100, David Hildenbrand wrote:
> On 29.11.24 14:19, Lorenzo Stoakes wrote:
> > On Fri, Nov 29, 2024 at 02:12:23PM +0100, David Hildenbrand wrote:
> > > On 29.11.24 14:02, Lorenzo Stoakes wrote:
> > > > On Fri, Nov 29, 2024 at 01:59:01PM +0100, David Hildenbrand wrote:
> > > > > On 29.11.24 13:55, Lorenzo Stoakes wrote:
> > > > > > On Fri, Nov 29, 2024 at 01:45:42PM +0100, David Hildenbrand wrote:
> > > > > > > On 29.11.24 13:26, Peter Zijlstra wrote:
> > > > > > > > On Fri, Nov 29, 2024 at 01:12:57PM +0100, David Hildenbrand wrote:
> > > > > > > >
> > > > > > > > > Well, I think we simply will want vm_insert_pages_prot() that stops treating
> > > > > > > > > these things like folios :) . *likely*  we'd want a distinct memdesc/type.
> > > > > > > > >
> > > > > > > > > We could start that work right now by making some user (iouring,
> > > > > > > > > ring_buffer) set a new page->_type, and checking that in
> > > > > > > > > vm_insert_pages_prot() + vm_normal_page(). If set, don't touch the refcount
> > > > > > > > > and the mapcount.
> > > > > > > > >
> > > > > > > > > Because then, we can just make all the relevant drivers set the type, refuse
> > > > > > > > > in vm_insert_pages_prot() anything that doesn't have the type set, and
> > > > > > > > > refuse in vm_normal_page() any pages with this memdesc.
> > > > > > > > >
> > > > > > > > > Maybe we'd have to teach CoW to copy from such pages, maybe not. GUP of
> > > > > > > > > these things will stop working, I hope that is not a problem.
> > > > > > > >
> > > > > > > > Well... perf-tool likes to call write() upon these pages in order to
> > > > > > > > write out the data from the mmap() into a file.
> > > > > >
> > > > > > I'm confused about what you mean, write() using the fd should work fine, how
> > > > > > would they interact with the mmap? I mean be making a silly mistake here
> > > > >
> > > > > write() to file from the mmap()'ed address range to *some* file.
> > > > >
> > > >
> > > > Yeah sorry my brain melted down briefly, for some reason was thinking of read()
> > > > writing into the buffer...
> > > >
> > > > > This will GUP the pages you inserted.
> > > > >
> > > > > GUP does not work on PFNMAP.
> > > >
> > > > Well it _does_ if struct page **pages is set to NULL :)
> > >
> > > Hm? :)
> > >
> > > check_vma_flags() unconditionally refuses VM_PFNMAP.
> >
> > Ha, funny with my name all over git blame there... ok yup missed this, the
> > vm_normal_page() == NULL stuff must but for mixed map (and those other weird
> > cases I think you can get0...
> >
> > Well good. Where is write() invoking GUP? I'm kind of surprised it's not just
> > using uaccess?
> >
> > One thing to note is I did run all the perf tests with no issues whatsoever. You
> > would _think_ this would have come up...
> >
> > I'm editing some test code to explicitly write() from the buffer anyway to see.

I just tested it and write() works fine, it uses uaccess afaict as part of the
lib/iov_iter.c code:

generic_perform_write()
-> copy_folio_from_iter_atomic()
-> copy_page_from_iter_atomic()
-> __copy_from_iter()
-> copy_from_user_iter()
-> raw_copy_from_user()
-> copy_user_generic()
-> [uaccess asm]

> >
> > If we can't do pfnmap, and we definitely can't do mixedmap (because it's
> > basically entirely equivalent in every way to just faulting in the pages as
> > before and requires the same hacks) then I will have to go back to the drawing
> > board or somehow change the faulting code.
> >
> > This really sucks.
> >
> > I'm not quite sure I even understand why we don't allow GUP used _just for
> > pinning_ on VM_PFNMAP when it is -in effect- already pinned on assumption
> > whatever mapped it will maintain the lifetime.
> >
> > What a mess...
>
> Because VM_PFNMAP is dangerous as hell. To get a feeling for that (and also why I
> raised my refcounting comment earlier) just read recent:
>
> commit 79a61cc3fc0466ad2b7b89618a6157785f0293b3
> Author: Linus Torvalds <torvalds@...ux-foundation.org>
> Date:   Wed Sep 11 17:11:23 2024 -0700
>
>     mm: avoid leaving partial pfn mappings around in error case
>     As Jann points out, PFN mappings are special, because unlike normal
>     memory mappings, there is no lifetime information associated with the
>     mapping - it is just a raw mapping of PFNs with no reference counting of
>     a 'struct page'.
>

I'm _very_ aware of this, having worked extensively on things around this kind
of issue recently (was a little bit involved with the above one too :), and
explicitly zap on error in this patch to ensure we leave no broken code paths.

I agree it's horrible, but we need to have a way of mapping this memory without
having to 'trick' the faulting mechanism to behave correctly.

At least in perf's case, we're safe, because we ref count in open/close of VMA's.

This is a special case due to the R/W, R/O thing.

In reference to that - you said in another email about mapping one part as a
separate R/W VMA and another as a separate R/O VMA, problem is all of the perf
code is set up with its own reference counting mechanism and it's not allowed to
split/merge etc., so we'd have to totally rework all of that to make that work
and correctly refcount things.

It'd be a big task. I don't think that's a reasonable thing to put effort into
at this time...

Also who knows if there's somebody, somewhere who _relies_ somehow on this being
a single mapping...

>
> GUP relies on the refcount. In a PFNMAP you don't have any way to make sure the
> driver won't go down, free the page, to have it used by someone else while IO is still
> happening to that page.

GUP isn't required here afaict, having eliminated write() as an issue.

I mean there's definitely things we need to fix here, but I think it's out of
scope for this fix.

>
> --
> Cheers,
>
> David / dhildenb
>

Powered by blists - more mailing lists