Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210107092652.3438696-1-atish.patra@wdc.com> <20210107092652.3438696-2-atish.patra@wdc.com>
In-Reply-To: <20210107092652.3438696-2-atish.patra@wdc.com>
From:   Anup Patel <anup@brainfault.org>
Date:   Mon, 11 Jan 2021 09:24:52 +0530
Message-ID: <CAAhSdy1h3ALfg5VVC8_Tq24mji=4Ds9DByqwmE2A3b_Kq2ZphQ@mail.gmail.com>
Subject: Re: [PATCH 1/4] RISC-V: Do not allocate memblock while iterating
 reserved memblocks
To:     Atish Patra <atish.patra@wdc.com>
Cc:     "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Anup Patel <anup.patel@wdc.com>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Nick Kossifidis <mick@ics.forth.gr>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Mike Rapoport <rppt@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> wrote:
>
> Currently, resource tree allocates memory blocks while iterating on the
> list. It leads to following kernel warning because memblock allocation
> also invokes memory block reservation API.
>
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] WARNING: CPU: 0 PID: 0 at kernel/resource.c:795
> __insert_resource+0x8e/0xd0
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted
> 5.10.0-00022-ge20097fb37e2-dirty #549
> [    0.000000] epc: c00125c2 ra : c001262c sp : c1c01f50
> [    0.000000]  gp : c1d456e0 tp : c1c0a980 t0 : ffffcf20
> [    0.000000]  t1 : 00000000 t2 : 00000000 s0 : c1c01f60
> [    0.000000]  s1 : ffffcf00 a0 : ffffff00 a1 : c1c0c0c4
> [    0.000000]  a2 : 80c12b15 a3 : 80402000 a4 : 80402000
> [    0.000000]  a5 : c1c0c0c4 a6 : 80c12b15 a7 : f5faf600
> [    0.000000]  s2 : c1c0c0c4 s3 : c1c0e000 s4 : c1009a80
> [    0.000000]  s5 : c1c0c000 s6 : c1d48000 s7 : c1613b4c
> [    0.000000]  s8 : 00000fff s9 : 80000200 s10: c1613b40
> [    0.000000]  s11: 00000000 t3 : c1d4a000 t4 : ffffffff
>
> This is also unnecessary as we can pre-compute the total memblocks required
> for each memory region and allocate it before the loop. It save precious
> boot time not going through memblock allocation code every time.
>
> Fixes: 00ab027a3b82 ("RISC-V: Add kernel image sections to the resource tree")
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

> ---
>  arch/riscv/kernel/setup.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 1d85e9bf783c..3fa3f26dde85 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -127,7 +127,9 @@ static void __init init_resources(void)
>  {
>         struct memblock_region *region = NULL;
>         struct resource *res = NULL;
> -       int ret = 0;
> +       struct resource *mem_res = NULL;
> +       size_t mem_res_sz = 0;
> +       int ret = 0, i = 0;
>
>         code_res.start = __pa_symbol(_text);
>         code_res.end = __pa_symbol(_etext) - 1;
> @@ -145,16 +147,17 @@ static void __init init_resources(void)
>         bss_res.end = __pa_symbol(__bss_stop) - 1;
>         bss_res.flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>
> +       mem_res_sz = (memblock.memory.cnt + memblock.reserved.cnt) * sizeof(*mem_res);
> +       mem_res = memblock_alloc(mem_res_sz, SMP_CACHE_BYTES);
> +       if (!mem_res)
> +               panic("%s: Failed to allocate %zu bytes\n", __func__, mem_res_sz);
>         /*
>          * Start by adding the reserved regions, if they overlap
>          * with /memory regions, insert_resource later on will take
>          * care of it.
>          */
>         for_each_reserved_mem_region(region) {
> -               res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
> -               if (!res)
> -                       panic("%s: Failed to allocate %zu bytes\n", __func__,
> -                             sizeof(struct resource));
> +               res = &mem_res[i++];
>
>                 res->name = "Reserved";
>                 res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> @@ -171,8 +174,10 @@ static void __init init_resources(void)
>                  * Ignore any other reserved regions within
>                  * system memory.
>                  */
> -               if (memblock_is_memory(res->start))
> +               if (memblock_is_memory(res->start)) {
> +                       memblock_free((phys_addr_t) res, sizeof(struct resource));
>                         continue;
> +               }
>
>                 ret = add_resource(&iomem_resource, res);
>                 if (ret < 0)
> @@ -181,10 +186,7 @@ static void __init init_resources(void)
>
>         /* Add /memory regions to the resource tree */
>         for_each_mem_region(region) {
> -               res = memblock_alloc(sizeof(struct resource), SMP_CACHE_BYTES);
> -               if (!res)
> -                       panic("%s: Failed to allocate %zu bytes\n", __func__,
> -                             sizeof(struct resource));
> +               res = &mem_res[i++];
>
>                 if (unlikely(memblock_is_nomap(region))) {
>                         res->name = "Reserved";
> @@ -205,9 +207,9 @@ static void __init init_resources(void)
>         return;
>
>   error:
> -       memblock_free((phys_addr_t) res, sizeof(struct resource));
>         /* Better an empty resource tree than an inconsistent one */
>         release_child_resources(&iomem_resource);
> +       memblock_free((phys_addr_t) mem_res, mem_res_sz);
>  }
>
>
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup