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 <[email protected]>
> Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Signed-off-by: "Kirill A. Shutemov" <[email protected]>
> Cc: "Kirill A. Shutemov" <[email protected]>
> Cc: Zi Yan <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
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
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 <[email protected]>
> > Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > Signed-off-by: "Kirill A. Shutemov" <[email protected]>
> > Cc: "Kirill A. Shutemov" <[email protected]>
> > Cc: Zi Yan <[email protected]>
> > Signed-off-by: Andrew Morton <[email protected]>
>
> 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
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 <[email protected]>
>>> Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>> Signed-off-by: "Kirill A. Shutemov" <[email protected]>
>>> Cc: "Kirill A. Shutemov" <[email protected]>
>>> Cc: Zi Yan <[email protected]>
>>> Signed-off-by: Andrew Morton <[email protected]>
>>
>> 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
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 <[email protected]>
> > > > Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > > Signed-off-by: "Kirill A. Shutemov" <[email protected]>
> > > > Cc: "Kirill A. Shutemov" <[email protected]>
> > > > Cc: Zi Yan <[email protected]>
> > > > Signed-off-by: Andrew Morton <[email protected]>
> > >
> > > 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
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 <[email protected]>
>>>>> Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>>>> Signed-off-by: "Kirill A. Shutemov" <[email protected]>
>>>>> Cc: "Kirill A. Shutemov" <[email protected]>
>>>>> Cc: Zi Yan <[email protected]>
>>>>> Signed-off-by: Andrew Morton <[email protected]>
>>>>
>>>> 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
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 <[email protected]>
> > > > > Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > > > Signed-off-by: "Kirill A. Shutemov" <[email protected]>
> > > > > Cc: "Kirill A. Shutemov" <[email protected]>
> > > > > Cc: Zi Yan <[email protected]>
> > > > > Signed-off-by: Andrew Morton <[email protected]>
> > > >
> > > > 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.
On Fri, 7 Apr 2023 00:14:31 +0300 Mike Rapoport <[email protected]> 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.
On Thu, Apr 06, 2023 at 03:44:23PM -0700, Andrew Morton wrote:
> On Fri, 7 Apr 2023 00:14:31 +0300 Mike Rapoport <[email protected]> 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
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 <[email protected]> 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) <[email protected]>
> + */
> + 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.