All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
@ 2023-04-06  5:20 Guenter Roeck
  2023-04-06  7:25 ` Kirill A. Shutemov
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2023-04-06  5:20 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: Andrew Morton, linux-mm, linux-kernel

Hi,

On Wed, Mar 15, 2023 at 06:38:00PM +0300, Kirill A. Shutemov wrote:
> fix min() warning
> 
> Link: https://lkml.kernel.org/r/20230315153800.32wib3n5rickolvh@box
> Reported-by: kernel test robot <lkp@intel.com>
>   Link: https://lore.kernel.org/oe-kbuild-all/202303152343.D93IbJmn-lkp@intel.com/
> Signed-off-by: "Kirill A. Shutemov" <kirill@shutemov.name>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

This patch results in various boot failures (hang) on arm targets
in linux-next. Debug messages reveal the reason.

########### MAX_ORDER=10 start=0 __ffs(start)=-1 min()=10 min_t=-1
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If start==0, __ffs(start) returns 0xfffffff or (as int) -1, which min_t()
interprets as such, while min() apparently uses the returned unsigned long
value. Obviously a negative order isn't received well by the rest of the
code.

Guenter

> ---
>  mm/memblock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 338b8cb0793e..7911224b1ed3 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2043,7 +2043,7 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
>  	int order;
>  
>  	while (start < end) {
> -		order = min(MAX_ORDER, __ffs(start));
> +		order = min_t(int, MAX_ORDER, __ffs(start));
>  
>  		while (start + (1UL << order) > end)
>  			order--;
> -- 
> 2.39.2

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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06  5:20 [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt Guenter Roeck
@ 2023-04-06  7:25 ` Kirill A. Shutemov
  2023-04-06 13:57   ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Kirill A. Shutemov @ 2023-04-06  7:25 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Andrew Morton, linux-mm, linux-kernel

On Wed, Apr 05, 2023 at 10:20:26PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Wed, Mar 15, 2023 at 06:38:00PM +0300, Kirill A. Shutemov wrote:
> > fix min() warning
> > 
> > Link: https://lkml.kernel.org/r/20230315153800.32wib3n5rickolvh@box
> > Reported-by: kernel test robot <lkp@intel.com>
> >   Link: https://lore.kernel.org/oe-kbuild-all/202303152343.D93IbJmn-lkp@intel.com/
> > Signed-off-by: "Kirill A. Shutemov" <kirill@shutemov.name>
> > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Cc: Zi Yan <ziy@nvidia.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> This patch results in various boot failures (hang) on arm targets
> in linux-next. Debug messages reveal the reason.
> 
> ########### MAX_ORDER=10 start=0 __ffs(start)=-1 min()=10 min_t=-1
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> If start==0, __ffs(start) returns 0xfffffff or (as int) -1, which min_t()
> interprets as such, while min() apparently uses the returned unsigned long
> value. Obviously a negative order isn't received well by the rest of the
> code.

Actually, __ffs() is not defined for 0.

Maybe something like this?

