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

[Sidhartha Kumar]

On 12/8/22 2:14 PM, John Hubbard wrote:
> On 12/8/22 14:12, Sidhartha Kumar wrote:
> > On 12/8/22 2:01 PM, John Hubbard wrote:
> > > On 12/8/22 13:58, Sidhartha Kumar wrote:
> > > > 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);
> > >
> > > Sounds good, except for this part: why is a function named
> > > folio_set_order() BUG-ing on a non-large folio? The naming
> > > is still wrong, perhaps?
> >
> > This is because the _folio_nr_pages and _folio_order fields are part 
> > of the first tail page in the folio. folio_test_large returns if the 
> > folio is larger than one page which would be required for setting the 
> > fields.
>
> OK, but then as I said, the name is wrong. One can either:
>
> a) handle the non-large case, or
>
> b) rename the function to indicate that it only works on large folios.

Discussed here[1], the BUG_ON line seemed more appropriate over
 	
if (!folio_test_large(folio))
	return;

as the misuse would not be silent. I think I would be against renaming 
the function as I don't see any large folio specific function names for 
other accessors of tail page fields. Would both the BUG_ON and return on 
non-large folio be included then?


[1]: 
https://lore.kernel.org/linux-mm/20221129225039.82257-1-sidhartha.kumar@xxxxxxxxxx/T/#m98cf80bb21ae533b7385f2e363c602e2c9e2802d
> thanks,