Re: [PATCH v23 3/6] arm64: kdump: Reimplement crashkernel=X

["Leizhen (ThunderTown)" <thunder.leizhen@huawei.com>]

On 2022/5/6 19:06, Catalin Marinas wrote:
> On Fri, May 06, 2022 at 11:22:51AM +0800, Leizhen (ThunderTown) wrote:
>> On 2022/5/6 1:01, Catalin Marinas wrote:
>>> On Thu, May 05, 2022 at 05:18:42PM +0800, Zhen Lei wrote:
>>>> From: Chen Zhou <chenzhou10@huawei.com>
>>>>
>>>> There are following issues in arm64 kdump:
>>>> 1. We use crashkernel=X to reserve crashkernel in DMA zone, which
>>>> will fail when there is not enough low memory.
>>>> 2. If reserving crashkernel above DMA zone, in this case, crash dump
>>>> kernel will fail to boot because there is no low memory available
>>>> for allocation.
>>>>
>>>> To solve these issues, introduce crashkernel=X,[high,low].
>>>> The "crashkernel=X,high" is used to select a region above DMA zone, and
>>>> the "crashkernel=Y,low" is used to allocate specified size low memory.
>>>
>>> Thanks for posting the simplified version, though the discussion with
>>> Baoquan is still ongoing. AFAICT there is no fallback if crashkernel=
>>> fails. The advantage with this series is cleaner code, we set the limits
>>> during parsing and don't have to adjust them if some of the first
>>> allocation failed.
>>
>> Yes, I'm currently implementing it in the simplest version, providing only
>> the most basic functions. Because the conclusions of this part of the discussion
>> are clear. I think I can send the fallback, default low size, and mapping optimization
>> patches separately after this basic version is merged. These three functions can
>> be discussed separately.
> 
> This works for me. If we decide to go for fallbacks, it can be done as a
> separate patch.
> 
>>>> +		ret = parse_crashkernel_high(cmdline, 0, &crash_size, &crash_base);
>>>> +		if (ret || !crash_size)
>>>> +			return;
>>>> +
>>>> +		/*
>>>> +		 * crashkernel=Y,low can be specified or not, but invalid value
>>>> +		 * is not allowed.
>>>> +		 */
>>>> +		ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base);
>>>> +		if (ret && (ret != -ENOENT))
>>>> +			return;
>>>> +
>>>> +		crash_max = CRASH_ADDR_HIGH_MAX;
>>>> +	}
>>>>  
>>>>  	crash_size = PAGE_ALIGN(crash_size);
>>>>  
>>>> @@ -118,8 +159,7 @@ static void __init reserve_crashkernel(void)
>>>>  	if (crash_base)
>>>>  		crash_max = crash_base + crash_size;
>>>>  
>>>> -	/* Current arm64 boot protocol requires 2MB alignment */
>>>> -	crash_base = memblock_phys_alloc_range(crash_size, SZ_2M,
>>>> +	crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>>>>  					       crash_base, crash_max);
>>>>  	if (!crash_base) {
>>>>  		pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>>>
>>> I personally like this but let's see how the other thread goes. I guess
>>
>> Me too. This fallback complicates code logic more than just a little.
>> I'm not sure why someone would rather add fallback than change the bootup
>> options to crashkernel=X,[high|low]. Perhaps fallback to high/low is a better
>> compatible and extended mode when crashkernel=X fails to reserve memory. And
>> the code logic will be much clearer.
>>
>> //parse crashkernel=X		//To simplify the discussion, Ignore [@offset]
>> crash_base = memblock_phys_alloc_range()
>> if (!crash_base || /* crashkernel=X is not specified */) {
>> 	//parse crashkernel=X,[high,low]
>> 	//reserve high/low memory
>> }
>>
>> So that, the following three modes are supported:
>> 1) crashkernel=X[@offset]
>> 2) crashkernel=X,high crashkernel=X,low
>> 3) crashkernel=X[@offset] crashkernel=X,high [crashkernel=Y,low]
> 
> The whole interface isn't great but if we add fall-back options, I'd
> rather stick close to what x86 does. IOW, if crashkernel=X is provided,
> ignore explicit high/low (so 3 does not exist).
> 
> (if I had added it from the beginning, I'd have removed 'high'
> completely and allow crashkernel=X to fall-back to 'high' with an
> optional explicit 'low' or 'dma' if the default is not sufficient; but I

Er, my idea almost coincides with yours. When 3) removes 'high', it's the same
way you think. Of course, I haven't thought of deleting 'high' yet. So your
idea is more perfect.

> think there's too much bikeshedding already)

Yeah, the oldest prince has royal power. There's no choice now.

> 
>>> if we want a fallback, it would come just before the check the above:
>>>
>>> 	if (!crash_base && crash_max != CRASH_ADDR_HIGH_MAX) {
>>> 		/* attempt high allocation with default low */
>>> 		if (!crash_low_size)
>>> 			crash_low_size = some default;
>>> 		crash_max = CRASH_ADDR_LOW_MAX;
>>
>> crash_max = CRASH_ADDR_HIGH_MAX; We should fallback to high memory now.
> 
> Yes, that's the idea.
> 
> Anyway, please post the current series with the minor updates I
> mentioned and we can add a fallback patch (or two) on top.
> 
> Thanks.
> 

-- 
Regards,
  Zhen Lei