Message-ID: <Y5I7z3Kz4Bur4e6f@monkey>
Date:   Thu, 8 Dec 2022 11:32:31 -0800
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Sidhartha Kumar <sidhartha.kumar@...cle.com>, willy@...radead.org
Cc:     John Hubbard <jhubbard@...dia.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, akpm@...ux-foundation.org,
        songmuchun@...edance.com, tsahu@...ux.ibm.com, david@...hat.com
Subject: Re: [PATCH mm-unstable] mm: clarify folio_set_compound_order() zero
 support

On 12/08/22 10:06, Sidhartha Kumar wrote:
> On 12/7/22 6:27 PM, John Hubbard wrote:
> > On 12/7/22 17:42, Sidhartha Kumar wrote:
> > > > Wouldn't it be better to instead just create a new function for that
> > > > case, such as:
> > > > 
> > > >      dissolve_large_folio()
> > > > 
> > > 
> > > Prior to the folio conversion, the helper function
> > > __destroy_compound_gigantic_page() did:
> > > 
> > >      set_compound_order(page, 0);
> > > #ifdef CONFIG_64BIT
> > >      page[1].compound_nr = 0;
> > > #endif
> > > 
> > > as part of dissolving the page. My goal for this patch was to create
> > > a function that would encapsulate that segment of code with a single
> > > call of folio_set_compound_order(folio, 0). set_compound_order()
> > > does not set compound_nr to 0 when 0 is passed in to the order
> > > argument so explicitly setting it is required. I don't think a
> > > separate dissolve_large_folio() function for the hugetlb case is
> > > needed as __destroy_compound_gigantic_folio() is pretty concise as
> > > it is.
> > > 
> > 
> > Instead of "this is abusing function X()" comments, we should prefer
> > well-named functions that do something understandable. And you can get
> > that by noticing that folio_set_compound_order() collapses down to
> > nearly nothing in the special "order 0" case. So just inline that code
> > directly into __destroy_compound_gigantic_folio(), taking a moment to
> > fill in and consolidate the CONFIG_64BIT missing parts in mm.h.
> > 
> > And now you can get rid of this cruft and "abuse" comment, and instead
> > just end up with two simple lines of code that are crystal clear--as
> > they should be, in a "__destroy" function. Like this:
> > 
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 105878936485..cf227ed00945 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1754,6 +1754,7 @@ static inline void set_page_links(struct page
> > *page, enum zone_type zone,
> >   #endif
> >   }
> > 
> > +#ifdef CONFIG_64BIT
> >   /**
> >    * folio_nr_pages - The number of pages in the folio.
> >    * @folio: The folio.
> > @@ -1764,13 +1765,32 @@ static inline long folio_nr_pages(struct folio
> > *folio)
> >   {
> >       if (!folio_test_large(folio))
> >           return 1;
> > -#ifdef CONFIG_64BIT
> >       return folio->_folio_nr_pages;
> > +}
> > +
> > +static inline void folio_set_nr_pages(struct folio *folio, long nr_pages)
> > +{
> > +    folio->_folio_nr_pages = nr_pages;
> > +}
> >   #else
> > +/**
> > + * folio_nr_pages - The number of pages in the folio.
> > + * @folio: The folio.
> > + *
> > + * Return: A positive power of two.
> > + */
> > +static inline long folio_nr_pages(struct folio *folio)
> > +{
> > +    if (!folio_test_large(folio))
> > +        return 1;
> >       return 1L << folio->_folio_order;
> > -#endif
> >   }
> > 
> > +static inline void folio_set_nr_pages(struct folio *folio, long nr_pages)
> > +{
> > +}
> > +#endif
> > +
> >   /**
> >    * folio_next - Move to the next physical folio.
> >    * @folio: The folio we're currently operating on.
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index e3500c087893..b507a98063e6 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1344,7 +1344,8 @@ static void
> > __destroy_compound_gigantic_folio(struct folio *folio,
> >               set_page_refcounted(p);
> >       }
> > 
> > -    folio_set_compound_order(folio, 0);
> > +    folio->_folio_order = 0;
> > +    folio_set_nr_pages(folio, 0);
> >       __folio_clear_head(folio);
> >   }
> > 
> >
> > Yes?
> 
> 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.

My opinion ... which is often wrong :)

I agree 100% with John about these being clear and understandable routines,
as well as Muchun's comment about the need for an interface for setting
_folio_order instead of accessing directly.

However, I do have some concern about two independent interfaces for setting
_folio_order and _folio_nr_pages.  These two fields really do need to be "in
sync" and having two interfaces to modify independently may lead to errors.

Any thoughts Matthew?  You introduced folios as well as the somewhat
redundant compound_nr/_folio_nr_pages fields.

Here is another thought.  Since this discussion started with a question
about "Can/should you really pass a zero order to folio_set_compound_order?",
what about a separate "folio_clear_order" interface?

We would then have:
folio_set_order()	/* passed non-zero order when creating large folio */
folio_clear_order()	/* zero order is implied */

Note that I did drop 'compound' from the names.  The more I think about the
folio direction we really should not use compound in the same.

Both of these could wrap a renamed version of Sidhartha's routine
folio_set_compound_order().  In this way the two fields are automatically
kept in sync.

Again, just my thoughts/opinion.
-- 
Mike Kravetz

Powered by blists - more mailing lists