Message-ID: <c6e7bedc-bbe3-0166-3ed8-8f8012fad132@mips.com>
Date:   Tue, 23 Jan 2018 14:01:50 +0000
From:   Matt Redfearn <matt.redfearn@...s.com>
To:     Mathieu Malaterre <malat@...ian.org>
CC:     Serge Semin <fancer.lancer@...il.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Miodrag Dinic <miodrag.dinic@...s.com>,
        James Hogan <jhogan@...nel.org>, <goran.ferenc@...s.com>,
        David Daney <david.daney@...ium.com>,
        <paul.gortmaker@...driver.com>, Paul Burton <paul.burton@...s.com>,
        <alex.belits@...ium.com>, <Steven.Hill@...ium.com>,
        <alexander.sverdlin@...ia.com>, <kumba@...too.org>,
        Marcin Nowakowski <marcin.nowakowski@...s.com>,
        James Hogan <James.hogan@...s.com>,
        Peter Wotton <Peter.Wotton@...s.com>,
        <Sergey.Semin@...latforms.ru>, <linux-mips@...ux-mips.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 00/14] MIPS: memblock: Switch arch code to NO_BOOTMEM

Hi Mathieu,

On 23/01/18 11:29, Mathieu Malaterre wrote:
> Hi Matt,
> 
> On Mon, Jan 22, 2018 at 5:36 PM, Matt Redfearn <matt.redfearn@...s.com> wrote:
>>
>> Hi Serge,
>>
>>
>> On 17/01/18 22:22, Serge Semin wrote:
>>>
>>> Even though it's common to see the architecture code using both
>>> bootmem and memblock early memory allocators, it's not good for
>>> multiple reasons. First of all, it's redundant to have two
>>> early memory allocator while one would be more than enough from
>>> functionality and stability points of view. Secondly, some new
>>> features introduced in the kernel utilize the methods of the most
>>> modern allocator ignoring the older one. It means the architecture
>>> code must keep the both subsystems up synchronized with information
>>> about memory regions and reservations, which leads to the code
>>> complexity increase, that obviously increases bugs probability.
>>> Finally it's better to keep all the architectures code unified for
>>> better readability and code simplification. All these reasons lead
>>> to one conclusion - arch code should use just one memory allocator,
>>> which is supposed to be memblock as the most modern and already
>>> utilized by the most of the kernel platforms. This patchset is
>>> mostly about it.
>>>
>>> One more reason why the MIPS arch code should finally move to
>>> memblock is a BUG somewhere in the initialization process, when
>>> CMA is activated:
>>>
>>> [    0.248762] BUG: Bad page state in process swapper/0  pfn:01f93
>>> [    0.255415] page:8205b0ac count:0 mapcount:-127 mapping:  (null) index:0x1
>>> [    0.263172] flags: 0x40000000()
>>> [    0.266723] page dumped because: nonzero mapcount
>>> [    0.272049] Modules linked in:
>>> [    0.275511] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.4.88-module #5
>>> [    0.282900] Stack : 00000000 00000000 80b6dd6a 0000003a 00000000 00000000 80930000 8092bff4
>>>             86073a14 80ac88c7 809f21ac 00000000 00000001 80b6998c 00000400 00000000
>>>             80a00000 801822e8 80b6dd68 00000000 00000002 00000000 809f8024 86077ccc
>>>             80b80000 801e9328 809fcbc0 00000000 00000400 00010000 86077ccc 86073a14
>>>             00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>>             ...
>>> [    0.323148] Call Trace:
>>> [    0.325935] [<8010e7c4>] show_stack+0x8c/0xa8
>>> [    0.330859] [<80404814>] dump_stack+0xd4/0x110
>>> [    0.335879] [<801f0bc0>] bad_page+0xfc/0x14c
>>> [    0.340710] [<801f0e04>] free_pages_prepare+0x1f4/0x330
>>> [    0.346632] [<801f36c4>] __free_pages_ok+0x2c/0x104
>>> [    0.352154] [<80b23a40>] init_cma_reserved_pageblock+0x5c/0x74
>>> [    0.358761] [<80b29390>] cma_init_reserved_areas+0x1b4/0x240
>>> [    0.365170] [<8010058c>] do_one_initcall+0xe8/0x27c
>>> [    0.370697] [<80b14e60>] kernel_init_freeable+0x200/0x2c4
>>> [    0.376828] [<808faca4>] kernel_init+0x14/0x104
>>> [    0.381939] [<80107598>] ret_from_kernel_thread+0x14/0x1c
>>>
>>> The bugus pfn seems to be the one allocated for bootmem allocator
>>> pages and hasn't been freed before letting the CMA working with its
>>> areas. Anyway the bug is solved by this patchset.
>>>
>>> Another reason why this patchset is useful is that it fixes the fdt
>>> reserved-memory nodes functionality for MIPS. Really it's bug to have
>>> the fdt reserved nodes scanning before the memblock is
>>> fully initialized (calling early_init_fdt_scan_reserved_mem before
>>> bootmem_init is called). Additionally no-map flag of the
>>> reserved-memory node hasn't been taking into account. This patchset
>>> fixes all of these.
>>>
>>> As you probably remember I already did another attempt to merge a
>>> similar functionality into the kernel. This time the patchset got
>>> to be less complex (14 patches vs 21 last time) and fixes the
>>> platform code like SGI IP27 and Loongson3, which due to being
>>> NUMA introduce its own memory initialization process. Although
>>> I have much doubt in SGI IP27 code operability in the first place,
>>> since it got prom_meminit() method of early memory initialization,
>>> which hasn't been called at any other place in the kernel. It must
>>> have been left there unrenamed after arch/mips/mips-boards/generic
>>> code had been discarded.
>>>
>>> Here are the list of folks, who agreed to perform some tests of
>>> the patchset:
>>> Alexander Sverdlin <alexander.sverdlin@...ia.com> - Octeon2
>>> Matt Redfearn <matt.redfearn@...s.com> - Loongson3, etc
>>
>>
>>
>> I have applied and tested these patches on various platforms that we have available here, and the kernel appears to boot and get to userspace as normal on the following platforms:
>>
>> UTM8 (Cavium Octeon III)
>> Creator CI20
> 
> A bit off-topic, but could you please Acked one of Marcin's patch that
> I re-submitted:
> 
> https://patchwork.linux-mips.org/patch/17986/
> 
> I believe CI20 wont boot from upstream kernel, since you are testing
> patch I am guessing you have a running system (unless of course you
> tweaked your u-boot env vars or use another different patch).
> 

Indeed, with the factory default arguments the kernel does not boot 
since 73fbc1eba7ff. Those arguments are redundant though since the 
memory is discovered via device tree. Testing locally we do not use 
those arguments. Anyway, the patch mentioned does not seem to break 
anything and does fix this issue so I've replied with my Tested-by.

Thanks,
Matt



> Thanks for your help
> 

Powered by blists - more mailing lists