Re: [mm-unstable v7 13/18] proc/smaps: add PMDMappable field to smaps

[Zach O'Keefe <zokeefe@google.com>]

On Jul 11 14:37, Yang Shi wrote:
> On Wed, Jul 6, 2022 at 5:06 PM Zach O'Keefe <zokeefe@google.com> wrote:
> >
> > Add PMDMappable field to smaps output which informs the user if memory
> > in the VMA can be PMD-mapped by MADV_COLLAPSE.
> >
> > The distinction from THPeligible is needed for two reasons:
> >
> > 1) For THP, MADV_COLLAPSE is not coupled to THP sysfs controls, which
> >    THPeligible reports.
> >
> > 2) PMDMappable can also be used in HugeTLB fine-granularity mappings,
> >    which are independent from THP.
> 
> Could you please elaborate the usecase? The user checks this hint
> before calling MADV_COLLAPSE? Is it really necessary?
> 
> And, TBH it sounds confusing and we don't have to maintain both
> THPeligible and PMDMappable. We could just relax THPeligible to make
> it return 1 even though THP is disabled by sysfs but MADV_COLLAPSE
> could collapse it if such hint is useful.
> 

Hey Yang,

Thanks for taking the time to review this series again, and thanks for
challenging this.

TLDR: "Is it really necessary" - at the moment, no, probably not .. but I think
it's "useful".

Rationale:

1. IMO, I thought was was confusing seeing:

	...
	AnonHugePages:      2048 kB
	ShmemPmdMapped:        0 kB
	FilePmdMapped:         0 kB
	Shared_Hugetlb:        0 kB
	Private_Hugetlb:       0 kB
	Swap:                  0 kB
	SwapPss:               0 kB
	Locked:                0 kB
	THPeligible:    0
	...

Maybe this could simply be clarified in the docs though.  I guess we can already
get:

	...
	AnonHugePages:         0 kB
	ShmemPmdMapped:        0 kB
	FilePmdMapped:      2048 kB
	Shared_Hugetlb:        0 kB
	Private_Hugetlb:       0 kB
	Swap:                  0 kB
	SwapPss:               0 kB
	Locked:                0 kB
	THPeligible:    0
	...

today[1], so perhaps it's not a big deal.


2. It was useful for debugging - similar to rationale for including
THPeligible1[2], the logic for determining if a VMA is eligible is pretty
complicated. I.e. is this file mapped suitably? Unlike THPeligible, however,
madvise(2) has the ability to set errno on failure to help* diagnose why some
memory isn't being backed.

3. For the immediately-envisioned usecases, the user "knows" about what memory
they are acting on. However, eventually we'd like to experiment with moving THP
utilization policy to userspace. Here, it would be useful if the userspace agent
managing was made aware of what memory it should be managing. I don't have a
working prototype of what this would like yet, however.

4. I thought it was neat that this field could be reused for HugeTLB
fine-granularity mappings - but TBH I'm not sure how useful it'd be there.

I figured relaxing existing THPeligible could break existing users / tests, and
it'd be likewise confusing for them to see THPeligible:	1, but then have faults
fail and they'd then have to go check sysfs settings and vma flags ; we'd be
back in pre-commit 7635d9cbe832 ("mm, thp, proc: report THP eligibility for each
vma").

Thanks,
Zach

[1] https://lore.kernel.org/linux-mm/YrxbQGiwml24APCx@google.com/


> 
> >
> > Signed-off-by: Zach O'Keefe <zokeefe@google.com>
> > ---
> >  Documentation/filesystems/proc.rst | 10 ++++++++--
> >  fs/proc/task_mmu.c                 |  2 ++
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> > index 47e95dbc820d..f207903a57a5 100644
> > --- a/Documentation/filesystems/proc.rst
> > +++ b/Documentation/filesystems/proc.rst
> > @@ -466,6 +466,7 @@ Memory Area, or VMA) there is a series of lines such as the following::
> >      MMUPageSize:           4 kB
> >      Locked:                0 kB
> >      THPeligible:           0
> > +    PMDMappable:           0
> >      VmFlags: rd ex mr mw me dw
> >
> >  The first of these lines shows the same information as is displayed for the
> > @@ -518,9 +519,14 @@ replaced by copy-on-write) part of the underlying shmem object out on swap.
> >  does not take into account swapped out page of underlying shmem objects.
> >  "Locked" indicates whether the mapping is locked in memory or not.
> >
> > +"PMDMappable" indicates if the memory can be mapped by PMDs - 1 if true, 0
> > +otherwise.  It just shows the current status. Note that this is memory
> > +operable on explicitly by MADV_COLLAPSE.
> > +
> >  "THPeligible" indicates whether the mapping is eligible for allocating THP
> > -pages as well as the THP is PMD mappable or not - 1 if true, 0 otherwise.
> > -It just shows the current status.
> > +pages by the kernel, as well as the THP is PMD mappable or not - 1 if true, 0
> > +otherwise. It just shows the current status.  Note this is memory the kernel can
> > +transparently provide as THPs.
> >
> >  "VmFlags" field deserves a separate description. This member represents the
> >  kernel flags associated with the particular virtual memory area in two letter
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index f8cd58846a28..29f2089456ba 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -867,6 +867,8 @@ static int show_smap(struct seq_file *m, void *v)
> >
> >         seq_printf(m, "THPeligible:    %d\n",
> >                    hugepage_vma_check(vma, vma->vm_flags, true, false, true));
> > +       seq_printf(m, "PMDMappable:    %d\n",
> > +                  hugepage_vma_check(vma, vma->vm_flags, true, false, false));
> >
> >         if (arch_pkeys_enabled())
> >                 seq_printf(m, "ProtectionKey:  %8u\n", vma_pkey(vma));
> > --
> > 2.37.0.rc0.161.g10f37bed90-goog
> >