linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
@ 2022-12-13 21:20 Sidhartha Kumar
  2022-12-14  8:43 ` John Hubbard
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Sidhartha Kumar @ 2022-12-13 21:20 UTC (permalink / raw)
  To: linux-kernel, linux-mm
  Cc: akpm, songmuchun, mike.kravetz, willy, jhubbard, Sidhartha Kumar

folio_set_compound_order() is moved to an mm-internal location so external
folio users cannot misuse this function. Change the name of the function
to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
handle the case if a non-large folio is passed and add clarifying comments
to the function.

Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
Suggested-by: Muchun Song <songmuchun@bytedance.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Suggested-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/mm.h | 16 ----------------
 mm/hugetlb.c       |  6 +++---
 mm/internal.h      | 21 +++++++++++++++++++++
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7dc376052d40..300d92d2b49d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1019,22 +1019,6 @@ static inline void set_compound_order(struct page *page, unsigned int order)
 #endif
 }
 
-/*
- * folio_set_compound_order is generally passed a non-zero order to
- * initialize a large folio.  However, hugetlb code abuses this by
- * passing in zero when 'dissolving' a large folio.
- */
-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;
-#endif
-}
-
 /* Returns the number of pages in this potentially compound page. */
 static inline unsigned long compound_nr(struct page *page)
 {
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7cdbcc22587b..810e840bb4f1 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1344,7 +1344,7 @@ static void __destroy_compound_gigantic_folio(struct folio *folio,
 			set_page_refcounted(p);
 	}
 
-	folio_set_compound_order(folio, 0);
+	folio_set_order(folio, 0);
 	__folio_clear_head(folio);
 }
 
@@ -1808,7 +1808,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
 	__folio_clear_reserved(folio);
 	__folio_set_head(folio);
 	/* we rely on prep_new_hugetlb_folio to set the destructor */
-	folio_set_compound_order(folio, order);
+	folio_set_order(folio, order);
 	for (i = 0; i < nr_pages; i++) {
 		p = folio_page(folio, i);
 
@@ -1872,7 +1872,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
 		p = folio_page(folio, j);
 		__ClearPageReserved(p);
 	}
-	folio_set_compound_order(folio, 0);
+	folio_set_order(folio, 0);
 	__folio_clear_head(folio);
 	return false;
 }
diff --git a/mm/internal.h b/mm/internal.h
index bcf75a8b032d..829b6a60ceb7 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -378,6 +378,27 @@ extern void *memmap_alloc(phys_addr_t size, phys_addr_t align,
 int split_free_page(struct page *free_page,
 			unsigned int order, unsigned long split_pfn_offset);
 
+/*
+ * This will have no effect, other than possibly generating a warning, if the
+ * caller passes in a non-large folio.
+ */
+static inline void folio_set_order(struct folio *folio, unsigned int order)
+{
+	if (!folio_test_large(folio)) {
+		WARN_ON_ONCE(order);
+		return;
+	}
+
+	folio->_folio_order = order;
+#ifdef CONFIG_64BIT
+	/*
+	 * When hugetlb dissolves a folio, we need to clear the tail
+	 * page, rather than setting nr_pages to 1.
+	 */
+	folio->_folio_nr_pages = order ? 1U << order : 0;
+#endif
+}
+
 #if defined CONFIG_COMPACTION || defined CONFIG_CMA
 
 /*
-- 
2.38.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-13 21:20 [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h Sidhartha Kumar
@ 2022-12-14  8:43 ` John Hubbard
  2022-12-14 20:35   ` Sidhartha Kumar
  2022-12-15  3:44 ` Muchun Song
  2022-12-16 22:27 ` Andrew Morton
  2 siblings, 1 reply; 11+ messages in thread
From: John Hubbard @ 2022-12-14  8:43 UTC (permalink / raw)
  To: Sidhartha Kumar, linux-kernel, linux-mm
  Cc: akpm, songmuchun, mike.kravetz, willy

On 12/13/22 13:20, Sidhartha Kumar wrote:
> folio_set_compound_order() is moved to an mm-internal location so external
> folio users cannot misuse this function. Change the name of the function
> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
> handle the case if a non-large folio is passed and add clarifying comments
> to the function.
> 
> Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
> Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")
> 
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
> Suggested-by: Muchun Song <songmuchun@bytedance.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Suggested-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  include/linux/mm.h | 16 ----------------
>  mm/hugetlb.c       |  6 +++---
>  mm/internal.h      | 21 +++++++++++++++++++++
>  3 files changed, 24 insertions(+), 19 deletions(-)

I think this looks good. One small question below.

> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7dc376052d40..300d92d2b49d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1019,22 +1019,6 @@ static inline void set_compound_order(struct page *page, unsigned int order)
>  #endif
>  }
>  
> -/*
> - * folio_set_compound_order is generally passed a non-zero order to
> - * initialize a large folio.  However, hugetlb code abuses this by
> - * passing in zero when 'dissolving' a large folio.
> - */
> -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;
> -#endif
> -}
> -
>  /* Returns the number of pages in this potentially compound page. */
>  static inline unsigned long compound_nr(struct page *page)
>  {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7cdbcc22587b..810e840bb4f1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1344,7 +1344,7 @@ static void __destroy_compound_gigantic_folio(struct folio *folio,
>  			set_page_refcounted(p);
>  	}
>  
> -	folio_set_compound_order(folio, 0);
> +	folio_set_order(folio, 0);
>  	__folio_clear_head(folio);
>  }
>  
> @@ -1808,7 +1808,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
>  	__folio_clear_reserved(folio);
>  	__folio_set_head(folio);
>  	/* we rely on prep_new_hugetlb_folio to set the destructor */
> -	folio_set_compound_order(folio, order);
> +	folio_set_order(folio, order);
>  	for (i = 0; i < nr_pages; i++) {
>  		p = folio_page(folio, i);
>  
> @@ -1872,7 +1872,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
>  		p = folio_page(folio, j);
>  		__ClearPageReserved(p);
>  	}
> -	folio_set_compound_order(folio, 0);
> +	folio_set_order(folio, 0);
>  	__folio_clear_head(folio);
>  	return false;
>  }
> diff --git a/mm/internal.h b/mm/internal.h
> index bcf75a8b032d..829b6a60ceb7 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -378,6 +378,27 @@ extern void *memmap_alloc(phys_addr_t size, phys_addr_t align,
>  int split_free_page(struct page *free_page,
>  			unsigned int order, unsigned long split_pfn_offset);
>  
> +/*
> + * This will have no effect, other than possibly generating a warning, if the
> + * caller passes in a non-large folio.
> + */
> +static inline void folio_set_order(struct folio *folio, unsigned int order)
> +{
> +	if (!folio_test_large(folio)) {
> +		WARN_ON_ONCE(order);
> +		return;
> +	}

Would it be better to do this (below)? I'm not sure of the value of
warning on "order"--it's a little odd and unexplained and doesn't really
do anything more helpful than simply warning about what why the code is
failing, which is really about !large, rather than order. Unless I'm
missing something?

	if (WARN_ON_ONCE(!folio_test_large(folio)))
		return;

Sorry to drive you crazy over nits. This is the last one from me. :)

thanks,
-- 
John Hubbard
NVIDIA

> +
> +	folio->_folio_order = order;
> +#ifdef CONFIG_64BIT
> +	/*
> +	 * When hugetlb dissolves a folio, we need to clear the tail
> +	 * page, rather than setting nr_pages to 1.
> +	 */
> +	folio->_folio_nr_pages = order ? 1U << order : 0;
> +#endif
> +}
> +
>  #if defined CONFIG_COMPACTION || defined CONFIG_CMA
>  
>  /*




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-14  8:43 ` John Hubbard
@ 2022-12-14 20:35   ` Sidhartha Kumar
  2022-12-14 22:53     ` John Hubbard
  0 siblings, 1 reply; 11+ messages in thread
