Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Tue, 12 Apr 2022 20:56:19 +0300
From:   Mike Rapoport <rppt@kernel.org>
To:     Michael Ellerman <mpe@ellerman.id.au>
Cc:     Christophe Leroy <christophe.leroy@csgroup.eu>,
        Ariel Marcovitch <arielmarcovitch@gmail.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "benh@kernel.crashing.org" <benh@kernel.crashing.org>,
        "paulus@samba.org" <paulus@samba.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: False positive kmemleak report for dtb properties names on
 powerpc
Message-ID: <YlW9Q05HDWwSmr7l@kernel.org>
References: <9dd08bb5-f39e-53d8-f88d-bec598a08c93@gmail.com>
 <YjtXXlnbEp64eL0T@arm.com>
 <Yjtvtkn+CishCef6@kernel.org>
 <2603cae9-3b75-cd13-1d41-2f1bed6ca32e@gmail.com>
 <a9d13878-7820-d41c-9357-135094c8357f@csgroup.eu>
 <87pmlm6bn0.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87pmlm6bn0.fsf@mpe.ellerman.id.au>
Precedence: bulk

On Tue, Apr 12, 2022 at 04:47:47PM +1000, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> > Hi Ariel
> >
> > Le 09/04/2022 ? 15:47, Ariel Marcovitch a ?crit?:
> >> Hi Christophe, did you get the chance to look at this?
> >
> > I tested something this morning, it works for me, see below
> >
> >> 
> >> On 23/03/2022 21:06, Mike Rapoport wrote:
> >>> Hi Catalin,
> >>>
> >>> On Wed, Mar 23, 2022 at 05:22:38PM +0000, Catalin Marinas wrote:
> >>>> Hi Ariel,
> >>>>
> >>>> On Fri, Feb 18, 2022 at 09:45:51PM +0200, Ariel Marcovitch wrote:
> >>>>> I was running a powerpc 32bit kernel (built using
> >>>>> qemu_ppc_mpc8544ds_defconfig
> >>>>> buildroot config, with enabling DEBUGFS+KMEMLEAK+HIGHMEM in the kernel
> >>>>> config)
> >
> > ...
> >
> >>>>> I don't suppose I can just shuffle the calls in setup_arch() around, 
> >>>>> so I
> >>>>> wanted to hear your opinions first
> >>>> I think it's better if we change the logic than shuffling the calls.
> >>>> IIUC MEMBLOCK_ALLOC_ACCESSIBLE means that __va() works on the phys
> >>>> address return by memblock, so something like below (untested):
> >>> MEMBLOCK_ALLOC_ACCESSIBLE means "anywhere", see commit e63075a3c937
> >>> ("memblock: Introduce default allocation limit and use it to replace
> >>> explicit ones"), so it won't help to detect high memory.
> >>>
> >>> If I remember correctly, ppc initializes memblock *very* early, so 
> >>> setting
> >>> max_low_pfn along with lowmem_end_addr in
> >>> arch/powerpc/mm/init_32::MMU_init() makes sense to me.
> >>>
> >>> Maybe ppc folks have other ideas...
> >>> I've added Christophe who works on ppc32 these days.
> >
> > I think memblock is already available at the end of MMU_init() on PPC32 
> > and at the end of early_setup() on PPC64. It means it is ready when we 
> > enter setup_arch().
> >
> > I tested the change below, it works for me, I don't get any kmemleak 
> > report anymore.
> >
> > diff --git a/arch/powerpc/kernel/setup-common.c 
> > b/arch/powerpc/kernel/setup-common.c
> > index 518ae5aa9410..9f4e50b176c9 100644
> > --- a/arch/powerpc/kernel/setup-common.c
> > +++ b/arch/powerpc/kernel/setup-common.c
> > @@ -840,6 +840,9 @@ void __init setup_arch(char **cmdline_p)
> >   	/* Set a half-reasonable default so udelay does something sensible */
> >   	loops_per_jiffy = 500000000 / HZ;
> >
> > +	/* Parse memory topology */
> > +	mem_topology_setup();
> > +
> >   	/* Unflatten the device-tree passed by prom_init or kexec */
> >   	unflatten_device_tree();
> 
> The 64-bit/NUMA version of mem_topology_setup() requires the device tree
> to be unflattened, so I don't think that can work.
> 
> Setting max_low_pfn etc in MMU_init() as Mike suggested seems more
> likely to work.
> 
> But we might need to set it again in mem_topology_setup() though, so
> that things that change memblock_end_of_DRAM() are reflected, eg. memory
> limit or crash dump?

I don't think this can cause issues for kmemleak Ariel reported. The
kmemleak checks if there is a linear mapping for a PFN or that PFN is only
accessible via HIGHMEM. Memory limit or crash dump won't change the split,
or am I missing something?
 
> cheers

-- 
Sincerely yours,
Mike.