| Message ID | 16dad33282617ecb90a3f466b35273b10be68d21.1720755678.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some cleanups for shmem | expand |
On 13/07/2024 14:24, Baolin Wang wrote: > Move shmem_huge_global_enabled() into the shmem_allowable_huge_orders() function, > so that shmem_allowable_huge_orders() can also help to find the allowable huge > orders for tmpfs. Moreover the shmem_huge_global_enabled() can become static. > > No functional changes. > > Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> one nit below, but either way: Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> > --- > include/linux/shmem_fs.h | 12 ++---------- > mm/huge_memory.c | 12 +++--------- > mm/shmem.c | 41 ++++++++++++++++++++++++++-------------- > 3 files changed, 32 insertions(+), 33 deletions(-) > > diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h > index 405ee8d3589a..1564d7d3ca61 100644 > --- a/include/linux/shmem_fs.h > +++ b/include/linux/shmem_fs.h > @@ -111,21 +111,13 @@ extern void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end); > int shmem_unuse(unsigned int type); > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > -extern bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, bool shmem_huge_force, > - struct mm_struct *mm, unsigned long vm_flags); > unsigned long shmem_allowable_huge_orders(struct inode *inode, > struct vm_area_struct *vma, pgoff_t index, > - bool global_huge); > + bool shmem_huge_force); > #else > -static __always_inline bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, > - bool shmem_huge_force, struct mm_struct *mm, > - unsigned long vm_flags) > -{ > - return false; > -} > static inline unsigned long shmem_allowable_huge_orders(struct inode *inode, > struct vm_area_struct *vma, pgoff_t index, > - bool global_huge) > + bool shmem_huge_force) > { > return 0; > } > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index cc9bad12be75..f69980b5b5fc 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -151,16 +151,10 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, > * Must be done before hugepage flags check since shmem has its > * own flags. > */ > - if (!in_pf && shmem_file(vma->vm_file)) { > - bool global_huge = shmem_huge_global_enabled(file_inode(vma->vm_file), > - vma->vm_pgoff, !enforce_sysfs, > - vma->vm_mm, vm_flags); > - > - if (!vma_is_anon_shmem(vma)) > - return global_huge ? orders : 0; > + if (!in_pf && shmem_file(vma->vm_file)) > return shmem_allowable_huge_orders(file_inode(vma->vm_file), > - vma, vma->vm_pgoff, global_huge); > - } > + vma, vma->vm_pgoff, > + !enforce_sysfs); > > if (!vma_is_anonymous(vma)) { > /* > diff --git a/mm/shmem.c b/mm/shmem.c > index 1445dcd39b6f..4d274f5a17d9 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -581,7 +581,7 @@ static bool __shmem_huge_global_enabled(struct inode *inode, pgoff_t index, > } > } > > -bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, > +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, > bool shmem_huge_force, struct mm_struct *mm, > unsigned long vm_flags) > { > @@ -772,6 +772,13 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo, > { > return 0; > } > + > +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, > + bool shmem_huge_force, struct mm_struct *mm, > + unsigned long vm_flags) > +{ > + return false; > +} > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > /* > @@ -1625,27 +1632,39 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp) > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > unsigned long shmem_allowable_huge_orders(struct inode *inode, > struct vm_area_struct *vma, pgoff_t index, > - bool global_huge) > + bool shmem_huge_force) > { > unsigned long mask = READ_ONCE(huge_shmem_orders_always); > unsigned long within_size_orders = READ_ONCE(huge_shmem_orders_within_size); > - unsigned long vm_flags = vma->vm_flags; > + unsigned long vm_flags = vma ? vma->vm_flags : 0; > + struct mm_struct *fault_mm = vma ? vma->vm_mm : NULL; nit: rather than deriving the fault_mm here, I wonder if its cleaner to just pass vma to shmem_huge_global_enabled()? shmem_huge_global_enabled() is just using it as a guard to access vm_flags, which you can just as easily do by testing the vma for non-NULL. And you can access mm flags with vma->vm_mm->flags after testing the vma too. > /* > * Check all the (large) orders below HPAGE_PMD_ORDER + 1 that > * are enabled for this vma. > */ > unsigned long orders = BIT(PMD_ORDER + 1) - 1; > + bool global_huge; > loff_t i_size; > int order; > > - if ((vm_flags & VM_NOHUGEPAGE) || > - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) > + if (vma && ((vm_flags & VM_NOHUGEPAGE) || > + test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))) > return 0; > > /* If the hardware/firmware marked hugepage support disabled. */ > if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_UNSUPPORTED)) > return 0; > > + global_huge = shmem_huge_global_enabled(inode, index, shmem_huge_force, > + fault_mm, vm_flags); > + if (!vma || !vma_is_anon_shmem(vma)) { > + /* > + * For tmpfs, we now only support PMD sized THP if huge page > + * is enabled, otherwise fallback to order 0. > + */ > + return global_huge ? BIT(HPAGE_PMD_ORDER) : 0; > + } > + > /* > * Following the 'deny' semantics of the top level, force the huge > * option off from all mounts. > @@ -2081,7 +2100,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, > struct mm_struct *fault_mm; > struct folio *folio; > int error; > - bool alloced, huge; > + bool alloced; > unsigned long orders = 0; > > if (WARN_ON_ONCE(!shmem_mapping(inode->i_mapping))) > @@ -2154,14 +2173,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, > return 0; > } > > - huge = shmem_huge_global_enabled(inode, index, false, fault_mm, > - vma ? vma->vm_flags : 0); > - /* Find hugepage orders that are allowed for anonymous shmem. */ > - if (vma && vma_is_anon_shmem(vma)) > - orders = shmem_allowable_huge_orders(inode, vma, index, huge); > - else if (huge) > - orders = BIT(HPAGE_PMD_ORDER); > - > + /* Find hugepage orders that are allowed for anonymous shmem and tmpfs. */ > + orders = shmem_allowable_huge_orders(inode, vma, index, false); > if (orders > 0) { > gfp_t huge_gfp; >
(Sorry for the late reply due to my vacation.) On 2024/7/15 21:36, Ryan Roberts wrote: > On 13/07/2024 14:24, Baolin Wang wrote: >> Move shmem_huge_global_enabled() into the shmem_allowable_huge_orders() function, >> so that shmem_allowable_huge_orders() can also help to find the allowable huge >> orders for tmpfs. Moreover the shmem_huge_global_enabled() can become static. >> >> No functional changes. >> >> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> > > one nit below, but either way: > > Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> > >> --- >> include/linux/shmem_fs.h | 12 ++---------- >> mm/huge_memory.c | 12 +++--------- >> mm/shmem.c | 41 ++++++++++++++++++++++++++-------------- >> 3 files changed, 32 insertions(+), 33 deletions(-) >> >> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h >> index 405ee8d3589a..1564d7d3ca61 100644 >> --- a/include/linux/shmem_fs.h >> +++ b/include/linux/shmem_fs.h >> @@ -111,21 +111,13 @@ extern void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end); >> int shmem_unuse(unsigned int type); >> >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> -extern bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, bool shmem_huge_force, >> - struct mm_struct *mm, unsigned long vm_flags); >> unsigned long shmem_allowable_huge_orders(struct inode *inode, >> struct vm_area_struct *vma, pgoff_t index, >> - bool global_huge); >> + bool shmem_huge_force); >> #else >> -static __always_inline bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, >> - bool shmem_huge_force, struct mm_struct *mm, >> - unsigned long vm_flags) >> -{ >> - return false; >> -} >> static inline unsigned long shmem_allowable_huge_orders(struct inode *inode, >> struct vm_area_struct *vma, pgoff_t index, >> - bool global_huge) >> + bool shmem_huge_force) >> { >> return 0; >> } >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index cc9bad12be75..f69980b5b5fc 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -151,16 +151,10 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, >> * Must be done before hugepage flags check since shmem has its >> * own flags. >> */ >> - if (!in_pf && shmem_file(vma->vm_file)) { >> - bool global_huge = shmem_huge_global_enabled(file_inode(vma->vm_file), >> - vma->vm_pgoff, !enforce_sysfs, >> - vma->vm_mm, vm_flags); >> - >> - if (!vma_is_anon_shmem(vma)) >> - return global_huge ? orders : 0; >> + if (!in_pf && shmem_file(vma->vm_file)) >> return shmem_allowable_huge_orders(file_inode(vma->vm_file), >> - vma, vma->vm_pgoff, global_huge); >> - } >> + vma, vma->vm_pgoff, >> + !enforce_sysfs); >> >> if (!vma_is_anonymous(vma)) { >> /* >> diff --git a/mm/shmem.c b/mm/shmem.c >> index 1445dcd39b6f..4d274f5a17d9 100644 >> --- a/mm/shmem.c >> +++ b/mm/shmem.c >> @@ -581,7 +581,7 @@ static bool __shmem_huge_global_enabled(struct inode *inode, pgoff_t index, >> } >> } >> >> -bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, >> +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, >> bool shmem_huge_force, struct mm_struct *mm, >> unsigned long vm_flags) >> { >> @@ -772,6 +772,13 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo, >> { >> return 0; >> } >> + >> +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, >> + bool shmem_huge_force, struct mm_struct *mm, >> + unsigned long vm_flags) >> +{ >> + return false; >> +} >> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ >> >> /* >> @@ -1625,27 +1632,39 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp) >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> unsigned long shmem_allowable_huge_orders(struct inode *inode, >> struct vm_area_struct *vma, pgoff_t index, >> - bool global_huge) >> + bool shmem_huge_force) >> { >> unsigned long mask = READ_ONCE(huge_shmem_orders_always); >> unsigned long within_size_orders = READ_ONCE(huge_shmem_orders_within_size); >> - unsigned long vm_flags = vma->vm_flags; >> + unsigned long vm_flags = vma ? vma->vm_flags : 0; >> + struct mm_struct *fault_mm = vma ? vma->vm_mm : NULL; > > nit: rather than deriving the fault_mm here, I wonder if its cleaner to just > pass vma to shmem_huge_global_enabled()? shmem_huge_global_enabled() is just > using it as a guard to access vm_flags, which you can just as easily do by > testing the vma for non-NULL. And you can access mm flags with vma->vm_mm->flags > after testing the vma too. Make sense to me, and will do in next version. Thanks for reviewing.
>>> /* >>> @@ -1625,27 +1632,39 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp) >>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >>> unsigned long shmem_allowable_huge_orders(struct inode *inode, >>> struct vm_area_struct *vma, pgoff_t index, >>> - bool global_huge) >>> + bool shmem_huge_force) >>> { >>> unsigned long mask = READ_ONCE(huge_shmem_orders_always); >>> unsigned long within_size_orders = READ_ONCE(huge_shmem_orders_within_size); >>> - unsigned long vm_flags = vma->vm_flags; >>> + unsigned long vm_flags = vma ? vma->vm_flags : 0; >>> + struct mm_struct *fault_mm = vma ? vma->vm_mm : NULL; >> >> nit: rather than deriving the fault_mm here, I wonder if its cleaner to just >> pass vma to shmem_huge_global_enabled()? shmem_huge_global_enabled() is just >> using it as a guard to access vm_flags, which you can just as easily do by >> testing the vma for non-NULL. And you can access mm flags with vma->vm_mm->flags >> after testing the vma too. > > Make sense to me, and will do in next version. Feel free to add my Acked-by: David Hildenbrand <david@redhat.com>
On 2024/7/25 21:09, David Hildenbrand wrote: >>>> /* >>>> @@ -1625,27 +1632,39 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, >>>> gfp_t limit_gfp) >>>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >>>> unsigned long shmem_allowable_huge_orders(struct inode *inode, >>>> struct vm_area_struct *vma, pgoff_t index, >>>> - bool global_huge) >>>> + bool shmem_huge_force) >>>> { >>>> unsigned long mask = READ_ONCE(huge_shmem_orders_always); >>>> unsigned long within_size_orders = >>>> READ_ONCE(huge_shmem_orders_within_size); >>>> - unsigned long vm_flags = vma->vm_flags; >>>> + unsigned long vm_flags = vma ? vma->vm_flags : 0; >>>> + struct mm_struct *fault_mm = vma ? vma->vm_mm : NULL; >>> >>> nit: rather than deriving the fault_mm here, I wonder if its cleaner >>> to just >>> pass vma to shmem_huge_global_enabled()? shmem_huge_global_enabled() >>> is just >>> using it as a guard to access vm_flags, which you can just as easily >>> do by >>> testing the vma for non-NULL. And you can access mm flags with >>> vma->vm_mm->flags >>> after testing the vma too. >> >> Make sense to me, and will do in next version. > > Feel free to add my > > Acked-by: David Hildenbrand <david@redhat.com> Thanks David. Andrew has already queued my v3 patchset into the mm-unstable branch. Andrew, please help to add David's acked tag for v3 series. Thanks.
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index 405ee8d3589a..1564d7d3ca61 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -111,21 +111,13 @@ extern void shmem_truncate_range(struct inode *inode, loff_t start, loff_t end); int shmem_unuse(unsigned int type); #ifdef CONFIG_TRANSPARENT_HUGEPAGE -extern bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, bool shmem_huge_force, - struct mm_struct *mm, unsigned long vm_flags); unsigned long shmem_allowable_huge_orders(struct inode *inode, struct vm_area_struct *vma, pgoff_t index, - bool global_huge); + bool shmem_huge_force); #else -static __always_inline bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, - bool shmem_huge_force, struct mm_struct *mm, - unsigned long vm_flags) -{ - return false; -} static inline unsigned long shmem_allowable_huge_orders(struct inode *inode, struct vm_area_struct *vma, pgoff_t index, - bool global_huge) + bool shmem_huge_force) { return 0; } diff --git a/mm/huge_memory.c b/mm/huge_memory.c index cc9bad12be75..f69980b5b5fc 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -151,16 +151,10 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma, * Must be done before hugepage flags check since shmem has its * own flags. */ - if (!in_pf && shmem_file(vma->vm_file)) { - bool global_huge = shmem_huge_global_enabled(file_inode(vma->vm_file), - vma->vm_pgoff, !enforce_sysfs, - vma->vm_mm, vm_flags); - - if (!vma_is_anon_shmem(vma)) - return global_huge ? orders : 0; + if (!in_pf && shmem_file(vma->vm_file)) return shmem_allowable_huge_orders(file_inode(vma->vm_file), - vma, vma->vm_pgoff, global_huge); - } + vma, vma->vm_pgoff, + !enforce_sysfs); if (!vma_is_anonymous(vma)) { /* diff --git a/mm/shmem.c b/mm/shmem.c index 1445dcd39b6f..4d274f5a17d9 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -581,7 +581,7 @@ static bool __shmem_huge_global_enabled(struct inode *inode, pgoff_t index, } } -bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, bool shmem_huge_force, struct mm_struct *mm, unsigned long vm_flags) { @@ -772,6 +772,13 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo, { return 0; } + +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t index, + bool shmem_huge_force, struct mm_struct *mm, + unsigned long vm_flags) +{ + return false; +} #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ /* @@ -1625,27 +1632,39 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp) #ifdef CONFIG_TRANSPARENT_HUGEPAGE unsigned long shmem_allowable_huge_orders(struct inode *inode, struct vm_area_struct *vma, pgoff_t index, - bool global_huge) + bool shmem_huge_force) { unsigned long mask = READ_ONCE(huge_shmem_orders_always); unsigned long within_size_orders = READ_ONCE(huge_shmem_orders_within_size); - unsigned long vm_flags = vma->vm_flags; + unsigned long vm_flags = vma ? vma->vm_flags : 0; + struct mm_struct *fault_mm = vma ? vma->vm_mm : NULL; /* * Check all the (large) orders below HPAGE_PMD_ORDER + 1 that * are enabled for this vma. */ unsigned long orders = BIT(PMD_ORDER + 1) - 1; + bool global_huge; loff_t i_size; int order; - if ((vm_flags & VM_NOHUGEPAGE) || - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) + if (vma && ((vm_flags & VM_NOHUGEPAGE) || + test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))) return 0; /* If the hardware/firmware marked hugepage support disabled. */ if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_UNSUPPORTED)) return 0; + global_huge = shmem_huge_global_enabled(inode, index, shmem_huge_force, + fault_mm, vm_flags); + if (!vma || !vma_is_anon_shmem(vma)) { + /* + * For tmpfs, we now only support PMD sized THP if huge page + * is enabled, otherwise fallback to order 0. + */ + return global_huge ? BIT(HPAGE_PMD_ORDER) : 0; + } + /* * Following the 'deny' semantics of the top level, force the huge * option off from all mounts. @@ -2081,7 +2100,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, struct mm_struct *fault_mm; struct folio *folio; int error; - bool alloced, huge; + bool alloced; unsigned long orders = 0; if (WARN_ON_ONCE(!shmem_mapping(inode->i_mapping))) @@ -2154,14 +2173,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, return 0; } - huge = shmem_huge_global_enabled(inode, index, false, fault_mm, - vma ? vma->vm_flags : 0); - /* Find hugepage orders that are allowed for anonymous shmem. */ - if (vma && vma_is_anon_shmem(vma)) - orders = shmem_allowable_huge_orders(inode, vma, index, huge); - else if (huge) - orders = BIT(HPAGE_PMD_ORDER); - + /* Find hugepage orders that are allowed for anonymous shmem and tmpfs. */ + orders = shmem_allowable_huge_orders(inode, vma, index, false); if (orders > 0) { gfp_t huge_gfp;
Move shmem_huge_global_enabled() into the shmem_allowable_huge_orders() function, so that shmem_allowable_huge_orders() can also help to find the allowable huge orders for tmpfs. Moreover the shmem_huge_global_enabled() can become static. No functional changes. Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- include/linux/shmem_fs.h | 12 ++---------- mm/huge_memory.c | 12 +++--------- mm/shmem.c | 41 ++++++++++++++++++++++++++-------------- 3 files changed, 32 insertions(+), 33 deletions(-)