Re: [PATCH mm-unstable] mm: clarify folio_set_compound_order() zero support

[Sidhartha Kumar]

On 12/8/22 12:01 PM, Mike Kravetz wrote:
> On 12/08/22 19:33, Matthew Wilcox wrote:
> > On Thu, Dec 08, 2022 at 10:06:07AM -0800, Sidhartha Kumar wrote:
> > > On 12/7/22 6:27 PM, John Hubbard wrote:
> > > > On 12/7/22 17:42, Sidhartha Kumar wrote:
> > > This works for me, I will take this approach along with Muchun's feedback
> > > about a wrapper function so as not to touch _folio_order directly and send
> > > out a new version.
> > >
> > > One question I have is if I should then get rid of
> > > folio_set_compound_order() as hugetlb is the only compound page user I've
> > > converted to folios so far and its use can be replaced by the suggested
> > > folio_set_nr_pages() and folio_set_order().
> > >
> > > Hugetlb also has one has one call to folio_set_compound_order() with a
> > > non-zero order, should I replace this with a call to folio_set_order() and
> > > folio_set_nr_pages() as well, or keep folio_set_compound_order() and remove
> > > zero order support and the comment. Please let me know which approach you
> > > would prefer.
> >
> > None of the above!
> >
> > Whatever we're calling this function *it does not belong* in mm.h.
> > Anything outside the MM calling it is going to be a disaster -- can you
> > imagine what will happen if a filesystem or device driver is handed a
> > folio and decides "Oh, I'll just change the size of this folio"?  It is
> > an attractive nuisance and should be confined to mm/internal.h *at best*.
>
> I suspect it was placed in mm.h as it is the 'folio version' of
> set_compound_order which resides in mm.h.  But, no need to repeat that
> unfortunate placement.
>
> > Equally, we *must not have* separate folio_set_order() and
> > folio_set_nr_pages().  These are the same thing!  They must be kept
> > in sync!  If we are to have a folio_set_order() instead of open-coding
> > it, then it should also update nr_pages.
>
> Ok.  Agree.
>
> > So, given that this is now an internal-to-mm, if not internal-to-hugetlb
> > function, I see no reason that it should not handle the case of 0.
> > I haven't studied what hugetlb_dissolve does, or why it can't use the
> > standard split_folio(), but I'm sure there's a good reason.
>
> The hugetlb code is changing the compound page/folio it created from a set of
> individual pages back to individual pages so they can be returned to the
> low level allocator.  Somewhat like what page_alloc/page_free do.  split_folio
> is overkill.  split_page would be a closer match.
>
> It makes perfect sense to put the function in mm internal.h.

Thanks John, Mike, Matthew, and Muchun for the feedback.

To summarize this discussion and outline the next version of this patch, 
the changes I'll make include:

1) change the name of folio_set_compound_order() to folio_set_order()
2) change the placement of this function from mm.h to mm/internal.h
3) folio_set_order() will set both _folio_order and _folio_nr_pages and 
handle the zero order case correctly.
4) remove the comment about hugetlb's specific use for zero orders
5) improve the style of folio_set_order() by removing ifdefs from inside 
the function to doing

#ifdef CONFIG_64BIT
 static inline void folio_set_order(struct folio *folio,
                 unsigned int order)
 {
	 VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);

	 folio->_folio_order = order;
         folio->_folio_nr_pages = order ? 1U << order : 0;
}
#else
static inline void folio_set_order(struct folio *folio,
                 unsigned int order)
 {
	 VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);

	 folio->_folio_order = order;
}
#endif

Please let me know if I missing something.
Thanks,
Sidhartha Kumar
> Thanks,