Message-ID: <1af66528-0551-4735-87f3-d5feadadf33a@lucifer.local>
Date: Thu, 28 Nov 2024 13:20:34 +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 Thu, Nov 28, 2024 at 02:08:27PM +0100, David Hildenbrand wrote:
> On 28.11.24 12:37, Lorenzo Stoakes wrote:
> > We are current refactoring struct page to make it smaller, removing
> > unneeded fields that correctly belong to struct folio.
> >
> > Two of those fields are page->index and page->mapping. Perf is currently
> > making use of both of these, so this patch removes this usage as it turns
> > out it is unnecessary.
> >
> > Perf establishes its own internally controlled memory-mapped pages using
> > vm_ops hooks. The first page in the mapping is the read/write user control
> > page, and the rest of the mapping consists of read-only pages.
> >
> > The VMA is backed by kernel memory either from the buddy allocator or
> > vmalloc depending on configuration. It is intended to be mapped read/write,
> > but because it has a page_mkwrite() hook, vma_wants_writenotify() indicaets
> > that it should be mapped read-only.
> >
> > When a write fault occurs, the provided page_mkwrite() hook,
> > perf_mmap_fault() (doing double duty handing faults as well) uses the
> > vmf->pgoff field to determine if this is the first page, allowing for the
> > desired read/write first page, read-only rest mapping.
> >
> > For this to work the implementation has to carefully work around faulting
> > logic. When a page is write-faulted, the fault() hook is called first, then
> > its page_mkwrite() hook is called (to allow for dirty tracking in file
> > systems).
> >
> > On fault we set the folio's mapping in perf_mmap_fault(), this is because
> > when do_page_mkwrite() is subsequently invoked, it treats a missing mapping
> > as an indicator that the fault should be retried.
> >
> > We also set the folio's index so, given the folio is being treated as faux
> > user memory, it correctly references its offset within the VMA.
> >
> > This explains why the mapping and index fields are used - but it's not
> > necessary.
> >
> > We preallocate pages when perf_mmap() is called for the first time via
> > rb_alloc(), and further allocate auxiliary pages via rb_aux_alloc() as
> > needed if the mapping requires it.
> >
> > This allocation is done in the f_ops->mmap() hook provided in perf_mmap(),
> > and so we can instead simply map all the memory right away here - there's
> > no point in handling (read) page faults when we don't demand page nor need
> > to be notified about them (perf does not).
> >
> > This patch therefore changes this logic to map everything when the mmap()
> > hook is called, establishing a PFN map. It implements vm_ops->pfn_mkwrite()
> > to provide the required read/write vs. read-only behaviour, which does not
> > require the previously implemented workarounds.
> >
> > It makes sense semantically to establish a PFN map too - we are managing
> > the pages internally and so it is appropriate to mark this as a special
> > mapping.
>
> It's rather sad seeing more PFNMAP users where PFNMAP is not really required
> (-> this is struct page backed).
>
> Especially having to perform several independent remap_pfn_range() calls
> rather looks like yet another workaround ...
>
> Would we be able to achieve something comparable with vm_insert_pages(), to
> just map them in advance?

Well, that's the thing, we can't use VM_MIXEDMAP as vm_insert_pages() and
friends all refer vma->vm_page_prot which is not yet _correctly_ established at
the point of the f_op->mmap() hook being invoked :)

We set the field in __mmap_new_vma(), _but_ importantly, we defer the
writenotify check to __mmap_complete() (set in vma_set_page_prot()) - so if we
were to try to map using VM_MIXEDMAP in the f_op->mmap() hook, we'd get
read/write mappings, which is emphatically not what we want - we want them
read-only mapped, and for vm_ops->pfn_mkwrite() to be called so we can make the
first page read/write and the rest read-only.

It's this requirement that means this is really the only way to do this as far
as I can tell.

It is appropriate and correct that this is either a VM_PFNMAP or VM_MIXEDMAP
mapping, as the pages reference kernel-allocated memory and are managed by perf,
not put on any LRU, etc.

It sucks to have to loop like this and it feels like a workaround, which makes
me wonder if we need a new interface to better allow this stuff on mmap...

In any case I think this is the most sensible solution currently available that
avoids the pre-existing situation of pretending the pages are folios but
somewhat abusing the interface to allow page_mkwrite() to work correctly by
setting page->index, mapping.

The alternative to this would be to folio-fy, but these are emphatically _not_
folios, that is userland memory managed as userland memory, it's a mapping onto
kernel memory exposed to userspace.

It feels like probably VM_MIXEDMAP is a better way of doing it, but you'd need
to expose an interface that doesn't assume the VMA is already fully set
up... but I think one for a future series perhaps.

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

Powered by blists - more mailing lists