Re: [PATCH v2 2/2] smaps: set THPeligible if file mapping supports large folios

[Zach O'Keefe]

On Fri, Aug 18, 2023 at 3:17 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Fri, Aug 18, 2023 at 03:14:02PM -0700, Zach O'Keefe wrote:
> > Sorry -- noticed only too late that there are still many
> > false-negatives for THPeligible, since by this point in the function
> > we've already applied sysfs and prctl restrictions, which file-fault
> > ignores. VM_HUGEPAGE also needs to be checked for the file-fault case.
>
> I'm not entirely convinced that unifying all of these things leads
> to code that's simpler to understand.
>

I'm trying my hand at rearranging this particular function to make it
simpler -- but thought the refactor better left for a follow-up patch.

This patch was just about surfacing the possibility of getting THPs
through file fault to the user. Maybe the inverse is more frustrating
(claiming THPs are eligible, but then never getting them), but given
we expose the field, I feel like we might as well try to make it
accurate. The actual complexity is just a reflection of the the
different eligibility requirements chosen for each of
(anon, shmem, file) x (fault, khugepaged, MADV_COLLAPSE).



> > On Fri, Aug 18, 2023 at 2:15 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
> > >
> > > File-backed memory can be backed by THPs either through collapse, when
> > > CONFIG_READ_ONLY_THP_FOR_FS is enabled, or through fault, when the
> > > filesystem supports large folio mappings.
> > >
> > > Currently, smaps only knows about the former, so teach it about the
> > > latter.
> > >
> > > Signed-off-by: Zach O'Keefe <zokeefe@xxxxxxxxxx>
> > > Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > > ---
> > >  mm/huge_memory.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index cd379b2c077b..d8d6e83820f3 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -136,7 +136,16 @@ bool hugepage_vma_check(struct vm_area_struct *vma, unsigned long vm_flags,
> > >                          */
> > >                         !!vma->vm_ops->huge_fault :
> > >                         /* Only regular file is valid in collapse path */
> > > -                       file_thp_enabled(vma);
> > > +                       file_thp_enabled(vma) ||
> > > +                        /*
> > > +                         * THPeligible bit of smaps should surface the
> > > +                         * possibility of THP through fault if the filesystem
> > > +                         * supports it.  We don't check this in fault path,
> > > +                         * because we want to fallback to the actual ->fault()
> > > +                         * handler to make the decision.
> > > +                         */
> > > +                        (smaps && vma->vm_file &&
> > > +                        mapping_large_folio_support(vma->vm_file->f_mapping));
> > >
> > >         if (vma_is_temporary_stack(vma))
> > >                 return false;
> > > --
> > > 2.42.0.rc1.204.g551eb34607-goog
> > >