Messages in this thread | | | Date | Thu, 6 Apr 2023 11:23:30 -0700 | Subject | Re: [PATCH] mm-treewide-redefine-max_order-sanely-fix.txt | From | Guenter Roeck <> |
| |
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
| |