Re: [PATCH] mm: add more readable thp_vma_allowable_order_foo()

[David Hildenbrand <david@xxxxxxxxxx>]

On 24.04.24 16:05, Ryan Roberts wrote:
> On 24/04/2024 15:07, Kefeng Wang wrote:
> > There are too many bool arguments in thp_vma_allowable_orders(), adding
> > some more readable thp_vma_allowable_order_foo(),
> >
> >    thp_vma_allowable_orders_insmaps() is used in samps
> >    thp_vma_allowable_order[s]_inpf()  is used in page fault
> >    thp_vma_allowable_pmd_order_inhuge is used in khugepaged scan and madvise
> >
> > Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
>
> Just one nit below. With that addressed:
>
> Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>
>
> > ---
> >   fs/proc/task_mmu.c      |  3 +--
> >   include/linux/huge_mm.h | 14 ++++++++++++--
> >   mm/khugepaged.c         | 20 ++++++++------------
> >   mm/memory.c             |  8 ++++----
> >   4 files changed, 25 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index f4259b7edfde..1136aa97f143 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -871,8 +871,7 @@ static int show_smap(struct seq_file *m, void *v)
> >   	__show_smap(m, &mss, false);
> >   
> >   	seq_printf(m, "THPeligible:    %8u\n",
> > -		   !!thp_vma_allowable_orders(vma, vma->vm_flags, true, false,
> > -					      true, THP_ORDERS_ALL));
> > +		   thp_vma_allowable_orders_insmaps(vma, vma->vm_flags));
> >   
> >   	if (arch_pkeys_enabled())
> >   		seq_printf(m, "ProtectionKey:  %8u\n", vma_pkey(vma));
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 56c7ea73090b..345cf394480b 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -83,8 +83,18 @@ extern struct kobj_attribute shmem_enabled_attr;
> >    */
> >   #define THP_ORDERS_ALL		(THP_ORDERS_ALL_ANON | THP_ORDERS_ALL_FILE)
> >   
> > -#define thp_vma_allowable_order(vma, vm_flags, smaps, in_pf, enforce_sysfs, order) \
> > -	(!!thp_vma_allowable_orders(vma, vm_flags, smaps, in_pf, enforce_sysfs, BIT(order)))
> > +#define thp_vma_allowable_orders_insmaps(vma, vm_flags) \
> > +	(!!thp_vma_allowable_orders(vma, vm_flags, true, false, true, THP_ORDERS_ALL))
> > +
> > +#define thp_vma_allowable_orders_inpf(vma, vm_flags, orders) \
> > +	(!!thp_vma_allowable_orders(vma, vm_flags, false, true, true, orders))
> > +
> > +#define thp_vma_allowable_order_inpf(vma, vm_flags, order) \
> > +	(!!thp_vma_allowable_orders_inpf(vma, vm_flags, BIT(order)))
> > +
> > +#define thp_vma_allowable_pmd_order_inhuge(vma, vm_flags, enforce_sysfs) \
> > +	(!!thp_vma_allowable_orders(vma, vm_flags, false, false, enforce_sysfs, BIT(PMD_ORDER)))
>
> nit: Personally I'd leave the order as an argument rather than encoding it in
> the name. It's likely that khugepaged will grow support for non-PMD-size

Agreed.

> collapse in future. The first part of the name "thp_vma_allowable_order" is then
> consistent and easy to search for all variants. And perhaps "inkhuge" is more
> precise?

"_khugepaged" or something else that people can actually parse and 
understand.

--
Cheers,

David / dhildenb