Date:   Thu, 8 Dec 2022 13:58:20 -0800
From:   Sidhartha Kumar <sidhartha.kumar@...cle.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>,
        Matthew Wilcox <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/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,

Powered by blists - more mailing lists