Re: [PATCH -next] crash: Fix riscv64 crash memory reserve dead loop

[Jinjie Ruan <ruanjinjie@huawei.com>]

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.
>