Date:   Fri, 29 Oct 2021 13:04:22 -0700
From:   Mina Almasry <almasrymina@...gle.com>
To:     David Hildenbrand <david@...hat.com>, Nathan Lewis <npl@...gle.com>
Cc:     Yu Zhao <yuzhao@...gle.com>,
        "Paul E . McKenney" <paulmckrcu@...com>,
        Jonathan Corbet <corbet@....net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Xu <peterx@...hat.com>,
        Ivan Teterevkov <ivan.teterevkov@...anix.com>,
        Florian Schmidt <florian.schmidt@...anix.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH v1] mm: Add /proc/$PID/pageflags

On Fri, Oct 29, 2021 at 12:11 AM David Hildenbrand <david@...hat.com> wrote:
>
> On 28.10.21 22:58, Mina Almasry wrote:
> > From: Yu Zhao <yuzhao@...gle.com>
> >
> > This file lets a userspace process know the page flags of each of its virtual
> > pages.  It contains a 64-bit set of flags for each virtual page, containing
> > data identical to that emitted by /proc/kpageflags.  This allows the user-space
> > task can learn the kpageflags for the pages backing its address-space by
> > consulting one file, without needing to be root.
> >
> > Example use case is a performance sensitive user-space process querying the
> > hugepage backing of its own memory without the root access required to access
> > /proc/kpageflags, and without accessing /proc/self/smaps_rollup which can be
> > slow and needs to hold mmap_lock.
>
> Can you elaborate on
>
> a) The target use case. Are you primarily interested to see if a page
> given base page is head or tail?
>

Not quite. Generally some userspace process (most notably our network
service) has a region of performance critical memory and would like to
know if this memory is backed by hugepages or not. It uses
/proc/self/pageflags to inspect the pageflags of the pages backing
this region, and counts how many ranges are backed by hugepages and
how many are not. Generally we export this data to metrics, and if the
hugepage backing drops or is insufficient we look into the issue
postmortem.

> b) Your mmap_lock comment. pagemap_read() needs to hold the mmap lock in
> read mode while walking process page tables via walk_page_range().
>

Gah, I'm _very_ sorry for the misinformation. I was (very incorrectly)
under the impression that /proc/self/smaps_rollup required holding the
mmap lock but /proc/self/pageflags didn't. I'll remove the comment
about the mmap lock from the commit message in V2.

> Also, do you have a rough performance comparison?
>

So from my tests with simple processes querying smaps/pageflags I
don't see any performance difference, but I suspect it's due to my
test cases not mapping much memory or regions.

I've CC'd Nathan who works on our network service and has run into
performance issues with smaps. Nathan, do you have a rough performance
comparison? If so please do share.

> >
> > Similar to /proc/kpageflags, the flags printed out by the kernel for
> > each page are provided by stable_page_flags(), which exports flag bits
> > that are user visible and stable over time.
>
> It exports flags (documented for pageflags_read()) that are not
> applicable to processes, like OFFLINE. BUDDY, SLAB, PGTABLE ... and can
> never happen. Some of these kpageflags are not even page->flags, they
> include abstracted types we use for physical memory pages based on other
> struct page members (OFFLINE, BUDDY, MMAP, PGTABLE, ...). This feels wrong.
>
> Also, to me it feels like we are exposing too much internal information
> to the user, essentially making it ABI that user space processes will
> rely on.
>

I'm honestly a bit surprised by this comment because AFAIU (sorry if
wrong) we are already exporting this information via /proc/kpageflags
and therefore it's already somewhat part of an ABI, and the
stable_page_flags() output already needs to be stable and backwards
compatible due to potential root users being affected by any
non-backwards compatible changes. I am yes extending access to this
information to non-root users.

> Did you investigate
>
> a) Reducing the flags we expose to a bare minimum necessary for your use
> case (and actually applicable to mmaped pages).
>

To be honest I haven't, but this is something that's certainly doable.
I'm not sure it's easier for processes to understand or the kernel to
maintain. My thinking:
1. Processes parsing /proc/kpageflags can also easily parse
/proc/self/pageflags and re-use code/implementations between them.
2. Userspace code can extract the flags they need and ignore the ones
they don't need or are not applicable.
3. For kernel it's maybe easier to maintain 1 set of
stable_page_flags() and keep that list backwards compatible. To
address your comment I'd need to create a subset,
stable_ps_page_flags(), and both lists now need to be backwards
compatible.

But I hear you, and if you feel strongly about this I'm more than
happy to oblige. Please confirm if this is something you would like to
see in V2.

> b) Extending pagemap output instead.
>

No I have not until you mentioned it, but even now AFAIU (and again
sorry if wrong, please correct) all the bits exposed by pagemap as
documented in pagemap.rst are in use, and it's a non-starter for me to
modify how pagemap works because it'd break backwards compatibility.
But if you see a way I'm happy to oblige :-)

Thanks for your review!

> You seem to be interested in the "hugepage backing", which smells like
> "what is mapped" as in "pagemap".
>
>
> --
> Thanks,
>
> David / dhildenb
>

Powered by blists - more mailing lists