From: Sidhartha Kumar @ 2022-12-14 20:35 UTC (permalink / raw)
  To: John Hubbard, linux-kernel, linux-mm
  Cc: akpm, songmuchun, mike.kravetz, willy

On 12/14/22 12:43 AM, John Hubbard wrote:
> On 12/13/22 13:20, Sidhartha Kumar wrote:
>> folio_set_compound_order() is moved to an mm-internal location so external
>> folio users cannot misuse this function. Change the name of the function
>> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
>> handle the case if a non-large folio is passed and add clarifying comments
>> to the function.
>>
>> Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
>> Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")
>>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Suggested-by: Muchun Song <songmuchun@bytedance.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Suggested-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   include/linux/mm.h | 16 ----------------
>>   mm/hugetlb.c       |  6 +++---
>>   mm/internal.h      | 21 +++++++++++++++++++++
>>   3 files changed, 24 insertions(+), 19 deletions(-)
> 
> I think this looks good. One small question below.
> 
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 7dc376052d40..300d92d2b49d 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1019,22 +1019,6 @@ static inline void set_compound_order(struct page *page, unsigned int order)
>>   #endif
>>   }
>>   
>> -/*
>> - * folio_set_compound_order is generally passed a non-zero order to
>> - * initialize a large folio.  However, hugetlb code abuses this by
>> - * passing in zero when 'dissolving' a large folio.
>> - */
>> -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;
>> -#endif
>> -}
>> -
>>   /* Returns the number of pages in this potentially compound page. */
>>   static inline unsigned long compound_nr(struct page *page)
>>   {
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 7cdbcc22587b..810e840bb4f1 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1344,7 +1344,7 @@ static void __destroy_compound_gigantic_folio(struct folio *folio,
>>   			set_page_refcounted(p);
>>   	}
>>   
>> -	folio_set_compound_order(folio, 0);
>> +	folio_set_order(folio, 0);
>>   	__folio_clear_head(folio);
>>   }
>>   
>> @@ -1808,7 +1808,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
>>   	__folio_clear_reserved(folio);
>>   	__folio_set_head(folio);
>>   	/* we rely on prep_new_hugetlb_folio to set the destructor */
>> -	folio_set_compound_order(folio, order);
>> +	folio_set_order(folio, order);
>>   	for (i = 0; i < nr_pages; i++) {
>>   		p = folio_page(folio, i);
>>   
>> @@ -1872,7 +1872,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
>>   		p = folio_page(folio, j);
>>   		__ClearPageReserved(p);
>>   	}
>> -	folio_set_compound_order(folio, 0);
>> +	folio_set_order(folio, 0);
>>   	__folio_clear_head(folio);
>>   	return false;
>>   }
>> diff --git a/mm/internal.h b/mm/internal.h
>> index bcf75a8b032d..829b6a60ceb7 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -378,6 +378,27 @@ extern void *memmap_alloc(phys_addr_t size, phys_addr_t align,
>>   int split_free_page(struct page *free_page,
>>   			unsigned int order, unsigned long split_pfn_offset);
>>   
>> +/*
>> + * This will have no effect, other than possibly generating a warning, if the
>> + * caller passes in a non-large folio.
>> + */
>> +static inline void folio_set_order(struct folio *folio, unsigned int order)
>> +{
>> +	if (!folio_test_large(folio)) {
>> +		WARN_ON_ONCE(order);
>> +		return;
>> +	}
> 
> Would it be better to do this (below)? I'm not sure of the value of
> warning on "order"--it's a little odd and unexplained and doesn't really
> do anything more helpful than simply warning about what why the code is
> failing, which is really about !large, rather than order. Unless I'm
> missing something?
> 
> 	if (WARN_ON_ONCE(!folio_test_large(folio)))
> 		return;
>I agree that warning this way is clearer, I will change in a v2.

> Sorry to drive you crazy over nits. This is the last one from me. :)
> 
No worries, I appreciate the feedback.

Thanks,
Sidhartha Kumar

> thanks,



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-14 20:35   ` Sidhartha Kumar
@ 2022-12-14 22:53     ` John Hubbard
  0 siblings, 0 replies; 11+ messages in thread
From: John Hubbard @ 2022-12-14 22:53 UTC (permalink / raw)
  To: Sidhartha Kumar, linux-kernel, linux-mm
  Cc: akpm, songmuchun, mike.kravetz, willy

On 12/14/22 12:35, Sidhartha Kumar wrote:
>>     if (WARN_ON_ONCE(!folio_test_large(folio)))
>>         return;
>> I agree that warning this way is clearer, I will change in a v2.
  
With that change, please feel free to add:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-13 21:20 [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h Sidhartha Kumar
  2022-12-14  8:43 ` John Hubbard
@ 2022-12-15  3:44 ` Muchun Song
  2022-12-15  5:09   ` Sidhartha Kumar
  2022-12-16 22:27 ` Andrew Morton
  2 siblings, 1 reply; 11+ messages in thread
From: Muchun Song @ 2022-12-15  3:44 UTC (permalink / raw)
  To: Sidhartha Kumar
  Cc: linux-kernel, Linux Memory Management List, Andrew Morton,
	Muchun Song, Mike Kravetz, Matthew Wilcox, John Hubbard



> On Dec 14, 2022, at 05:20, Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> 
> folio_set_compound_order() is moved to an mm-internal location so external
> folio users cannot misuse this function. Change the name of the function
> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
> handle the case if a non-large folio is passed and add clarifying comments
> to the function.
> 
> Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
> Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")
> 
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
> Suggested-by: Muchun Song <songmuchun@bytedance.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Suggested-by: John Hubbard <jhubbard@nvidia.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-15  3:44 ` Muchun Song
@ 2022-12-15  5:09   ` Sidhartha Kumar
  2022-12-15  5:31     ` Muchun Song
  0 siblings, 1 reply; 11+ messages in thread
From: Sidhartha Kumar @ 2022-12-15  5:09 UTC (permalink / raw)
  To: Muchun Song
  Cc: linux-kernel, Linux Memory Management List, Andrew Morton,
	Muchun Song, Mike Kravetz, Matthew Wilcox, John Hubbard

On 12/14/22 7:44 PM, Muchun Song wrote:
> 
> 
>> On Dec 14, 2022, at 05:20, Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
>>
>> folio_set_compound_order() is moved to an mm-internal location so external
>> folio users cannot misuse this function. Change the name of the function
>> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
>> handle the case if a non-large folio is passed and add clarifying comments
>> to the function.
>>
>> Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
>> Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")
>>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Suggested-by: Muchun Song <songmuchun@bytedance.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Suggested-by: John Hubbard <jhubbard@nvidia.com>
> 
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
Hi Muchun,

Does this review include the change from

+	if (!folio_test_large(folio)) {
+		WARN_ON_ONCE(order);
+		return;
+	}

to

if (WARN_ON_ONCE(!folio_test_large(folio)))
		return;

discussed in in this thread?

Thanks,
Sidhartha Kumar
> 
> Thanks.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-15  5:09   ` Sidhartha Kumar
@ 2022-12-15  5:31     ` Muchun Song
  0 siblings, 0 replies; 11+ messages in thread
From: Muchun Song @ 2022-12-15  5:31 UTC (permalink / raw)
  To: Sidhartha Kumar
  Cc: linux-kernel, Linux Memory Management List, Andrew Morton,
	Muchun Song, Mike Kravetz, Matthew Wilcox, John Hubbard



> On Dec 15, 2022, at 13:09, Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> 
> On 12/14/22 7:44 PM, Muchun Song wrote:
>>> On Dec 14, 2022, at 05:20, Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
>>> 
>>> folio_set_compound_order() is moved to an mm-internal location so external
>>> folio users cannot misuse this function. Change the name of the function
>>> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
>>> handle the case if a non-large folio is passed and add clarifying comments
>>> to the function.
>>> 
>>> Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
>>> Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")
>>> 
>>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
>>> Suggested-by: Muchun Song <songmuchun@bytedance.com>
>>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>>> Suggested-by: John Hubbard <jhubbard@nvidia.com>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> Hi Muchun,
> 
> Does this review include the change from

Yes.

Thanks.

> 
> + if (!folio_test_large(folio)) {
> + 	WARN_ON_ONCE(order);
> + 	return;
> + }
> 
> to
> 
> if (WARN_ON_ONCE(!folio_test_large(folio)))
> 	return;
> 
> discussed in in this thread?
> 
> Thanks,
> Sidhartha Kumar
>> Thanks.




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-13 21:20 [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h Sidhartha Kumar
  2022-12-14  8:43 ` John Hubbard
  2022-12-15  3:44 ` Muchun Song
@ 2022-12-16 22:27 ` Andrew Morton
  2022-12-16 22:56   ` John Hubbard
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2022-12-16 22:27 UTC (permalink / raw)
  To: Sidhartha Kumar
  Cc: linux-kernel, linux-mm, songmuchun, mike.kravetz, willy, jhubbard

On Tue, 13 Dec 2022 13:20:53 -0800 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:

> folio_set_compound_order() is moved to an mm-internal location so external
> folio users cannot misuse this function. Change the name of the function
> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
> handle the case if a non-large folio is passed and add clarifying comments
> to the function.
> 

This differs from the version I previously merged:

--- a/mm/internal.h~mm-move-folio_set_compound_order-to-mm-internalh-update
+++ a/mm/internal.h
@@ -384,8 +384,10 @@ int split_free_page(struct page *free_pa
  */
 static inline void folio_set_order(struct folio *folio, unsigned int order)
 {
-	if (WARN_ON_ONCE(!folio_test_large(folio)))
+	if (!folio_test_large(folio)) {
+		WARN_ON_ONCE(order);
 		return;
+	}
 
 	folio->_folio_order = order;
 #ifdef CONFIG_64BIT

Makes sense.  But wouldn't

	if (WARN_ON_ONCE(order && !folio_test_large(folio)))

be clearer?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-16 22:27 ` Andrew Morton
@ 2022-12-16 22:56   ` John Hubbard
  2022-12-16 23:09     ` Andrew Morton
  2022-12-16 23:10     ` Sidhartha Kumar
  0 siblings, 2 replies; 11+ messages in thread
From: John Hubbard @ 2022-12-16 22:56 UTC (permalink / raw)
  To: Andrew Morton, Sidhartha Kumar
  Cc: linux-kernel, linux-mm, songmuchun, mike.kravetz, willy

On 12/16/22 14:27, Andrew Morton wrote:
> On Tue, 13 Dec 2022 13:20:53 -0800 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> 
>> folio_set_compound_order() is moved to an mm-internal location so external
>> folio users cannot misuse this function. Change the name of the function
>> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
>> handle the case if a non-large folio is passed and add clarifying comments
>> to the function.
>>
> 
> This differs from the version I previously merged:
> 
> --- a/mm/internal.h~mm-move-folio_set_compound_order-to-mm-internalh-update
> +++ a/mm/internal.h
> @@ -384,8 +384,10 @@ int split_free_page(struct page *free_pa
>    */
>   static inline void folio_set_order(struct folio *folio, unsigned int order)
>   {
> -	if (WARN_ON_ONCE(!folio_test_large(folio)))
> +	if (!folio_test_large(folio)) {
> +		WARN_ON_ONCE(order);
>   		return;
> +	}

I think that's out of date?

We eventually settled on the version that is (as of this a few minutes
ago) already in mm-unstable (commit fdea060a130d: "mm: move
folio_set_compound_order() to mm/internal.h"), which has it like this:

        if (WARN_ON_ONCE(!folio_test_large(folio)))
                return;

>   
>   	folio->_folio_order = order;
>   #ifdef CONFIG_64BIT
> 
> Makes sense.  But wouldn't
> 
> 	if (WARN_ON_ONCE(order && !folio_test_large(folio)))
> 
> be clearer?

That's a little narrower of a check. But maybe that's desirable. Could
someone (Mike, Muchun, Sidhartha) comment on which behavior is
preferable, please? I think I'm a little dizzy at this point. :)


thanks,
-- 
John Hubbard
NVIDIA


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-16 22:56   ` John Hubbard
@ 2022-12-16 23:09     ` Andrew Morton
  2022-12-16 23:10     ` Sidhartha Kumar
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2022-12-16 23:09 UTC (permalink / raw)
  To: John Hubbard
  Cc: Sidhartha Kumar, linux-kernel, linux-mm, songmuchun, mike.kravetz, willy

On Fri, 16 Dec 2022 14:56:47 -0800 John Hubbard <jhubbard@nvidia.com> wrote:

> On 12/16/22 14:27, Andrew Morton wrote:
> > On Tue, 13 Dec 2022 13:20:53 -0800 Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
> > 
> >> folio_set_compound_order() is moved to an mm-internal location so external
> >> folio users cannot misuse this function. Change the name of the function
> >> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
> >> handle the case if a non-large folio is passed and add clarifying comments
> >> to the function.
> >>
> > 
> > This differs from the version I previously merged:
> > 
> > --- a/mm/internal.h~mm-move-folio_set_compound_order-to-mm-internalh-update
> > +++ a/mm/internal.h
> > @@ -384,8 +384,10 @@ int split_free_page(struct page *free_pa
> >    */
> >   static inline void folio_set_order(struct folio *folio, unsigned int order)
> >   {
> > -	if (WARN_ON_ONCE(!folio_test_large(folio)))
> > +	if (!folio_test_large(folio)) {
> > +		WARN_ON_ONCE(order);
> >   		return;
> > +	}
> 
> I think that's out of date?

Yes thanks, I'm looking at patches in the wrong order.  I'll drop the
above delta.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h
  2022-12-16 22:56   ` John Hubbard
  2022-12-16 23:09     ` Andrew Morton
@ 2022-12-16 23:10     ` Sidhartha Kumar
  1 sibling, 0 replies; 11+ messages in thread
From: Sidhartha Kumar @ 2022-12-16 23:10 UTC (permalink / raw)
  To: John Hubbard, Andrew Morton
  Cc: linux-kernel, linux-mm, songmuchun, mike.kravetz, willy

On 12/16/22 2:56 PM, John Hubbard wrote:
> On 12/16/22 14:27, Andrew Morton wrote:
>> On Tue, 13 Dec 2022 13:20:53 -0800 Sidhartha Kumar 
>> <sidhartha.kumar@oracle.com> wrote:
>>
>>> folio_set_compound_order() is moved to an mm-internal location so 
>>> external
>>> folio users cannot misuse this function. Change the name of the function
>>> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
>>> handle the case if a non-large folio is passed and add clarifying 
>>> comments
>>> to the function.
>>>
>>
>> This differs from the version I previously merged:
>>
>> --- 
>> a/mm/internal.h~mm-move-folio_set_compound_order-to-mm-internalh-update
>> +++ a/mm/internal.h
>> @@ -384,8 +384,10 @@ int split_free_page(struct page *free_pa
>>    */
>>   static inline void folio_set_order(struct folio *folio, unsigned int 
>> order)
>>   {
>> -    if (WARN_ON_ONCE(!folio_test_large(folio)))
>> +    if (!folio_test_large(folio)) {
>> +        WARN_ON_ONCE(order);
>>           return;
>> +    }
> 
> I think that's out of date?
> 
> We eventually settled on the version that is (as of this a few minutes
> ago) already in mm-unstable (commit fdea060a130d: "mm: move
> folio_set_compound_order() to mm/internal.h"), which has it like this:
> 
Hi Andrew, yes this version that is already in mm-unstable represents 
the v2 of this patch which is what we agreed on. I think the patch 
mm-move-folio_set_compound_order-to-mm-internalh-update with description 
"alter the folio_set_order() warning" which was just added to 
mm-unstable should be removed as our discussion lead us away from that 
version.


>         if (WARN_ON_ONCE(!folio_test_large(folio)))
>                 return;
> 
>>       folio->_folio_order = order;
>>   #ifdef CONFIG_64BIT
>>
>> Makes sense.  But wouldn't
>>
>>     if (WARN_ON_ONCE(order && !folio_test_large(folio)))
>>
>> be clearer?
> That's a little narrower of a check. But maybe that's desirable. Could

Ya I think it would helpful to have a wider catch for the warn as any 
user calling folio_set_order() with a non-large folio should be aware as 
they could misuse the folio later on even if they passed in a 0 order 
because order itself would be an OOB access.

Thanks,
Sidhartha Kumar
> someone (Mike, Muchun, Sidhartha) comment on which behavior is
> preferable, please? I think I'm a little dizzy at this point. :)
> 
> 
> thanks,



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-12-16 23:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 21:20 [PATCH mm-unstable] mm: move folio_set_compound_order() to mm/internal.h Sidhartha Kumar
2022-12-14  8:43 ` John Hubbard
2022-12-14 20:35   ` Sidhartha Kumar
2022-12-14 22:53     ` John Hubbard
2022-12-15  3:44 ` Muchun Song
2022-12-15  5:09   ` Sidhartha Kumar
2022-12-15  5:31     ` Muchun Song
2022-12-16 22:27 ` Andrew Morton
2022-12-16 22:56   ` John Hubbard
2022-12-16 23:09     ` Andrew Morton
2022-12-16 23:10     ` Sidhartha Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).