| Message ID | 20230818211533.2523697-2-zokeefe@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] mm/thp: fix "mm: thp: kill __transhuge_page_enabled()" | expand |
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. On Fri, Aug 18, 2023 at 2:15 PM Zach O'Keefe <zokeefe@google.com> 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@google.com> > Cc: Matthew Wilcox <willy@infradead.org> > --- > 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 >
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. > On Fri, Aug 18, 2023 at 2:15 PM Zach O'Keefe <zokeefe@google.com> 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@google.com> > > Cc: Matthew Wilcox <willy@infradead.org> > > --- > > 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 > >
On Fri, Aug 18, 2023 at 3:17 PM Matthew Wilcox <willy@infradead.org> 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@google.com> 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@google.com> > > > Cc: Matthew Wilcox <willy@infradead.org> > > > --- > > > 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 > > >
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;
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@google.com> Cc: Matthew Wilcox <willy@infradead.org> --- mm/huge_memory.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)