Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code

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

On 2021/12/29 15:27, Dave Young wrote:
> On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/12/29 0:13, Borislav Petkov wrote:
>>> On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
>>>> Use parse_crashkernel_high_low() to bring the parsing of
>>>> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
>>>> are strongly dependent, make code logic clear and more readable.
>>>>
>>>> Suggested-by: Borislav Petkov <bp@alien8.de>
>>>
>>> Yeah, doesn't look like something I suggested...
>>>
>>>> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
>>>>  	/* crashkernel=XM */
>>>>  	ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
>>>>  	if (ret != 0 || crash_size <= 0) {
>>>> -		/* crashkernel=X,high */
>>>> -		ret = parse_crashkernel_high(boot_command_line, total_mem,
>>>> -					     &crash_size, &crash_base);
>>>> -		if (ret != 0 || crash_size <= 0)
>>>> +		/* crashkernel=X,high and possible crashkernel=Y,low */
>>>> +		ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
>>>
>>> So this calls parse_crashkernel() and when that one fails, it calls this
>>> new weird parse high/low helper you added.
>>>
>>> But then all three end up in the same __parse_crashkernel() worker
>>> function which seems to do the actual parsing.
>>>
>>> What I suggested and what would be real clean is if the arches would
>>> simply call a *single* 
>>>
>>> 	parse_crashkernel()
>>>
>>> function and when that one returns, *all* crashkernel= options would
>>> have been parsed properly, low, high, middle crashkernel, whatever...
>>> and the caller would know what crash kernel needs to be allocated.
>>>
>>> Then each arch can do its memory allocations and checks based on that
>>> parsed data and decide to allocate or bail.
>>
>> However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
>> will also support it. It is not supported on other architectures. So changing parse_crashkernel()
>> is not appropriate unless a new function is introduced. But naming this new function isn't easy,
>> and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
>> Of course, we can also consider changing parse_crashkernel() to another name, then use
>> parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
>> other architectures to change as well.
> 
> Hi, I did not follow up all discussions, but if the only difference is
> about the low -> high fallback, I think you can add another argument in
> parse_crashkernel(..., *fallback_high),  and doing some changes in
> __parse_crashkernel() before it returns.  But since there are two
> many arguments, you could need a wrapper struct for crashkernel_param if
> needed.

A wrapper struct is needed. Because at least two new output parameters need to be added:
1. high, to indicate whether "crashkernel=X,high" is specified
2. low_size, to save the memory size specified by "crashkernel=Y,low"

> 
> If you do not want to touch other arches, another function maybe
> something like parse_crashkernel_fallback() for x86 and arm64 to use.
> 
> But I may not get all the context, please ignore if this is not the
> case.  I agree that calling parse_crash_kernel* in the
> reserve_crashkernel funtions looks not good though. 
> 
> OTOH there are bunch of other logics in param parsing code,
> eg. determin the final size and offset etc. To split the logic out more
> things need to be done, eg. firstly parsing function just get all the
> inputs from cmdline string eg. an array of struct crashkernel_param with
> mem_range, expected size, expected offset, high, or low, and do not do
> any other things.   Then pass these parsed inputs to another function to
> determine the final crash_size and crash_base, that part can be arch
> specific somehow. 

Yes, this way makes the code logic clear. But there's a bit of trouble with
"crashkernel=range1:size1[,range2:size2,...][@offset]", the array size is dynamic.

> 
> So I think you can unify the parse_crashkernel* in x86 first with just
> one function.  And leave the further improvements to later work. But
> let's see how Boris think about this.
> 
>>
>>>
>>> So it is getting there but it needs more surgery...
>>>
>>> Thx.
>>>
>>
>> -- 
>> Regards,
>>   Zhen Lei
>>
> 
> Thanks
> Dave
> 
> .
> 

-- 
Regards,
  Zhen Lei