On Fri, Oct 29, 2021 at 12:11 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 28.10.21 22:58, Mina Almasry wrote: > > From: Yu Zhao <yuzhao@xxxxxxxxxx> > > > > 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 >
