Re: [PATCH mm-unstable v5 01/10] mm: add folio dtor and order setter functions

[Mike Kravetz <mike.kravetz@oracle.com>]

On 12/07/22 11:05, Sidhartha Kumar wrote:
> On 12/7/22 10:49 AM, Sidhartha Kumar wrote:
> > On 12/7/22 10:12 AM, Mike Kravetz wrote:
> > > On 12/07/22 12:11, Muchun Song wrote:
> > > > > On Dec 7, 2022, at 11:42, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> > > > > On 12/07/22 11:34, Muchun Song wrote:
> > > > 
> > > > Agree. It has confused me a lot. I suggest changing the code to the
> > > > followings. The folio_test_large() check is still to avoid unexpected
> > > > users for OOB.
> > > > 
> > > > static inline void folio_set_compound_order(struct folio *folio,
> > > >                         unsigned int order)
> > > > {
> > > >     VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
> > > >     // or
> > > >     // if (!folio_test_large(folio))
> > > >     //     return;
> > > > 
> > > >     folio->_folio_order = order;
> > > > #ifdef CONFIG_64BIT
> > > >     folio->_folio_nr_pages = order ? 1U << order : 0;
> > > > #endif
> > > > }
> > > 
> > > I think the VM_BUG_ON_FOLIO is appropriate as it would at least flag
> > > data corruption.
> > > 
> > As Mike pointed out, my intention with supporting the 0 case was to
> > cleanup the __destroy_compound_gigantic_page code by moving the ifdef
> > CONFIG_64BIT lines to folio_set_compound_order(). I'll add the
> > VM_BUG_ON_FOLIO line as well as a comment to make it clear it is not
> > normally supported.
> > 
> > > Thinking about this some more, it seems that hugetlb is the only caller
> > > that abuses folio_set_compound_order (and previously set_compound_order)
> > > by passing in a zero order.  Since it is unlikely that anyone knows of
> > > this abuse, it might be good to add a comment to the routine to note
> > > why it handles the zero case.  This might help prevent changes which
> > > would potentially break hugetlb.
> > 
> > +/*
> > + * _folio_nr_pages and _folio_order are invalid for
> > + * order-zero pages. An exception is hugetlb, which passes
> > + * in a zero order in __destroy_compound_gigantic_page().
> > + */
> >   static inline void folio_set_compound_order(struct folio *folio,
> >                  unsigned int order)
> >   {
> > +       VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
> > +
> >          folio->_folio_order = order;
> >   #ifdef CONFIG_64BIT
> >          folio->_folio_nr_pages = order ? 1U << order : 0;
> > 
> > Does this comment work?
> > 
> > 
> 
> I will change the comment from referencing
> __destory_compound_gigantic_page()
> to __destroy_compound_gigantic_folio, although
> __prep_compound_gigantic_folio() is another user of
> folio_set_compound_order(folio, 0). Should the sentence just be "An
> exception is hugetlb, which passes in a zero order"?

How about a comment like this?

/*
 * folio_set_compound_order is generally passed a non-zero order to
 * set up/create a large folio.  However, hugetlb code abuses this by
 * passing in zero when 'dissolving' a large folio.
 */

My only concern is that someone may modify the routine such that it no
longer works when passed zero order.  It is not likely as anyone should
notice the special case for zero, and look for callers.
-- 
Mike Kravetz