From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B7497C10F13 for ; Thu, 11 Apr 2019 12:18:12 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 828172133D for ; Thu, 11 Apr 2019 12:18:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="fHGJb2lE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 828172133D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=N0rJGFumm3e0evEZpqZ5JxBjSxHTTqucNDrhtVC47aQ=; b=fHGJb2lE+CPEwn Dn+2haLtFGOjuXOcNomU39rhf0d4lgEwdW7D0U92UHzlZUotzvAPxv4xTAP4VgikRLI1yEOd0ekOx HUkmoM9b4kNvtS59o3VuEM2lKrU0x0feFWkgA8PrYEbDL76aIUT6EvIpzlg3EaKaxLq+dw4FWZVvP E3BHl06kFVshCHNYRP85+NAmTO/MdWrR/Ow6k8tdXL3f6qS9l5qAwvdt3Zji0AyuTvQx9MF8F25wL PbKCykuomndGAcsMAhw7gNq+OxS2WWRZ/Yp7h35HypOcCfSNVELCOLjkDee1+ufjM+65yUaIZbYdW /Wu6DlNH4iGXGgxehOaA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hEYeh-0007XC-NQ; Thu, 11 Apr 2019 12:18:11 +0000 Received: from szxga06-in.huawei.com ([45.249.212.32] helo=huawei.com) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1hEYec-0007VQ-HP; Thu, 11 Apr 2019 12:18:09 +0000 Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id B924959CB1E64FCBE285; Thu, 11 Apr 2019 20:17:56 +0800 (CST) Received: from [127.0.0.1] (10.177.131.64) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.408.0; Thu, 11 Apr 2019 20:17:46 +0800 Subject: Re: [PATCH v3 3/4] arm64: kdump: support more than one crash kernel regions To: Mike Rapoport References: <20190409102819.121335-1-chenzhou10@huawei.com> <20190409102819.121335-4-chenzhou10@huawei.com> <20190410130917.GC17196@rapoport-lnx> From: Chen Zhou Message-ID: <137bef2e-8726-fd8f-1cb0-7592074f7870@huawei.com> Date: Thu, 11 Apr 2019 20:17:43 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <20190410130917.GC17196@rapoport-lnx> X-Originating-IP: [10.177.131.64] X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190411_051806_939159_7EC60904 X-CRM114-Status: GOOD ( 26.66 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: wangkefeng.wang@huawei.com, horms@verge.net.au, ard.biesheuvel@linaro.org, catalin.marinas@arm.com, will.deacon@arm.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, takahiro.akashi@linaro.org, mingo@redhat.com, bp@alien8.de, ebiederm@xmission.com, kexec@lists.infradead.org, tglx@linutronix.de, akpm@linux-foundation.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Mike, This overall looks well. Replacing memblock_cap_memory_range() with memblock_cap_memory_ranges() was= what i wanted to do in v1, sorry for don't express that clearly. But there are some issues as below. After fixing this, it can work correctl= y. On 2019/4/10 21:09, Mike Rapoport wrote: > Hi, > = > On Tue, Apr 09, 2019 at 06:28:18PM +0800, Chen Zhou wrote: >> After commit (arm64: kdump: support reserving crashkernel above 4G), >> there may be two crash kernel regions, one is below 4G, the other is >> above 4G. >> >> Crash dump kernel reads more than one crash kernel regions via a dtb >> property under node /chosen, >> linux,usable-memory-range =3D >> >> Signed-off-by: Chen Zhou >> --- >> arch/arm64/mm/init.c | 66 ++++++++++++++++++++++++++++++++++++++++-= ------- >> include/linux/memblock.h | 6 +++++ >> mm/memblock.c | 7 ++--- >> 3 files changed, 66 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 3bebddf..0f18665 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -65,6 +65,11 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init; >> = >> #ifdef CONFIG_KEXEC_CORE >> = >> +/* at most two crash kernel regions, low_region and high_region */ >> +#define CRASH_MAX_USABLE_RANGES 2 >> +#define LOW_REGION_IDX 0 >> +#define HIGH_REGION_IDX 1 >> + >> /* >> * reserve_crashkernel() - reserves memory for crash kernel >> * >> @@ -297,8 +302,8 @@ static int __init early_init_dt_scan_usablemem(unsig= ned long node, >> const char *uname, int depth, void *data) >> { >> struct memblock_region *usablemem =3D data; >> - const __be32 *reg; >> - int len; >> + const __be32 *reg, *endp; >> + int len, nr =3D 0; >> = >> if (depth !=3D 1 || strcmp(uname, "chosen") !=3D 0) >> return 0; >> @@ -307,22 +312,63 @@ static int __init early_init_dt_scan_usablemem(uns= igned long node, >> if (!reg || (len < (dt_root_addr_cells + dt_root_size_cells))) >> return 1; >> = >> - usablemem->base =3D dt_mem_next_cell(dt_root_addr_cells, ®); >> - usablemem->size =3D dt_mem_next_cell(dt_root_size_cells, ®); >> + endp =3D reg + (len / sizeof(__be32)); >> + while ((endp - reg) >=3D (dt_root_addr_cells + dt_root_size_cells)) { >> + usablemem[nr].base =3D dt_mem_next_cell(dt_root_addr_cells, ®); >> + usablemem[nr].size =3D dt_mem_next_cell(dt_root_size_cells, ®); >> + >> + if (++nr >=3D CRASH_MAX_USABLE_RANGES) >> + break; >> + } >> = >> return 1; >> } >> = >> static void __init fdt_enforce_memory_region(void) >> { >> - struct memblock_region reg =3D { >> - .size =3D 0, >> - }; >> + int i, cnt =3D 0; >> + struct memblock_region regs[CRASH_MAX_USABLE_RANGES]; > = > I only now noticed that fdt_enforce_memory_region() uses memblock_region = to > pass the ranges around. If we'd switch to memblock_type instead, the > implementation of memblock_cap_memory_ranges() would be really > straightforward. Can you check if the below patch works for you? = > = >>>From e476d584098e31273af573e1a78e308880c5cf28 Mon Sep 17 00:00:00 2001 > From: Mike Rapoport > Date: Wed, 10 Apr 2019 16:02:32 +0300 > Subject: [PATCH] memblock: extend memblock_cap_memory_range to multiple r= anges > = > The memblock_cap_memory_range() removes all the memory except the range > passed to it. Extend this function to recieve memblock_type with the > regions that should be kept. This allows switching to simple iteration ov= er > memblock arrays with 'for_each_mem_range' to remove the unneeded memory. > = > Enable use of this function in arm64 for reservation of multile regions f= or > the crash kernel. > = > Signed-off-by: Mike Rapoport > --- > arch/arm64/mm/init.c | 34 ++++++++++++++++++++++++---------- > include/linux/memblock.h | 2 +- > mm/memblock.c | 45 ++++++++++++++++++++++--------------------= --- > 3 files changed, 47 insertions(+), 34 deletions(-) > = > = > -void __init memblock_cap_memory_range(phys_addr_t base, phys_addr_t size) > +void __init memblock_cap_memory_ranges(struct memblock_type *regions_to_= keep) > { > - int start_rgn, end_rgn; > - int i, ret; > - > - if (!size) > - return; > - > - ret =3D memblock_isolate_range(&memblock.memory, base, size, > - &start_rgn, &end_rgn); > - if (ret) > - return; > - > - /* remove all the MAP regions */ > - for (i =3D memblock.memory.cnt - 1; i >=3D end_rgn; i--) > - if (!memblock_is_nomap(&memblock.memory.regions[i])) > - memblock_remove_region(&memblock.memory, i); > + phys_addr_t start, end; > + u64 i; > = > - for (i =3D start_rgn - 1; i >=3D 0; i--) > - if (!memblock_is_nomap(&memblock.memory.regions[i])) > - memblock_remove_region(&memblock.memory, i); > + /* truncate memory while skipping NOMAP regions */ > + for_each_mem_range(i, &memblock.memory, regions_to_keep, NUMA_NO_NODE, > + MEMBLOCK_NONE, &start, &end, NULL) > + memblock_remove(start, end); 1. use memblock_remove(start, size) instead of memblock_remove(start, end). 2. There is a another hidden issue. We couldn't mix __next_mem_range()(call= ed by for_each_mem_range) operation with remove operation because __next_mem_range() records the index of last = time. If we do remove between __next_mem_range(), the index may be mess. Therefore, we could do remove operation after for_each_mem_range like this,= solution A: void __init memblock_cap_memory_ranges(struct memblock_type *regions_to_ke= ep) { - phys_addr_t start, end; - u64 i; + phys_addr_t start[INIT_MEMBLOCK_RESERVED_REGIONS * 2]; + phys_addr_t end[INIT_MEMBLOCK_RESERVED_REGIONS * 2]; + u64 i, nr =3D 0; /* truncate memory while skipping NOMAP regions */ for_each_mem_range(i, &memblock.memory, regions_to_keep, NUMA_NO_NODE, - MEMBLOCK_NONE, &start, &end, NULL) - memblock_remove(start, end); + MEMBLOCK_NONE, &start[nr], &end[nr], NULL) + nr++; + for (i =3D 0; i < nr; i++) + memblock_remove(start[i], end[i] - start[i]); /* truncate the reserved regions */ + nr =3D 0; for_each_mem_range(i, &memblock.reserved, regions_to_keep, NUMA_NO_NODE, - MEMBLOCK_NONE, &start, &end, NULL) - memblock_remove_range(&memblock.reserved, start, end); + MEMBLOCK_NONE, &start[nr], &end[nr], NULL) + nr++; + for (i =3D 0; i < nr; i++) + memblock_remove_range(&memblock.reserved, start[i], + end[i] - start[i]); } But a warning occurs when compiling: CALL scripts/atomic/check-atomics.sh CALL scripts/checksyscalls.sh CHK include/generated/compile.h CC mm/memblock.o mm/memblock.c: In function =91memblock_cap_memory_ranges=92: mm/memblock.c:1635:1: warning: the frame size of 36912 bytes is larger than= 2048 bytes [-Wframe-larger-than=3D] } another solution is my implementation in v1, solution B: +void __init memblock_cap_memory_ranges(struct memblock_type *regions_to_ke= ep) +{ + int start_rgn[INIT_MEMBLOCK_REGIONS], end_rgn[INIT_MEMBLOCK_REGIONS]; + int i, j, ret, nr =3D 0; + memblock_region *regs =3D regions_to_keep->regions; + + nr =3D regions_to_keep -> cnt; + if (!nr) + return; + + /* remove all the MAP regions */ + for (i =3D memblock.memory.cnt - 1; i >=3D end_rgn[nr - 1]; i--) + if (!memblock_is_nomap(&memblock.memory.regions[i])) + memblock_remove_region(&memblock.memory, i); + + for (i =3D nr - 1; i > 0; i--) + for (j =3D start_rgn[i] - 1; j >=3D end_rgn[i - 1]; j--) + if (!memblock_is_nomap(&memblock.memory.regions[j])) + memblock_remove_region(&memblock.memory, j); + + for (i =3D start_rgn[0] - 1; i >=3D 0; i--) + if (!memblock_is_nomap(&memblock.memory.regions[i])) + memblock_remove_region(&memblock.memory, i); + + /* truncate the reserved regions */ + memblock_remove_range(&memblock.reserved, 0, regs[0].base); + + for (i =3D nr - 1; i > 0; i--) + memblock_remove_range(&memblock.reserved, + regs[i - 1].base + regs[i - 1].size, + regs[i].base - regs[i - 1].base - regs[i - 1].size); + + memblock_remove_range(&memblock.reserved, + regs[nr - 1].base + regs[nr - 1].size, PHYS_ADDR_MAX); +} solution A: phys_addr_t start[INIT_MEMBLOCK_RESERVED_REGIONS * 2]; phys_addr_t end[INIT_MEMBLOCK_RESERVED_REGIONS * 2]; start, end is physical addr solution B: int start_rgn[INIT_MEMBLOCK_REGIONS], end_rgn[INIT_MEMBLOCK_RE= GIONS]; start_rgn, end_rgn is rgn index = Solution B do less remove operations and with no warning comparing to solut= ion A. I think solution B is better, could you give some suggestions? > = > /* truncate the reserved regions */ > - memblock_remove_range(&memblock.reserved, 0, base); > - memblock_remove_range(&memblock.reserved, > - base + size, PHYS_ADDR_MAX); > + for_each_mem_range(i, &memblock.reserved, regions_to_keep, NUMA_NO_NODE, > + MEMBLOCK_NONE, &start, &end, NULL) > + memblock_remove_range(&memblock.reserved, start, end); There are the same issues as above. > } > = > void __init memblock_mem_limit_remove_map(phys_addr_t limit) > { > + struct memblock_region rgn =3D { > + .base =3D 0, > + }; > + > + struct memblock_type region_to_keep =3D { > + .cnt =3D 1, > + .max =3D 1, > + .regions =3D &rgn, > + }; > + > phys_addr_t max_addr; > = > if (!limit) > @@ -1646,7 +1644,8 @@ void __init memblock_mem_limit_remove_map(phys_addr= _t limit) > if (max_addr =3D=3D PHYS_ADDR_MAX) > return; > = > - memblock_cap_memory_range(0, max_addr); > + region_to_keep.regions[0].size =3D max_addr; > + memblock_cap_memory_ranges(®ion_to_keep); > } > = > static int __init_memblock memblock_search(struct memblock_type *type, p= hys_addr_t addr) > = Thanks, Chen Zhou _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel