* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop [not found] ` <ZqywtegyIS/YXOVv@MiWiFi-R3L-srv> @ 2024-08-06 19:10 ` Catalin Marinas 2024-08-06 19:34 ` Catalin Marinas 2024-08-07 1:40 ` Jinjie Ruan 0 siblings, 2 replies; 10+ messages in thread From: Catalin Marinas @ 2024-08-06 19:10 UTC (permalink / raw) To: Baoquan He Cc: Jinjie Ruan, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon To Jinjie, if you make generic changes that affect other architectures, please either cc the individual lists/maintainers or at least cross-post to linux-arch. I don't follow lkml, there's just too much traffic there. On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: > On 08/02/24 at 05:01pm, Jinjie Ruan wrote: > > On RISCV64 Qemu machine with 512MB memory, cmdline "crashkernel=500M,high" > > will cause system stall as below: > > > > Zone ranges: > > DMA32 [mem 0x0000000080000000-0x000000009fffffff] > > Normal empty > > Movable zone start for each node > > Early memory node ranges > > node 0: [mem 0x0000000080000000-0x000000008005ffff] > > node 0: [mem 0x0000000080060000-0x000000009fffffff] > > Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff] > > (stall here) > > > > commit 5d99cadf1568 ("crash: fix x86_32 crash memory reserve dead loop > > bug") fix this on 32-bit architecture. However, the problem is not > > completely solved. If `CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX` on 64-bit > > architecture, for example, when system memory is equal to > > CRASH_ADDR_LOW_MAX on RISCV64, the following infinite loop will also occur: > > Interesting, I didn't expect risc-v defining them like these. > > #define CRASH_ADDR_LOW_MAX dma32_phys_limit > #define CRASH_ADDR_HIGH_MAX memblock_end_of_DRAM() arm64 defines the high limit as PHYS_MASK+1, it doesn't need to be dynamic and x86 does something similar (SZ_64T). Not sure why the generic code and riscv define it like this. > > -> reserve_crashkernel_generic() and high is true > > -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail > > -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly > > (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX). > > > > Before refactor in commit 9c08a2a139fe ("x86: kdump: use generic interface > > to simplify crashkernel reservation code"), x86 do not try to reserve crash > > memory at low if it fails to alloc above high 4G. However before refator in > > commit fdc268232dbba ("arm64: kdump: use generic interface to simplify > > crashkernel reservation"), arm64 try to reserve crash memory at low if it > > fails above high 4G. For 64-bit systems, this attempt is less beneficial > > than the opposite, remove it to fix this bug and align with native x86 > > implementation. > > And I don't like the idea crashkernel=,high failure will fallback to > attempt in low area, so this looks good to me. Well, I kind of liked this behaviour. One can specify ,high as a preference rather than forcing a range. The arm64 land has different platforms with some constrained memory layouts. Such fallback works well as a default command line option shipped with distros without having to guess the SoC memory layout. Something like below should fix the issue as well (untested): diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c index d3b4cd12bdd1..ae92d6745ef4 100644 --- a/kernel/crash_reserve.c +++ b/kernel/crash_reserve.c @@ -420,7 +420,8 @@ void __init reserve_crashkernel_generic(char *cmdline, * For crashkernel=size[KMG],high, if the first attempt was * for high memory, fall back to low memory. */ - if (high && search_end == CRASH_ADDR_HIGH_MAX) { + if (high && search_end == CRASH_ADDR_HIGH_MAX && + CRASH_ADDR_LOW_MAX < CRASH_ADDR_HIGH_MAX) { search_end = CRASH_ADDR_LOW_MAX; search_base = 0; goto retry; -- Catalin ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-06 19:10 ` [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop Catalin Marinas @ 2024-08-06 19:34 ` Catalin Marinas 2024-08-08 7:56 ` Jinjie Ruan 2024-08-13 8:40 ` Petr Tesařík 2024-08-07 1:40 ` Jinjie Ruan 1 sibling, 2 replies; 10+ messages in thread From: Catalin Marinas @ 2024-08-06 19:34 UTC (permalink / raw) To: Baoquan He Cc: Jinjie Ruan, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On Tue, Aug 06, 2024 at 08:10:30PM +0100, Catalin Marinas wrote: > On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: > > On 08/02/24 at 05:01pm, Jinjie Ruan wrote: > > > On RISCV64 Qemu machine with 512MB memory, cmdline "crashkernel=500M,high" > > > will cause system stall as below: > > > > > > Zone ranges: > > > DMA32 [mem 0x0000000080000000-0x000000009fffffff] > > > Normal empty > > > Movable zone start for each node > > > Early memory node ranges > > > node 0: [mem 0x0000000080000000-0x000000008005ffff] > > > node 0: [mem 0x0000000080060000-0x000000009fffffff] > > > Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff] > > > (stall here) > > > > > > commit 5d99cadf1568 ("crash: fix x86_32 crash memory reserve dead loop > > > bug") fix this on 32-bit architecture. However, the problem is not > > > completely solved. If `CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX` on 64-bit > > > architecture, for example, when system memory is equal to > > > CRASH_ADDR_LOW_MAX on RISCV64, the following infinite loop will also occur: > > > > Interesting, I didn't expect risc-v defining them like these. > > > > #define CRASH_ADDR_LOW_MAX dma32_phys_limit > > #define CRASH_ADDR_HIGH_MAX memblock_end_of_DRAM() > > arm64 defines the high limit as PHYS_MASK+1, it doesn't need to be > dynamic and x86 does something similar (SZ_64T). Not sure why the > generic code and riscv define it like this. > > > > -> reserve_crashkernel_generic() and high is true > > > -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail > > > -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly > > > (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX). > > > > > > Before refactor in commit 9c08a2a139fe ("x86: kdump: use generic interface > > > to simplify crashkernel reservation code"), x86 do not try to reserve crash > > > memory at low if it fails to alloc above high 4G. However before refator in > > > commit fdc268232dbba ("arm64: kdump: use generic interface to simplify > > > crashkernel reservation"), arm64 try to reserve crash memory at low if it > > > fails above high 4G. For 64-bit systems, this attempt is less beneficial > > > than the opposite, remove it to fix this bug and align with native x86 > > > implementation. > > > > And I don't like the idea crashkernel=,high failure will fallback to > > attempt in low area, so this looks good to me. > > Well, I kind of liked this behaviour. One can specify ,high as a > preference rather than forcing a range. The arm64 land has different > platforms with some constrained memory layouts. Such fallback works well > as a default command line option shipped with distros without having to > guess the SoC memory layout. I haven't tried but it's possible that this patch also breaks those arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is memblock_end_of_DRAM(). Here all memory would be low and in the absence of no fallback, it fails to allocate. So, my strong preference would be to re-instate the current behaviour and work around the infinite loop in a different way. Thanks. -- Catalin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-06 19:34 ` Catalin Marinas @ 2024-08-08 7:56 ` Jinjie Ruan 2024-08-09 1:56 ` Baoquan He 2024-08-09 9:51 ` Catalin Marinas 2024-08-13 8:40 ` Petr Tesařík 1 sibling, 2 replies; 10+ messages in thread From: Jinjie Ruan @ 2024-08-08 7:56 UTC (permalink / raw) To: Catalin Marinas, Baoquan He Cc: vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On 2024/8/7 3:34, Catalin Marinas wrote: > On Tue, Aug 06, 2024 at 08:10:30PM +0100, Catalin Marinas wrote: >> On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: >>> On 08/02/24 at 05:01pm, Jinjie Ruan wrote: >>>> On RISCV64 Qemu machine with 512MB memory, cmdline "crashkernel=500M,high" >>>> will cause system stall as below: >>>> >>>> Zone ranges: >>>> DMA32 [mem 0x0000000080000000-0x000000009fffffff] >>>> Normal empty >>>> Movable zone start for each node >>>> Early memory node ranges >>>> node 0: [mem 0x0000000080000000-0x000000008005ffff] >>>> node 0: [mem 0x0000000080060000-0x000000009fffffff] >>>> Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff] >>>> (stall here) >>>> >>>> commit 5d99cadf1568 ("crash: fix x86_32 crash memory reserve dead loop >>>> bug") fix this on 32-bit architecture. However, the problem is not >>>> completely solved. If `CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX` on 64-bit >>>> architecture, for example, when system memory is equal to >>>> CRASH_ADDR_LOW_MAX on RISCV64, the following infinite loop will also occur: >>> >>> Interesting, I didn't expect risc-v defining them like these. >>> >>> #define CRASH_ADDR_LOW_MAX dma32_phys_limit >>> #define CRASH_ADDR_HIGH_MAX memblock_end_of_DRAM() >> >> arm64 defines the high limit as PHYS_MASK+1, it doesn't need to be >> dynamic and x86 does something similar (SZ_64T). Not sure why the >> generic code and riscv define it like this. >> >>>> -> reserve_crashkernel_generic() and high is true >>>> -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail >>>> -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly >>>> (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX). >>>> >>>> Before refactor in commit 9c08a2a139fe ("x86: kdump: use generic interface >>>> to simplify crashkernel reservation code"), x86 do not try to reserve crash >>>> memory at low if it fails to alloc above high 4G. However before refator in >>>> commit fdc268232dbba ("arm64: kdump: use generic interface to simplify >>>> crashkernel reservation"), arm64 try to reserve crash memory at low if it >>>> fails above high 4G. For 64-bit systems, this attempt is less beneficial >>>> than the opposite, remove it to fix this bug and align with native x86 >>>> implementation. >>> >>> And I don't like the idea crashkernel=,high failure will fallback to >>> attempt in low area, so this looks good to me. >> >> Well, I kind of liked this behaviour. One can specify ,high as a >> preference rather than forcing a range. The arm64 land has different >> platforms with some constrained memory layouts. Such fallback works well >> as a default command line option shipped with distros without having to >> guess the SoC memory layout. > > I haven't tried but it's possible that this patch also breaks those > arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is > memblock_end_of_DRAM(). Here all memory would be low and in the absence > of no fallback, it fails to allocate. > > So, my strong preference would be to re-instate the current behaviour > and work around the infinite loop in a different way. Hi, baoquan, What's your opinion? Only this patch should be re-instate or all the 3 dead loop fix patch? > > Thanks. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-08 7:56 ` Jinjie Ruan @ 2024-08-09 1:56 ` Baoquan He 2024-08-09 9:51 ` Catalin Marinas 1 sibling, 0 replies; 10+ messages in thread From: Baoquan He @ 2024-08-09 1:56 UTC (permalink / raw) To: Jinjie Ruan, Catalin Marinas Cc: vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On 08/08/24 at 03:56pm, Jinjie Ruan wrote: > > > On 2024/8/7 3:34, Catalin Marinas wrote: > > On Tue, Aug 06, 2024 at 08:10:30PM +0100, Catalin Marinas wrote: > >> On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: > >>> On 08/02/24 at 05:01pm, Jinjie Ruan wrote: > >>>> On RISCV64 Qemu machine with 512MB memory, cmdline "crashkernel=500M,high" > >>>> will cause system stall as below: > >>>> > >>>> Zone ranges: > >>>> DMA32 [mem 0x0000000080000000-0x000000009fffffff] > >>>> Normal empty > >>>> Movable zone start for each node > >>>> Early memory node ranges > >>>> node 0: [mem 0x0000000080000000-0x000000008005ffff] > >>>> node 0: [mem 0x0000000080060000-0x000000009fffffff] > >>>> Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff] > >>>> (stall here) > >>>> > >>>> commit 5d99cadf1568 ("crash: fix x86_32 crash memory reserve dead loop > >>>> bug") fix this on 32-bit architecture. However, the problem is not > >>>> completely solved. If `CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX` on 64-bit > >>>> architecture, for example, when system memory is equal to > >>>> CRASH_ADDR_LOW_MAX on RISCV64, the following infinite loop will also occur: > >>> > >>> Interesting, I didn't expect risc-v defining them like these. > >>> > >>> #define CRASH_ADDR_LOW_MAX dma32_phys_limit > >>> #define CRASH_ADDR_HIGH_MAX memblock_end_of_DRAM() > >> > >> arm64 defines the high limit as PHYS_MASK+1, it doesn't need to be > >> dynamic and x86 does something similar (SZ_64T). Not sure why the > >> generic code and riscv define it like this. > >> > >>>> -> reserve_crashkernel_generic() and high is true > >>>> -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail > >>>> -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly > >>>> (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX). > >>>> > >>>> Before refactor in commit 9c08a2a139fe ("x86: kdump: use generic interface > >>>> to simplify crashkernel reservation code"), x86 do not try to reserve crash > >>>> memory at low if it fails to alloc above high 4G. However before refator in > >>>> commit fdc268232dbba ("arm64: kdump: use generic interface to simplify > >>>> crashkernel reservation"), arm64 try to reserve crash memory at low if it > >>>> fails above high 4G. For 64-bit systems, this attempt is less beneficial > >>>> than the opposite, remove it to fix this bug and align with native x86 > >>>> implementation. > >>> > >>> And I don't like the idea crashkernel=,high failure will fallback to > >>> attempt in low area, so this looks good to me. > >> > >> Well, I kind of liked this behaviour. One can specify ,high as a > >> preference rather than forcing a range. The arm64 land has different > >> platforms with some constrained memory layouts. Such fallback works well > >> as a default command line option shipped with distros without having to > >> guess the SoC memory layout. > > > > I haven't tried but it's possible that this patch also breaks those > > arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is > > memblock_end_of_DRAM(). Here all memory would be low and in the absence > > of no fallback, it fails to allocate. > > > > So, my strong preference would be to re-instate the current behaviour > > and work around the infinite loop in a different way. > > Hi, baoquan, What's your opinion? > > Only this patch should be re-instate or all the 3 dead loop fix patch? I am not sure which way Catalin suggested to take. Hi Catalin, Could you say more words about your preference so that Jinjie can proceed accordingly? Thanks Baoquan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-08 7:56 ` Jinjie Ruan 2024-08-09 1:56 ` Baoquan He @ 2024-08-09 9:51 ` Catalin Marinas 2024-08-09 10:15 ` Jinjie Ruan 1 sibling, 1 reply; 10+ messages in thread From: Catalin Marinas @ 2024-08-09 9:51 UTC (permalink / raw) To: Jinjie Ruan Cc: Baoquan He, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On Thu, Aug 08, 2024 at 03:56:35PM +0800, Jinjie Ruan wrote: > On 2024/8/7 3:34, Catalin Marinas wrote: > > On Tue, Aug 06, 2024 at 08:10:30PM +0100, Catalin Marinas wrote: > >> On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: > >>> And I don't like the idea crashkernel=,high failure will fallback to > >>> attempt in low area, so this looks good to me. > >> > >> Well, I kind of liked this behaviour. One can specify ,high as a > >> preference rather than forcing a range. The arm64 land has different > >> platforms with some constrained memory layouts. Such fallback works well > >> as a default command line option shipped with distros without having to > >> guess the SoC memory layout. > > > > I haven't tried but it's possible that this patch also breaks those > > arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is > > memblock_end_of_DRAM(). Here all memory would be low and in the absence > > of no fallback, it fails to allocate. > > > > So, my strong preference would be to re-instate the current behaviour > > and work around the infinite loop in a different way. > > Hi, baoquan, What's your opinion? > > Only this patch should be re-instate or all the 3 dead loop fix patch? Only the riscv64 patch that that removes the ,high reservation fallback to ,low. From this series: https://lore.kernel.org/r/20240719095735.1912878-1-ruanjinjie@huawei.com/ the first two fixes look fine (x86_32). The third one (arm32), not sure why it's in the series called "crash: Fix x86_32 memory reserve dead loop bug". Does it fix a problem on arm32? Anyway, I'm not against it getting merged but I'm not maintaining arm32. If the first two patches could be merged for 6.11, I think the arm32 one is more of a 6.12 material (unless it does fix something). On the riscv64 patch removing the high->low fallback to avoid the infinite loop, I'd rather replace it with something similar to the x86_32 fix in the series above. I suggested something in the main if block but, looking at the x86_32 fix, for consistency, I think it would look better as something like: diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c index d3b4cd12bdd1..64d44a52c011 100644 --- a/kernel/crash_reserve.c +++ b/kernel/crash_reserve.c @@ -423,7 +423,8 @@ void __init reserve_crashkernel_generic(char *cmdline, if (high && search_end == CRASH_ADDR_HIGH_MAX) { search_end = CRASH_ADDR_LOW_MAX; search_base = 0; - goto retry; + if (search_end != CRASH_ADDR_HIGH_MAX) + goto retry; } pr_warn("cannot allocate crashkernel (size:0x%llx)\n", crash_size); In summary, just replace the riscv64 fix with something along the lines of the diff above (or pick whatever you prefer that still keeps the fallback). Thanks. -- Catalin ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-09 9:51 ` Catalin Marinas @ 2024-08-09 10:15 ` Jinjie Ruan 0 siblings, 0 replies; 10+ messages in thread From: Jinjie Ruan @ 2024-08-09 10:15 UTC (permalink / raw) To: Catalin Marinas Cc: Baoquan He, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On 2024/8/9 17:51, Catalin Marinas wrote: > On Thu, Aug 08, 2024 at 03:56:35PM +0800, Jinjie Ruan wrote: >> On 2024/8/7 3:34, Catalin Marinas wrote: >>> On Tue, Aug 06, 2024 at 08:10:30PM +0100, Catalin Marinas wrote: >>>> On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: >>>>> And I don't like the idea crashkernel=,high failure will fallback to >>>>> attempt in low area, so this looks good to me. >>>> >>>> Well, I kind of liked this behaviour. One can specify ,high as a >>>> preference rather than forcing a range. The arm64 land has different >>>> platforms with some constrained memory layouts. Such fallback works well >>>> as a default command line option shipped with distros without having to >>>> guess the SoC memory layout. >>> >>> I haven't tried but it's possible that this patch also breaks those >>> arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is >>> memblock_end_of_DRAM(). Here all memory would be low and in the absence >>> of no fallback, it fails to allocate. >>> >>> So, my strong preference would be to re-instate the current behaviour >>> and work around the infinite loop in a different way. >> >> Hi, baoquan, What's your opinion? >> >> Only this patch should be re-instate or all the 3 dead loop fix patch? > > Only the riscv64 patch that that removes the ,high reservation fallback > to ,low. From this series: > > https://lore.kernel.org/r/20240719095735.1912878-1-ruanjinjie@huawei.com/ > > the first two fixes look fine (x86_32). The third one (arm32), not sure > why it's in the series called "crash: Fix x86_32 memory reserve dead > loop bug". Does it fix a problem on arm32? Anyway, I'm not against it > getting merged but I'm not maintaining arm32. If the first two patches > could be merged for 6.11, I think the arm32 one is more of a 6.12 > material (unless it does fix something). > > On the riscv64 patch removing the high->low fallback to avoid the > infinite loop, I'd rather replace it with something similar to the > x86_32 fix in the series above. I suggested something in the main if > block but, looking at the x86_32 fix, for consistency, I think it would > look better as something like: > > diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c > index d3b4cd12bdd1..64d44a52c011 100644 > --- a/kernel/crash_reserve.c > +++ b/kernel/crash_reserve.c > @@ -423,7 +423,8 @@ void __init reserve_crashkernel_generic(char *cmdline, > if (high && search_end == CRASH_ADDR_HIGH_MAX) { > search_end = CRASH_ADDR_LOW_MAX; > search_base = 0; > - goto retry; > + if (search_end != CRASH_ADDR_HIGH_MAX) > + goto retry; > } > pr_warn("cannot allocate crashkernel (size:0x%llx)\n", > crash_size); > > In summary, just replace the riscv64 fix with something along the lines > of the diff above (or pick whatever you prefer that still keeps the > fallback). Hi, Andrew Could you please help to remove the riscv64 fix from your mm tree as Catalin suggested? we will give the new patch sooner. > > Thanks. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-06 19:34 ` Catalin Marinas 2024-08-08 7:56 ` Jinjie Ruan @ 2024-08-13 8:40 ` Petr Tesařík 2024-08-13 12:04 ` Catalin Marinas 1 sibling, 1 reply; 10+ messages in thread From: Petr Tesařík @ 2024-08-13 8:40 UTC (permalink / raw) To: Catalin Marinas Cc: Baoquan He, Jinjie Ruan, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon Hi Catalin, On Tue, 6 Aug 2024 20:34:42 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Aug 06, 2024 at 08:10:30PM +0100, Catalin Marinas wrote: > > On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: > > > On 08/02/24 at 05:01pm, Jinjie Ruan wrote: > > > > On RISCV64 Qemu machine with 512MB memory, cmdline "crashkernel=500M,high" > > > > will cause system stall as below: > > > > > > > > Zone ranges: > > > > DMA32 [mem 0x0000000080000000-0x000000009fffffff] > > > > Normal empty > > > > Movable zone start for each node > > > > Early memory node ranges > > > > node 0: [mem 0x0000000080000000-0x000000008005ffff] > > > > node 0: [mem 0x0000000080060000-0x000000009fffffff] > > > > Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff] > > > > (stall here) > > > > > > > > commit 5d99cadf1568 ("crash: fix x86_32 crash memory reserve dead loop > > > > bug") fix this on 32-bit architecture. However, the problem is not > > > > completely solved. If `CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX` on 64-bit > > > > architecture, for example, when system memory is equal to > > > > CRASH_ADDR_LOW_MAX on RISCV64, the following infinite loop will also occur: > > > > > > Interesting, I didn't expect risc-v defining them like these. > > > > > > #define CRASH_ADDR_LOW_MAX dma32_phys_limit > > > #define CRASH_ADDR_HIGH_MAX memblock_end_of_DRAM() > > > > arm64 defines the high limit as PHYS_MASK+1, it doesn't need to be > > dynamic and x86 does something similar (SZ_64T). Not sure why the > > generic code and riscv define it like this. > > > > > > -> reserve_crashkernel_generic() and high is true > > > > -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail > > > > -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly > > > > (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX). > > > > > > > > Before refactor in commit 9c08a2a139fe ("x86: kdump: use generic interface > > > > to simplify crashkernel reservation code"), x86 do not try to reserve crash > > > > memory at low if it fails to alloc above high 4G. However before refator in > > > > commit fdc268232dbba ("arm64: kdump: use generic interface to simplify > > > > crashkernel reservation"), arm64 try to reserve crash memory at low if it > > > > fails above high 4G. For 64-bit systems, this attempt is less beneficial > > > > than the opposite, remove it to fix this bug and align with native x86 > > > > implementation. > > > > > > And I don't like the idea crashkernel=,high failure will fallback to > > > attempt in low area, so this looks good to me. > > > > Well, I kind of liked this behaviour. One can specify ,high as a > > preference rather than forcing a range. The arm64 land has different > > platforms with some constrained memory layouts. Such fallback works well > > as a default command line option shipped with distros without having to > > guess the SoC memory layout. > > I haven't tried but it's possible that this patch also breaks those > arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is > memblock_end_of_DRAM(). Here all memory would be low and in the absence > of no fallback, it fails to allocate. I'm afraid you've just opened a Pandora box... ;-) Another (unrelated) patch series made us aware of a platforms where RAM starts at 32G, but IIUC the host bridge maps 32G-33G to bus addresses 0-1G, and there is a device on that bus which can produce only 30-bit addresses. Now, what was the idea behind allocating some crash memory "low"? Right, it should allow the crash kernel to access devices with addressing constraints. So, on the above-mentioned platform, allocating "low" would in fact mean allocating between 32G and 33G (in host address domain). Should we rethink the whole concept of high/low? Petr T ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-13 8:40 ` Petr Tesařík @ 2024-08-13 12:04 ` Catalin Marinas 2024-08-13 13:33 ` Petr Tesařík 0 siblings, 1 reply; 10+ messages in thread From: Catalin Marinas @ 2024-08-13 12:04 UTC (permalink / raw) To: Petr Tesařík Cc: Baoquan He, Jinjie Ruan, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon Hi Petr, On Tue, Aug 13, 2024 at 10:40:06AM +0200, Petr Tesařík wrote: > On Tue, 6 Aug 2024 20:34:42 +0100 > Catalin Marinas <catalin.marinas@arm.com> wrote: > > I haven't tried but it's possible that this patch also breaks those > > arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is > > memblock_end_of_DRAM(). Here all memory would be low and in the absence > > of no fallback, it fails to allocate. > > I'm afraid you've just opened a Pandora box... ;-) Not that bad ;) but, yeah, this patch was dropped in favour of this: https://lore.kernel.org/r/20240812062017.2674441-1-ruanjinjie@huawei.com/ > Another (unrelated) patch series made us aware of a platforms where RAM > starts at 32G, but IIUC the host bridge maps 32G-33G to bus addresses > 0-1G, and there is a device on that bus which can produce only 30-bit > addresses. > > Now, what was the idea behind allocating some crash memory "low"? > Right, it should allow the crash kernel to access devices with > addressing constraints. So, on the above-mentioned platform, allocating > "low" would in fact mean allocating between 32G and 33G (in host address > domain). Indeed. If that's not available, the crash kernel won't be able to boot (unless the corresponding device is removed from DT or ACPI tables). > Should we rethink the whole concept of high/low? Yeah, it would be good to revisit those at some point. For the time being, 'low' in this context on arm64 means ZONE_DMA memory, basically the common denominator address range that supports all devices on an SoC. For others like x86_32, this means the memory that the kernel can actually map (not necessarily device/DMA related). So, it's not always about the DMA capabilities but also what the crash kernel can map (so somewhat different from the zone allocator case we've been discussing in other threads). -- Catalin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-13 12:04 ` Catalin Marinas @ 2024-08-13 13:33 ` Petr Tesařík 0 siblings, 0 replies; 10+ messages in thread From: Petr Tesařík @ 2024-08-13 13:33 UTC (permalink / raw) To: Catalin Marinas Cc: Baoquan He, Jinjie Ruan, vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On Tue, 13 Aug 2024 13:04:31 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote: > Hi Petr, > > On Tue, Aug 13, 2024 at 10:40:06AM +0200, Petr Tesařík wrote: > > On Tue, 6 Aug 2024 20:34:42 +0100 > > Catalin Marinas <catalin.marinas@arm.com> wrote: > > > I haven't tried but it's possible that this patch also breaks those > > > arm64 platforms with all RAM above 4GB when CRASH_ADDR_LOW_MAX is > > > memblock_end_of_DRAM(). Here all memory would be low and in the absence > > > of no fallback, it fails to allocate. > > > > I'm afraid you've just opened a Pandora box... ;-) > > Not that bad ;) but, yeah, this patch was dropped in favour of this: > > https://lore.kernel.org/r/20240812062017.2674441-1-ruanjinjie@huawei.com/ Yes, I have noticed. That one simply preserves the status quo and a fuzzy definition of "low". > > Another (unrelated) patch series made us aware of a platforms where RAM > > starts at 32G, but IIUC the host bridge maps 32G-33G to bus addresses > > 0-1G, and there is a device on that bus which can produce only 30-bit > > addresses. > > > > Now, what was the idea behind allocating some crash memory "low"? > > Right, it should allow the crash kernel to access devices with > > addressing constraints. So, on the above-mentioned platform, allocating > > "low" would in fact mean allocating between 32G and 33G (in host address > > domain). > > Indeed. If that's not available, the crash kernel won't be able to boot > (unless the corresponding device is removed from DT or ACPI tables). Then it may be able to boot, but it won't be able to save a crash dump on disk or send it over the network, rendering the panic kernel environment a bit less useful. > > Should we rethink the whole concept of high/low? > > Yeah, it would be good to revisit those at some point. For the time > being, 'low' in this context on arm64 means ZONE_DMA memory, basically > the common denominator address range that supports all devices on an > SoC. For others like x86_32, this means the memory that the kernel can > actually map (not necessarily device/DMA related). Ah, right. I forgot that there are also constraints on the placement of the kernel identity mapping in CPU physical address space. > So, it's not always about the DMA capabilities but also what the crash > kernel can map (so somewhat different from the zone allocator case we've > been discussing in other threads). It seems to me that a good panic kernel environment requires: 1. memory where kernel text/data can be mapped (even at early init) 2. memory that is accessible to I/O devices 3. memory that can be allocated to user space (e.g. makedumpfile) The first two blocks may require special placement in bus/CPU physical address space, the third does not, but it needs to be big enough for the workload. I'll try to transform this knowledge into something actionable or even reviewable. For now, I agree there's nothing more to discuss. Thanks Petr T ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop 2024-08-06 19:10 ` [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop Catalin Marinas 2024-08-06 19:34 ` Catalin Marinas @ 2024-08-07 1:40 ` Jinjie Ruan 1 sibling, 0 replies; 10+ messages in thread From: Jinjie Ruan @ 2024-08-07 1:40 UTC (permalink / raw) To: Catalin Marinas, Baoquan He Cc: vgoyal, dyoung, paul.walmsley, palmer, aou, chenjiahao16, akpm, kexec, linux-kernel, linux-riscv, linux-arm-kernel, Will Deacon On 2024/8/7 3:10, Catalin Marinas wrote: > To Jinjie, if you make generic changes that affect other architectures, > please either cc the individual lists/maintainers or at least cross-post > to linux-arch. I don't follow lkml, there's just too much traffic there. Sorry, I forgot to Cc to the other architectures. > > On Fri, Aug 02, 2024 at 06:11:01PM +0800, Baoquan He wrote: >> On 08/02/24 at 05:01pm, Jinjie Ruan wrote: >>> On RISCV64 Qemu machine with 512MB memory, cmdline "crashkernel=500M,high" >>> will cause system stall as below: >>> >>> Zone ranges: >>> DMA32 [mem 0x0000000080000000-0x000000009fffffff] >>> Normal empty >>> Movable zone start for each node >>> Early memory node ranges >>> node 0: [mem 0x0000000080000000-0x000000008005ffff] >>> node 0: [mem 0x0000000080060000-0x000000009fffffff] >>> Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff] >>> (stall here) >>> >>> commit 5d99cadf1568 ("crash: fix x86_32 crash memory reserve dead loop >>> bug") fix this on 32-bit architecture. However, the problem is not >>> completely solved. If `CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX` on 64-bit >>> architecture, for example, when system memory is equal to >>> CRASH_ADDR_LOW_MAX on RISCV64, the following infinite loop will also occur: >> >> Interesting, I didn't expect risc-v defining them like these. >> >> #define CRASH_ADDR_LOW_MAX dma32_phys_limit >> #define CRASH_ADDR_HIGH_MAX memblock_end_of_DRAM() > > arm64 defines the high limit as PHYS_MASK+1, it doesn't need to be > dynamic and x86 does something similar (SZ_64T). Not sure why the > generic code and riscv define it like this. > >>> -> reserve_crashkernel_generic() and high is true >>> -> alloc at [CRASH_ADDR_LOW_MAX, CRASH_ADDR_HIGH_MAX] fail >>> -> alloc at [0, CRASH_ADDR_LOW_MAX] fail and repeatedly >>> (because CRASH_ADDR_LOW_MAX = CRASH_ADDR_HIGH_MAX). >>> >>> Before refactor in commit 9c08a2a139fe ("x86: kdump: use generic interface >>> to simplify crashkernel reservation code"), x86 do not try to reserve crash >>> memory at low if it fails to alloc above high 4G. However before refator in >>> commit fdc268232dbba ("arm64: kdump: use generic interface to simplify >>> crashkernel reservation"), arm64 try to reserve crash memory at low if it >>> fails above high 4G. For 64-bit systems, this attempt is less beneficial >>> than the opposite, remove it to fix this bug and align with native x86 >>> implementation. >> >> And I don't like the idea crashkernel=,high failure will fallback to >> attempt in low area, so this looks good to me. > > Well, I kind of liked this behaviour. One can specify ,high as a > preference rather than forcing a range. The arm64 land has different > platforms with some constrained memory layouts. Such fallback works well > as a default command line option shipped with distros without having to > guess the SoC memory layout. > > Something like below should fix the issue as well (untested): I tested it on QEMU and it is good to fix this dead loop problem. > > diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c > index d3b4cd12bdd1..ae92d6745ef4 100644 > --- a/kernel/crash_reserve.c > +++ b/kernel/crash_reserve.c > @@ -420,7 +420,8 @@ void __init reserve_crashkernel_generic(char *cmdline, > * For crashkernel=size[KMG],high, if the first attempt was > * for high memory, fall back to low memory. > */ > - if (high && search_end == CRASH_ADDR_HIGH_MAX) { > + if (high && search_end == CRASH_ADDR_HIGH_MAX && > + CRASH_ADDR_LOW_MAX < CRASH_ADDR_HIGH_MAX) { > search_end = CRASH_ADDR_LOW_MAX; > search_base = 0; > goto retry; > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-08-13 13:36 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240802090105.3871929-1-ruanjinjie@huawei.com>
[not found] ` <ZqywtegyIS/YXOVv@MiWiFi-R3L-srv>
2024-08-06 19:10 ` [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop Catalin Marinas
2024-08-06 19:34 ` Catalin Marinas
2024-08-08 7:56 ` Jinjie Ruan
2024-08-09 1:56 ` Baoquan He
2024-08-09 9:51 ` Catalin Marinas
2024-08-09 10:15 ` Jinjie Ruan
2024-08-13 8:40 ` Petr Tesařík
2024-08-13 12:04 ` Catalin Marinas
2024-08-13 13:33 ` Petr Tesařík
2024-08-07 1:40 ` Jinjie Ruan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).