> On Dec 8, 2022, at 03:25, Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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.
> */
How about adding a new helper like "folio_dissolve_compound(struct folio *folio)"?
then it may be unnecessary to add a comment.
Thanks.
>
> 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