diff --git a/mm/memblock.c b/mm/memblock.c
index 7911224b1ed3..63603b943bd0 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2043,7 +2043,11 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
 	int order;
 
 	while (start < end) {
-		order = min_t(int, MAX_ORDER, __ffs(start));
+		/* __ffs() behaviour is undefined for 0 */
+		if (start)
+			order = min_t(int, MAX_ORDER, __ffs(start));
+		else
+			order = MAX_ORDER;
 
 		while (start + (1UL << order) > end)
 			order--;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c8f0a8c2d049..3179c30f7958 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -605,7 +605,13 @@ static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
 	 * this and the first chunk to online will be pageblock_nr_pages.
 	 */
 	for (pfn = start_pfn; pfn < end_pfn;) {
-		int order = min_t(int, MAX_ORDER, __ffs(pfn));
+		int order;
+
+		/* __ffs() behaviour is undefined for 0 */
+		if (pfn)
+			order = min_t(int, MAX_ORDER, __ffs(pfn));
+		else
+			order = MAX_ORDER;
 
 		(*online_page_callback)(pfn_to_page(pfn), order);
 		pfn += (1UL << order);
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06  7:25 ` Kirill A. Shutemov
@ 2023-04-06 13:57   ` Guenter Roeck
  2023-04-06 15:10     ` Kirill A. Shutemov
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2023-04-06 13:57 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: Andrew Morton, linux-mm, linux-kernel

On 4/6/23 00:25, Kirill A. Shutemov wrote:
> On Wed, Apr 05, 2023 at 10:20:26PM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> On Wed, Mar 15, 2023 at 06:38:00PM +0300, Kirill A. Shutemov wrote:
>>> fix min() warning
>>>
>>> Link: https://lkml.kernel.org/r/20230315153800.32wib3n5rickolvh@box
>>> Reported-by: kernel test robot <lkp@intel.com>
>>>    Link: https://lore.kernel.org/oe-kbuild-all/202303152343.D93IbJmn-lkp@intel.com/
>>> Signed-off-by: "Kirill A. Shutemov" <kirill@shutemov.name>
>>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>>> Cc: Zi Yan <ziy@nvidia.com>
>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>
>> This patch results in various boot failures (hang) on arm targets
>> in linux-next. Debug messages reveal the reason.
>>
>> ########### MAX_ORDER=10 start=0 __ffs(start)=-1 min()=10 min_t=-1
>>                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> If start==0, __ffs(start) returns 0xfffffff or (as int) -1, which min_t()
>> interprets as such, while min() apparently uses the returned unsigned long
>> value. Obviously a negative order isn't received well by the rest of the
>> code.
> 
> Actually, __ffs() is not defined for 0.
> 
> Maybe something like this?
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 7911224b1ed3..63603b943bd0 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2043,7 +2043,11 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
>   	int order;
>   
>   	while (start < end) {
> -		order = min_t(int, MAX_ORDER, __ffs(start));
> +		/* __ffs() behaviour is undefined for 0 */
> +		if (start)
> +			order = min_t(int, MAX_ORDER, __ffs(start));
> +		else
> +			order = MAX_ORDER;
>   

Shouldn't that be
		else
			order = 0;
?

Guenter


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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06 13:57   ` Guenter Roeck
@ 2023-04-06 15:10     ` Kirill A. Shutemov
  2023-04-06 18:23       ` Guenter Roeck
  2023-04-06 21:14       ` Mike Rapoport
  0 siblings, 2 replies; 9+ messages in thread
From: Kirill A. Shutemov @ 2023-04-06 15:10 UTC (permalink / raw)
  To: Guenter Roeck, Mike Rapoport; +Cc: Andrew Morton, linux-mm, linux-kernel

On Thu, Apr 06, 2023 at 06:57:41AM -0700, Guenter Roeck wrote:
> On 4/6/23 00:25, Kirill A. Shutemov wrote:
> > On Wed, Apr 05, 2023 at 10:20:26PM -0700, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > On Wed, Mar 15, 2023 at 06:38:00PM +0300, Kirill A. Shutemov wrote:
> > > > fix min() warning
> > > > 
> > > > Link: https://lkml.kernel.org/r/20230315153800.32wib3n5rickolvh@box
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > >    Link: https://lore.kernel.org/oe-kbuild-all/202303152343.D93IbJmn-lkp@intel.com/
> > > > Signed-off-by: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > > Cc: Zi Yan <ziy@nvidia.com>
> > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > 
> > > This patch results in various boot failures (hang) on arm targets
> > > in linux-next. Debug messages reveal the reason.
> > > 
> > > ########### MAX_ORDER=10 start=0 __ffs(start)=-1 min()=10 min_t=-1
> > >                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > If start==0, __ffs(start) returns 0xfffffff or (as int) -1, which min_t()
> > > interprets as such, while min() apparently uses the returned unsigned long
> > > value. Obviously a negative order isn't received well by the rest of the
> > > code.
> > 
> > Actually, __ffs() is not defined for 0.
> > 
> > Maybe something like this?
> > 
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 7911224b1ed3..63603b943bd0 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -2043,7 +2043,11 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
> >   	int order;
> >   	while (start < end) {
> > -		order = min_t(int, MAX_ORDER, __ffs(start));
> > +		/* __ffs() behaviour is undefined for 0 */
> > +		if (start)
> > +			order = min_t(int, MAX_ORDER, __ffs(start));
> > +		else
> > +			order = MAX_ORDER;
> 
> Shouldn't that be
> 		else
> 			order = 0;
> ?

+Mike.

No. start == 0 is MAX_ORDER-aligned. We want to free the pages in the
largest chunks alignment allows.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06 15:10     ` Kirill A. Shutemov
@ 2023-04-06 18:23       ` Guenter Roeck
  2023-04-06 21:14       ` Mike Rapoport
  1 sibling, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2023-04-06 18:23 UTC (permalink / raw)
  To: Kirill A. Shutemov, Mike Rapoport; +Cc: Andrew Morton, linux-mm, linux-kernel

On 4/6/23 08:10, Kirill A. Shutemov wrote:
> On Thu, Apr 06, 2023 at 06:57:41AM -0700, Guenter Roeck wrote:
>> On 4/6/23 00:25, Kirill A. Shutemov wrote:
>>> On Wed, Apr 05, 2023 at 10:20:26PM -0700, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On Wed, Mar 15, 2023 at 06:38:00PM +0300, Kirill A. Shutemov wrote:
>>>>> fix min() warning
>>>>>
>>>>> Link: https://lkml.kernel.org/r/20230315153800.32wib3n5rickolvh@box
>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>>     Link: https://lore.kernel.org/oe-kbuild-all/202303152343.D93IbJmn-lkp@intel.com/
>>>>> Signed-off-by: "Kirill A. Shutemov" <kirill@shutemov.name>
>>>>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>>>>> Cc: Zi Yan <ziy@nvidia.com>
>>>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>>>
>>>> This patch results in various boot failures (hang) on arm targets
>>>> in linux-next. Debug messages reveal the reason.
>>>>
>>>> ########### MAX_ORDER=10 start=0 __ffs(start)=-1 min()=10 min_t=-1
>>>>                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> If start==0, __ffs(start) returns 0xfffffff or (as int) -1, which min_t()
>>>> interprets as such, while min() apparently uses the returned unsigned long
>>>> value. Obviously a negative order isn't received well by the rest of the
>>>> code.
>>>
>>> Actually, __ffs() is not defined for 0.
>>>
>>> Maybe something like this?
>>>
>>> diff --git a/mm/memblock.c b/mm/memblock.c
>>> index 7911224b1ed3..63603b943bd0 100644
>>> --- a/mm/memblock.c
>>> +++ b/mm/memblock.c
>>> @@ -2043,7 +2043,11 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
>>>    	int order;
>>>    	while (start < end) {
>>> -		order = min_t(int, MAX_ORDER, __ffs(start));
>>> +		/* __ffs() behaviour is undefined for 0 */
>>> +		if (start)
>>> +			order = min_t(int, MAX_ORDER, __ffs(start));
>>> +		else
>>> +			order = MAX_ORDER;
>>
>> Shouldn't that be
>> 		else
>> 			order = 0;
>> ?
> 
> +Mike.
> 
> No. start == 0 is MAX_ORDER-aligned. We want to free the pages in the
> largest chunks alignment allows.
> 

Ah, ok. Makes sense.

Thanks,
Guenter


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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06 15:10     ` Kirill A. Shutemov
  2023-04-06 18:23       ` Guenter Roeck
@ 2023-04-06 21:14       ` Mike Rapoport
  2023-04-06 22:44         ` Andrew Morton
  1 sibling, 1 reply; 9+ messages in thread
From: Mike Rapoport @ 2023-04-06 21:14 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: Guenter Roeck, Andrew Morton, linux-mm, linux-kernel

On Thu, Apr 06, 2023 at 06:10:15PM +0300, Kirill A. Shutemov wrote:
> On Thu, Apr 06, 2023 at 06:57:41AM -0700, Guenter Roeck wrote:
> > On 4/6/23 00:25, Kirill A. Shutemov wrote:
> > > On Wed, Apr 05, 2023 at 10:20:26PM -0700, Guenter Roeck wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Mar 15, 2023 at 06:38:00PM +0300, Kirill A. Shutemov wrote:
> > > > > fix min() warning
> > > > > 
> > > > > Link: https://lkml.kernel.org/r/20230315153800.32wib3n5rickolvh@box
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > >    Link: https://lore.kernel.org/oe-kbuild-all/202303152343.D93IbJmn-lkp@intel.com/
> > > > > Signed-off-by: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > > > Cc: Zi Yan <ziy@nvidia.com>
> > > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > > 
> > > > This patch results in various boot failures (hang) on arm targets
> > > > in linux-next. Debug messages reveal the reason.
> > > > 
> > > > ########### MAX_ORDER=10 start=0 __ffs(start)=-1 min()=10 min_t=-1
> > > >                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > 
> > > > If start==0, __ffs(start) returns 0xfffffff or (as int) -1, which min_t()
> > > > interprets as such, while min() apparently uses the returned unsigned long
> > > > value. Obviously a negative order isn't received well by the rest of the
> > > > code.
> > > 
> > > Actually, __ffs() is not defined for 0.
> > > 
> > > Maybe something like this?
> > > 
> > > diff --git a/mm/memblock.c b/mm/memblock.c
> > > index 7911224b1ed3..63603b943bd0 100644
> > > --- a/mm/memblock.c
> > > +++ b/mm/memblock.c
> > > @@ -2043,7 +2043,11 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
> > >   	int order;
> > >   	while (start < end) {
> > > -		order = min_t(int, MAX_ORDER, __ffs(start));
> > > +		/* __ffs() behaviour is undefined for 0 */
> > > +		if (start)
> > > +			order = min_t(int, MAX_ORDER, __ffs(start));
> > > +		else
> > > +			order = MAX_ORDER;
> > 
> > Shouldn't that be
> > 		else
> > 			order = 0;
> > ?
> 
> +Mike.
> 
> No. start == 0 is MAX_ORDER-aligned. We want to free the pages in the
> largest chunks alignment allows.

Right. Before the changes to MAX_ORDER it was

		order = min(MAX_ORDER - 1UL, __ffs(start));

which would evaluate to 10.

I'd just prefer the comment to include the explanation about why we choose
MAX_ORDER for start == 0. Say

	/*
	 * __ffs() behaviour is undefined for 0 and we want to free the
	 * pages in the largest chunks alignment allows, so set order to
	 * MAX_ORDER when start == 0
	 */

> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov

-- 
Sincerely yours,
Mike.

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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06 21:14       ` Mike Rapoport
@ 2023-04-06 22:44         ` Andrew Morton
  2023-04-07 12:40           ` Kirill A. Shutemov
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2023-04-06 22:44 UTC (permalink / raw)
  To: Mike Rapoport; +Cc: Kirill A. Shutemov, Guenter Roeck, linux-mm, linux-kernel

On Fri, 7 Apr 2023 00:14:31 +0300 Mike Rapoport <rppt@kernel.org> wrote:

> > > Shouldn't that be
> > > 		else
> > > 			order = 0;
> > > ?
> > 
> > +Mike.
> > 
> > No. start == 0 is MAX_ORDER-aligned. We want to free the pages in the
> > largest chunks alignment allows.
> 
> Right. Before the changes to MAX_ORDER it was
> 
> 		order = min(MAX_ORDER - 1UL, __ffs(start));
> 
> which would evaluate to 10.
> 
> I'd just prefer the comment to include the explanation about why we choose
> MAX_ORDER for start == 0. Say
> 
> 	/*
> 	 * __ffs() behaviour is undefined for 0 and we want to free the
> 	 * pages in the largest chunks alignment allows, so set order to
> 	 * MAX_ORDER when start == 0
> 	 */

Meanwhile I'd like to fix "various boot failures (hang) on arm targets"
in -next, so I queued up Kirill's informal fix for now.


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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-06 22:44         ` Andrew Morton
@ 2023-04-07 12:40           ` Kirill A. Shutemov
  2023-04-07 18:03             ` Mike Rapoport
  0 siblings, 1 reply; 9+ messages in thread
From: Kirill A. Shutemov @ 2023-04-07 12:40 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Mike Rapoport, Guenter Roeck, linux-mm, linux-kernel

On Thu, Apr 06, 2023 at 03:44:23PM -0700, Andrew Morton wrote:
> On Fri, 7 Apr 2023 00:14:31 +0300 Mike Rapoport <rppt@kernel.org> wrote:
> 
> > > > Shouldn't that be
> > > > 		else
> > > > 			order = 0;
> > > > ?
> > > 
> > > +Mike.
> > > 
> > > No. start == 0 is MAX_ORDER-aligned. We want to free the pages in the
> > > largest chunks alignment allows.
> > 
> > Right. Before the changes to MAX_ORDER it was
> > 
> > 		order = min(MAX_ORDER - 1UL, __ffs(start));
> > 
> > which would evaluate to 10.
> > 
> > I'd just prefer the comment to include the explanation about why we choose
> > MAX_ORDER for start == 0. Say
> > 
> > 	/*
> > 	 * __ffs() behaviour is undefined for 0 and we want to free the
> > 	 * pages in the largest chunks alignment allows, so set order to
> > 	 * MAX_ORDER when start == 0
> > 	 */
> 
> Meanwhile I'd like to fix "various boot failures (hang) on arm targets"
> in -next, so I queued up Kirill's informal fix for now.

Here's my variant of the fix up with more vervose comments.

diff --git a/mm/memblock.c b/mm/memblock.c
index 7911224b1ed3..381e36ac9e4d 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2043,7 +2043,16 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
 	int order;
 
 	while (start < end) {
-		order = min_t(int, MAX_ORDER, __ffs(start));
+		/*
+		 * Free the pages in the largest chunks alignment allows.
+		 *
+		 * __ffs() behaviour is undefined for 0. start == 0 is
+		 * MAX_ORDER-aligned, Set order to MAX_ORDER for the case.
+		 */
+		if (start)
+			order = min_t(int, MAX_ORDER, __ffs(start));
+		else
+			order = MAX_ORDER;
 
 		while (start + (1UL << order) > end)
 			order--;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c8f0a8c2d049..8e0fa209d533 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -605,7 +605,18 @@ static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
 	 * this and the first chunk to online will be pageblock_nr_pages.
 	 */
 	for (pfn = start_pfn; pfn < end_pfn;) {
-		int order = min_t(int, MAX_ORDER, __ffs(pfn));
+		int order;
+
+		/*
+		 * Free to online pages in the largest chunks alignment allows.
+		 *
+		 * __ffs() behaviour is undefined for 0. start == 0 is
+		 * MAX_ORDER-aligned, Set order to MAX_ORDER for the case.
+		 */
+		if (pfn)
+			order = min_t(int, MAX_ORDER, __ffs(pfn));
+		else
+			order = MAX_ORDER;
 
 		(*online_page_callback)(pfn_to_page(pfn), order);
 		pfn += (1UL << order);
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

* Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt
  2023-04-07 12:40           ` Kirill A. Shutemov
@ 2023-04-07 18:03             ` Mike Rapoport
  0 siblings, 0 replies; 9+ messages in thread
From: Mike Rapoport @ 2023-04-07 18:03 UTC (permalink / raw)
  To: Kirill A. Shutemov; +Cc: Andrew Morton, Guenter Roeck, linux-mm, linux-kernel

On Fri, Apr 07, 2023 at 03:40:54PM +0300, Kirill A. Shutemov wrote:
> On Thu, Apr 06, 2023 at 03:44:23PM -0700, Andrew Morton wrote:
> > On Fri, 7 Apr 2023 00:14:31 +0300 Mike Rapoport <rppt@kernel.org> wrote:
> > 
> > > > > Shouldn't that be
> > > > > 		else
> > > > > 			order = 0;
> > > > > ?
> > > > 
> > > > +Mike.
> > > > 
> > > > No. start == 0 is MAX_ORDER-aligned. We want to free the pages in the
> > > > largest chunks alignment allows.
> > > 
> > > Right. Before the changes to MAX_ORDER it was
> > > 
> > > 		order = min(MAX_ORDER - 1UL, __ffs(start));
> > > 
> > > which would evaluate to 10.
> > > 
> > > I'd just prefer the comment to include the explanation about why we choose
> > > MAX_ORDER for start == 0. Say
> > > 
> > > 	/*
> > > 	 * __ffs() behaviour is undefined for 0 and we want to free the
> > > 	 * pages in the largest chunks alignment allows, so set order to
> > > 	 * MAX_ORDER when start == 0
> > > 	 */
> > 
> > Meanwhile I'd like to fix "various boot failures (hang) on arm targets"
> > in -next, so I queued up Kirill's informal fix for now.
> 
> Here's my variant of the fix up with more vervose comments.
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 7911224b1ed3..381e36ac9e4d 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2043,7 +2043,16 @@ static void __init __free_pages_memory(unsigned long start, unsigned long end)
>  	int order;
>  
>  	while (start < end) {
> -		order = min_t(int, MAX_ORDER, __ffs(start));
> +		/*
> +		 * Free the pages in the largest chunks alignment allows.
> +		 *
> +		 * __ffs() behaviour is undefined for 0. start == 0 is
> +		 * MAX_ORDER-aligned, Set order to MAX_ORDER for the case.

                                      ^ small s
otherwise feel free to add

Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>

> +		 */
> +		if (start)
> +			order = min_t(int, MAX_ORDER, __ffs(start));
> +		else
> +			order = MAX_ORDER;
>  
>  		while (start + (1UL << order) > end)
>  			order--;
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c8f0a8c2d049..8e0fa209d533 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -605,7 +605,18 @@ static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
>  	 * this and the first chunk to online will be pageblock_nr_pages.
>  	 */
>  	for (pfn = start_pfn; pfn < end_pfn;) {
> -		int order = min_t(int, MAX_ORDER, __ffs(pfn));
> +		int order;
> +
> +		/*
> +		 * Free to online pages in the largest chunks alignment allows.
> +		 *
> +		 * __ffs() behaviour is undefined for 0. start == 0 is
> +		 * MAX_ORDER-aligned, Set order to MAX_ORDER for the case.
> +		 */
> +		if (pfn)
> +			order = min_t(int, MAX_ORDER, __ffs(pfn));
> +		else
> +			order = MAX_ORDER;
>  
>  		(*online_page_callback)(pfn_to_page(pfn), order);
>  		pfn += (1UL << order);
> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov

-- 
Sincerely yours,
Mike.

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

end of thread, other threads:[~2023-04-07 18:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-06  5:20 [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt Guenter Roeck
2023-04-06  7:25 ` Kirill A. Shutemov
2023-04-06 13:57   ` Guenter Roeck
2023-04-06 15:10     ` Kirill A. Shutemov
2023-04-06 18:23       ` Guenter Roeck
2023-04-06 21:14       ` Mike Rapoport
2023-04-06 22:44         ` Andrew Morton
2023-04-07 12:40           ` Kirill A. Shutemov
2023-04-07 18:03             ` Mike Rapoport

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.