Re: [PATCH v3] arm64: kdump: simplify the reservation behaviour of crashkernel=,high

[Leizhen (ThunderTown)]

On 2023/3/3 11:01, Baoquan He wrote:
> On 03/02/23 at 11:32am, Leizhen (ThunderTown) wrote:
> ......
>>> @@ -166,31 +169,51 @@ static void __init reserve_crashkernel(void)
>>>  	/* User specifies base address explicitly. */
>>>  	if (crash_base) {
>>>  		fixed_base = true;
>>> +		search_base = crash_base;
>>>  		crash_max = crash_base + crash_size;
>>>  	}
>>>  
>>>  retry:
>>>  	crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>>> -					       crash_base, crash_max);
>>> +					       search_base, crash_max);
>>>  	if (!crash_base) {
>>>  		/*
>>> -		 * If the first attempt was for low memory, fall back to
>>> -		 * high memory, the minimum required low memory will be
>>> -		 * reserved later.
>>> +		 * For crashkernel=size[KMG]@offset[KMG], print out failure
>>> +		 * message if can't reserve the specified region.
>>>  		 */
>>> -		if (!fixed_base && (crash_max == CRASH_ADDR_LOW_MAX)) {
>>> +		if (fixed_base) {
>>> +			pr_info("crashkernel reservation failed - memory is in use.\n");
>>
>> How about changing pr_info to pr_warn?
>>
>>> +			return;
>>> +		}
>>> +
>>> +		/*
>>> +		 * For crashkernel=size[KMG], if the first attempt was for
>>> +		 * low memory, fall back to high memory, the minimum required
>>> +		 * low memory will be reserved later.
>>> +		 */
>>> +		if (!high && crash_max == CRASH_ADDR_LOW_MAX) {
>>>  			crash_max = CRASH_ADDR_HIGH_MAX;
>>> +			search_base = CRASH_ADDR_LOW_MAX;
>>>  			crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
>>>  			goto retry;
>>>  		}
>>>  
>>> +		/*
>>> +		 * For crashkernel=size[KMG],high, if the first attempt was
>>> +		 * for high memory, fall back to low memory.
>>> +		 */
>>> +		if (high && crash_max == CRASH_ADDR_HIGH_MAX) {
>>
>> Adding unlikely to indicate that it is rare would be better.
>>
>> if (unlikely(high && crash_max == CRASH_ADDR_HIGH_MAX))
> 
> Rethink about this and checked code in kernel, seems likely|unlikely are
> mostly used in highly frequent execution branch optimize code path, while 
> crashkernel resevatoin is one time execution during boot, we may not
> need to bother to add unlikely. What do you think?

OK.

> 
> 
>>
>>> +			crash_max = CRASH_ADDR_LOW_MAX;
>>> +			search_base = 0;
>>> +			goto retry;
>>> +		}
>>>  		pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>>>  			crash_size);
>>>  		return;
>>>  	}
>>>  
>>> -	if ((crash_base > CRASH_ADDR_LOW_MAX - crash_low_size) &&
>>> -	     crash_low_size && reserve_crashkernel_low(crash_low_size)) {
>>> +	if ((crash_base >= CRASH_ADDR_LOW_MAX) && crash_low_size &&
>>> +	     reserve_crashkernel_low(crash_low_size)) {
>>>  		memblock_phys_free(crash_base, crash_size);
>>>  		return;
>>>  	}
>>>
>>
>> -- 
>> Regards,
>>   Zhen Lei
>>
> 
> .
> 

-- 
Regards,
  Zhen Lei