[PATCH 1/2] mm/page_alloc: drop the unnecessary pfn_valid() for start pfn

[PATCH 1/2] mm/page_alloc: drop the unnecessary pfn_valid() for start pfn

[Baolin Wang]

We've already used pfn_to_online_page() for start pfn to make sure
it is online and valid, so the pfn_valid() for the start pfn is
unnecessary, drop it.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9de2a18519a1..6457b64fe562 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1512,7 +1512,7 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
 	/* end_pfn is one past the range we are checking */
 	end_pfn--;
 
-	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
+	if (!pfn_valid(end_pfn))
 		return NULL;
 
 	start_page = pfn_to_online_page(start_pfn);
-- 
2.27.0

[PATCH 2/2] mm/page_alloc: add some comments to explain the possible hole in __pageblock_pfn_to_page()

[Baolin Wang]

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, though the start pfn of a pageblock is valid, it can not always
guarantee the end pfn of the pageblock is also valid (may be holes) in some
cases. For example, if the pageblock order is MAX_ORDER - 1, 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/

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/page_alloc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6457b64fe562..dc4005b32ae0 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1502,6 +1502,14 @@ 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: if the start pfn of a pageblock is valid, but it can not always guarantee
+ * the end pfn of the pageblock is also valid (may be holes) in some cases. For
+ * example, if the pageblock order is MAX_ORDER - 1, 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 this possible issue when checking if the whole pfns are valid of a
+ * pageblock.
  */
 struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
 				     unsigned long end_pfn, struct zone *zone)
-- 
2.27.0

Re: [PATCH 1/2] mm/page_alloc: drop the unnecessary pfn_valid() for start pfn

[David Hildenbrand]

On 22.04.23 12:15, Baolin Wang wrote:
> We've already used pfn_to_online_page() for start pfn to make sure
> it is online and valid, so the pfn_valid() for the start pfn is
> unnecessary, drop it.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm/page_alloc: drop the unnecessary pfn_valid() for start pfn

[Huang, Ying]

Baolin Wang <baolin.wang@linux.alibaba.com> writes:

> We've already used pfn_to_online_page() for start pfn to make sure
> it is online and valid, so the pfn_valid() for the start pfn is
> unnecessary, drop it.
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Thanks!

Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9de2a18519a1..6457b64fe562 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1512,7 +1512,7 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>  	/* end_pfn is one past the range we are checking */
>  	end_pfn--;
>  
> -	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
> +	if (!pfn_valid(end_pfn))
>  		return NULL;
>  
>  	start_page = pfn_to_online_page(start_pfn);

Re: [PATCH 2/2] mm/page_alloc: add some comments to explain the possible hole in __pageblock_pfn_to_page()

[Huang, Ying]

Baolin Wang <baolin.wang@linux.alibaba.com> writes:

> 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, though the start pfn of a pageblock is valid, it can not always
> guarantee the end pfn of the pageblock is also valid (may be holes) in some
> cases. For example, if the pageblock order is MAX_ORDER - 1, 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/
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/page_alloc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6457b64fe562..dc4005b32ae0 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1502,6 +1502,14 @@ 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: if the start pfn of a pageblock is valid, but it can not always guarantee
> + * the end pfn of the pageblock is also valid (may be holes) in some cases. For

"valid" sounds confusing here.  pfn_valid() is true, but the pfn is
considered invalid at some degree.  How about the following?

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 - 1, 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 this possible issue when checking if the whole pfns are valid of a
                                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

whether all pfns of a pageblock are valid. ?

> + * pageblock.
>   */
>  struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>  				     unsigned long end_pfn, struct zone *zone)

My English is poor.  So, feel free to ignore the comments.

Best Regards,
Huang, Ying

Re: [PATCH 2/2] mm/page_alloc: add some comments to explain the possible hole in __pageblock_pfn_to_page()

[Baolin Wang]

On 4/23/2023 9:13 AM, Huang, Ying wrote:
> Baolin Wang <baolin.wang@linux.alibaba.com> writes:
> 
>> 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, though the start pfn of a pageblock is valid, it can not always
>> guarantee the end pfn of the pageblock is also valid (may be holes) in some
>> cases. For example, if the pageblock order is MAX_ORDER - 1, 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/
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/page_alloc.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 6457b64fe562..dc4005b32ae0 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1502,6 +1502,14 @@ 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: if the start pfn of a pageblock is valid, but it can not always guarantee
>> + * the end pfn of the pageblock is also valid (may be holes) in some cases. For
> 
> "valid" sounds confusing here.  pfn_valid() is true, but the pfn is
> considered invalid at some degree.  How about the following?
> 
> 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 - 1, 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 this possible issue when checking if the whole pfns are valid of a
>                                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> whether all pfns of a pageblock are valid. ?
> 
>> + * pageblock.
>>    */
>>   struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>>   				     unsigned long end_pfn, struct zone *zone)
> 
> My English is poor.  So, feel free to ignore the comments.

Better than me:) . Will do in next version. Thanks.

Re: [PATCH 2/2] mm/page_alloc: add some comments to explain the possible hole in __pageblock_pfn_to_page()

[Mike Rapoport]

Hi,

On Sat, Apr 22, 2023 at 06:15:18PM +0800, 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, though the start pfn of a pageblock is valid, it can not always
> guarantee the end pfn of the pageblock is also valid (may be holes) in some
> cases. For example, if the pageblock order is MAX_ORDER - 1, which will fall

Nit: in the current mm tree the default pageblock order is MAX_ORDER.

> 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/
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/page_alloc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6457b64fe562..dc4005b32ae0 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1502,6 +1502,14 @@ 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: if the start pfn of a pageblock is valid, but it can not always guarantee
> + * the end pfn of the pageblock is also valid (may be holes) in some cases. For
> + * example, if the pageblock order is MAX_ORDER - 1, 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 this possible issue when checking if the whole pfns are valid of a

careful about ...

> + * pageblock.
>   */
>  struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>  				     unsigned long end_pfn, struct zone *zone)
> -- 
> 2.27.0
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 2/2] mm/page_alloc: add some comments to explain the possible hole in __pageblock_pfn_to_page()

[Baolin Wang]

On 4/23/2023 1:19 PM, Mike Rapoport wrote:
> Hi,
> 
> On Sat, Apr 22, 2023 at 06:15:18PM +0800, 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, though the start pfn of a pageblock is valid, it can not always
>> guarantee the end pfn of the pageblock is also valid (may be holes) in some
>> cases. For example, if the pageblock order is MAX_ORDER - 1, which will fall
> 
> Nit: in the current mm tree the default pageblock order is MAX_ORDER.

Ah, yes, will change in next version.

> 
>> 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/
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/page_alloc.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 6457b64fe562..dc4005b32ae0 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1502,6 +1502,14 @@ 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: if the start pfn of a pageblock is valid, but it can not always guarantee
>> + * the end pfn of the pageblock is also valid (may be holes) in some cases. For
>> + * example, if the pageblock order is MAX_ORDER - 1, 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 this possible issue when checking if the whole pfns are valid of a
> 
> careful about ...

OK. Thanks for reviewing.