Re: [PATCH v5 4/6] mm: shmem: add mTHP support for anonymous shmem

[Bang Li]

Hi Bao lin,

On 2024/7/4 19:15, Baolin Wang wrote:
> > > +
> > > +    /*
> > > +     * Only allow inherit orders if the top-level value is 'force', 
> > > which
> > > +     * means non-PMD sized THP can not override 'huge' mount option 
> > > now.
> > > +     */
> > > +    if (shmem_huge == SHMEM_HUGE_FORCE)
> > > +        return READ_ONCE(huge_shmem_orders_inherit);
> >
> > I vaguely recall that we originally discussed that trying to set 
> > 'force' on the
> > top level control while any per-size controls were set to 'inherit' 
> > would be an
> > error, and trying to set 'force' on any per-size control except the 
> > PMD-size
> > would be an error?
>
> Right.
>
> > I don't really understand this logic. Shouldn't we just be looking at 
> > the
> > per-size control settings (or the top-level control as a proxy for every
> > per-size control that has 'inherit' set)?
>
> ‘force’ will apply the huge orders for anon shmem and tmpfs, so now we 
> only allow pmd-mapped THP to be forced. We should not look at per-size 
> control settings for tmpfs now (mTHP for tmpfs will be discussed in 
> future).
>
> > Then for tmpfs, which doesn't support non-PMD-sizes yet, we just 
> > always use the
> > PMD-size control for decisions.
> >
> > I'm also really struggling with the concept of shmem_is_huge() 
> > existing along
> > side shmem_allowable_huge_orders(). Surely this needs to all be 
> > refactored into
> > shmem_allowable_huge_orders()?
>
> I understood. But now they serve different purposes: shmem_is_huge() 
> will be used to check the huge orders for the top level, for *tmpfs* 
> and anon shmem; whereas shmem_allowable_huge_orders() will only be 
> used to check the per-size huge orders for anon shmem (excluding tmpfs 
> now). However, as I plan to add mTHP support for tmpfs, I think we can 
> perform some cleanups. 

Please count me in, I'd be happy to contribute to the cleanup and 
enhancement
process if I can.

Thanks,
Bang