lkml.org 
[lkml]   [2023]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 2/2] mm/page_alloc: add some comments to explain the possible hole in __pageblock_pfn_to_page()
From


On 4/24/2023 8:08 PM, Michal Hocko wrote:
> On Mon 24-04-23 19:40:30, Baolin Wang wrote:
>>
>>
>> On 4/24/2023 7:34 PM, Michal Hocko wrote:
>>> On Mon 24-04-23 19:20:43, Baolin Wang wrote:
>>>>
>>>>
>>>> On 4/24/2023 5:54 PM, Michal Hocko wrote:
>>>>> On Sun 23-04-23 18:59:11, Baolin Wang wrote:
>>>>>> Now the __pageblock_pfn_to_page() is used by set_zone_contiguous(), which
>>>>>> checks whether the given zone contains holes, and uses pfn_to_online_page()
>>>>>> to validate if the start pfn is online and valid, as well as using pfn_valid()
>>>>>> to validate the end pfn.
>>>>>>
>>>>>> However, the __pageblock_pfn_to_page() function may return non-NULL even
>>>>>> if the end pfn of a pageblock is in a memory hole in some situations. For
>>>>>> example, if the pageblock order is MAX_ORDER, which will fall into 2
>>>>>> sub-sections, and the end pfn of the pageblock may be hole even though
>>>>>> the start pfn is online and valid.
>>>>>>
>>>>>> This did not break anything until now, but the zone continuous is fragile
>>>>>> in this possible scenario. So as previous discussion[1], it is better to
>>>>>> add some comments to explain this possible issue in case there are some
>>>>>> future pfn walkers that rely on this.
>>>>>>
>>>>>> [1] https://lore.kernel.org/all/87r0sdsmr6.fsf@yhuang6-desk2.ccr.corp.intel.com/
>>>>>
>>>>> Do I remember correctly you've had a specific configuration that would
>>>>> trigger this case?
>>>>
>>>> Yes, I provided an example in previous thread [2] so show the
>>>> __pageblock_pfn_to_page() is fragile in some cases.
>>>>
>>>> [2] https://lore.kernel.org/all/52dfdd2e-9c99-eac4-233e-59919a24323e@linux.alibaba.com/
>>>
>>> Please make it a part of the changelog.
>>
>> Sure.
>>
>>>>>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>>> ---
>>>>>> Changes from v1:
>>>>>> - Update the comments per Ying and Mike, thanks.
>>>>>> ---
>>>>>> mm/page_alloc.c | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>>>> index 6457b64fe562..9756d66f471c 100644
>>>>>> --- a/mm/page_alloc.c
>>>>>> +++ b/mm/page_alloc.c
>>>>>> @@ -1502,6 +1502,13 @@ void __free_pages_core(struct page *page, unsigned int order)
>>>>>> * interleaving within a single pageblock. It is therefore sufficient to check
>>>>>> * the first and last page of a pageblock and avoid checking each individual
>>>>>> * page in a pageblock.
>>>>>> + *
>>>>>> + * Note: the function may return non-NULL even if the end pfn of a pageblock
>>>>>> + * is in a memory hole in some situations. For example, if the pageblock
>>>>>> + * order is MAX_ORDER, which will fall into 2 sub-sections, and the end pfn
>>>>>> + * of the pageblock may be hole even though the start pfn is online and valid.
>>>>>> + * This did not break anything until now, but be careful about this possible
>>>>>> + * issue when checking whether all pfns of a pageblock are valid.
>>>>>
>>>>> It is not really clear what you should be doing (other than to be
>>>>> careful which is not helpful much TBH) when you encounter this
>>>>> situation. If the reality changes and this would break in the future
>>>>> what would breakage look like? What should be done about that?
>>>>
>>>> That depends on what the future pfn walkers do, which may access some hole
>>>> memory with zero-init page frame. For example, if checking the
>>>> __PageMovable() for a zero-init page frame, that will crash the system. But
>>>> I can not list all the possible cases.
>>>>
>>>> So how about below words?
>>>>
>>>> * Note: the function may return non-NULL even if the end pfn of a pageblock
>>>> * is in a memory hole in some situations. For example, if the pageblock
>>>> * order is MAX_ORDER, which will fall into 2 sub-sections, and the end pfn
>>>> * of the pageblock may be hole even though the start pfn is online and
>>>> valid.
>>>> * This did not break anything until now, but be careful about this possible
>>>> * issue when checking whether all pfns of a pageblock are valid, that may
>>>> * lead to accessing empty page frame, and the worst case can crash the
>>>> system.
>>>> * So you should use pfn_to_onlie_page() instead of pfn_valid() to valid the
>>>> * pfns in a pageblock if such case happens.
>>>
>>> Does that mean that struct page is not initialized and PagePoisoned will
>>> trigger or it is just zero-prefilled?
>>
>> In the example I provided[2], these page frames of the hole memory are
>> zero-prefilled.
>
> OK, so make _that_ explicit in the comment. Essentially you want to say
> that there are cases where we have zero-initialized struct pages for
> memory holes. In general no pfn walker should touch a physical memory
> range for pfn where the struct page doesn't contain any metadata it
> recognizes. Zero fill struct pages do not contain any distinguishable
> state so that makes it less of a problem.
>
> All that being said I would reformulate the comment as follows:
>
> * Note: the function may return non-NULL struct page even for a
> * page block which contains a memory hole (i.e. there is no
> * physical memory for a subset of the pfn range). This should be
> * safe most of the time because struct pages are still zero
> * pre-filled and pfn walkers shouldn't touch any physical memory
> * range for which they do not recognize any specific metadata in
> * struct pages.

Thanks. That makes sense to me. A trivial thing is I still want to add
the example in the comments to make it clear. Are you okay with below
description?

+ * Note: the function may return non-NULL struct page even for a page block
+ * which contains a memory hole (i.e. there is no physical memory for a
subset
+ * of the pfn range). For example, if the pageblock order is MAX_ORDER,
which
+ * will fall into 2 sub-sections, and the end pfn of the pageblock may
be hole
+ * even though the start pfn is online and valid. This should be safe
most of
+ * the time because struct pages are still zero pre-filled and pfn walkers
+ * shouldn't touch any physical memory range for which they do not
recognize
+ * any specific metadata in struct pages.

\
 
 \ /
  Last update: 2023-04-24 14:49    [W:0.064 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site