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@xxxxxxxxxx>
>>>>
>>>> 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
