Hi Philipp,
I just switched back from another task.
Thanks for your careful review.
On Tue, May 9, 2023 at 12:17 AM Philipp Rudo <prudo(a)redhat.com> wrote:
On Wed, 19 Apr 2023 20:13:55 +0800
Pingfan Liu <piliu(a)redhat.com> wrote:
> From: Pingfan Liu <piliu at redhat.com>
>
> On aarch64, both 4K and 64K kernel can be installed, while they demand
> different size reserved memory for kdump kernel.
>
> 'get_conf PAGE_SIZE' can not work if installing a 64K kernel when
> running a 4K kernel. Hence resorting to the kernel release naming
> convention. At present, the 64K kernel has the keyword '64k' in its
> suffix.
Can you give an example how the version for a the 64k kernel looks
like. I think that is good to document for the future.
> The base line for 64K is decided based on 4K. The difference 100M is
> chosen because on a high end machine without smmu enabled, the
> difference of MemFree is 82M.
>
> As for the smmu case, a significant memory consumption difference lies
> between 64k and 4k driver. And it should be calculated separatedly.
> (Implemented in later patch)
>
> Signed-off-by: Pingfan Liu <piliu(a)redhat.com>
> To: kexec(a)lists.fedoraproject.org
> Cc: Coiby Xu <coxu(a)redhat.com>
>
> ---
> kdump-lib.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
> kdumpctl | 22 ++++++++++++++++++++++
> 2 files changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index bc725c0..0869703 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -879,8 +879,41 @@ get_recommend_size()
> echo "0M"
> }
>
> +
> +kdump_get_aarch64_64k_crashkernel()
> +{
> + # base line 4K
> + local _ck_cmdline="1G-4G:256M,4G-64G:320M,64G-:576M"
> + local _new_str=""
> + # Without smmu, the diff of MemFree between 4K and 64K measured on a high end
aarch64 machine is 82M.
> + # Picking up 100M to cover this diff. And finally, we have
"1G-4G:356M;4G-64G:420M;64G-:676M"
> + local _64k_delta="100"
> +
> + IFS=',' read -ra ADDR <<< "$_ck_cmdline"
> + for i in "${ADDR[@]}"; do
> + key=$(echo "$i" | cut -d':' -f 1)
> + value=$(echo "$i" | cut -d':' -f 2)
> + value=$(echo "$value" | tr -d 'M')
> + value=$((value + _64k_delta))
> + _new_str+="${key}:${value}M,"
> + done
> + _new_str=${_new_str::-1}
> + echo "$_new_str"
> +}
> +
> +kdump_get_aarch64_4k_crashkernel()
> +{
> + # For 4KB page size, the formula is based on x86 plus extra = 64M
> + local _ck_cmdline="1G-4G:256M,4G-64G:320M,64G-:576M"
> +
> + echo $"_ck_cmdline"
> +}
> +
There are several things I don't like about these two functions
1) the default _ck_cmdline get's duplicated. So if that changes we will
need to update two locations.
Yes, my purpose is to use the 4K configuration as a base line. It is
better to remove the duplication.
2) The loop that updates _ck_cmdline will be introduced a second
time
in the last patch. Why don't you move it to a generic
'_crashkernel_add'.
Good idea.
3) I would prefer the loop that updates _ck_cmdline to be more
generic.
It does work with the given cmdline but there are many cases it does
not cover, e.g. when the size isn't given in Megabytes or when a offset
is provided (@...). While reviewing I've played around a little bit and
came up with the function below. Ideally you could add a test case for
it, too ;-)
Yes, maybe I can introduce another patch based on it
4) In general I don't understand why you need to have those two
functions at all. Personally I prefer something a long the line of
kdump_get_arch_recommend_crashkernel()
{
[...]
elif [[ $_arch == aarch64 ]]; then
_ck_cmdline="1G-4G:256M,4G-64G:320M,64G-:576M"
_delta=0
if [[ $_variant == 64k ]]; then
_delta+=100
has_aarch64_smmu && _delta+=384
has_mlx5 && _delta+=200
else
has_mlx5 && _delta+=150
fi
_crshkernel_add "$_ck_cmdline" "$_delta"
elif [[ $_arch == aarch64 ]]; then
[...]
}
It looks great and the total layout looks more neat.
> # get default crashkernel
> # $1 dump mode, if not specified, dump_mode will be judged by is_fadump_capable
> +# $2 target-kernel, if not specified, got by 'uname -r'
> +#
> +# shellcheck disable=SC2120
> kdump_get_arch_recommend_crashkernel()
> {
> local _arch _ck_cmdline _dump_mode
> @@ -900,8 +933,20 @@ kdump_get_arch_recommend_crashkernel()
> if [[ $_arch == "x86_64" ]] || [[ $_arch == "s390x" ]];
then
> _ck_cmdline="1G-4G:192M,4G-64G:256M,64G-:512M"
> elif [[ $_arch == "aarch64" ]]; then
> - # For 4KB page size, the formula is based on x86 plus extra = 64M
> - _ck_cmdline="1G-4G:256M,4G-64G:320M,64G-:576M"
> + local _target_kernel
> +
> + if [[ -z "$2" ]]; then
> + _target_kernel=$(uname -r)
s/uname -r/_get_kdump_kernel_version/
otherwise you might not use the same version like the rest of the
script.
Adopt.
Again, thanks for your help.
Rgards,
Pingfan
> + else
> + _target_kernel=$2
> + fi
> +
> + if echo "$_target_kernel" | grep -q 64k; then
> + # For 64KB page size
> + _ck_cmdline=$(kdump_get_aarch64_64k_crashkernel)
> + else
> + _ck_cmdline=$(kdump_get_aarch64_4k_crashkernel)
> + fi
> elif [[ $_arch == "ppc64le" ]]; then
> if [[ $_dump_mode == "fadump" ]]; then
>
_ck_cmdline="4G-16G:768M,16G-64G:1G,64G-128G:2G,128G-1T:4G,1T-2T:6G,2T-4T:12G,4T-8T:20G,8T-16T:36G,16T-32T:64G,32T-64T:128G,64T-:180G"
Thanks
Philipp
----
_crashkernel_add()
{
local _ck _add _entry _ret
local _range _size _offset
_ck="$1"
_add="$2"
_ret=""
if [[ "$_ck" == *@* ]]; then
_offset="@${_ck##*@}"
_ck=${_ck%@*}
elif [[ "$_ck" == *,high ]] || [[ "$_ck" == *,low ]]; then
_offset=",${_ck##*,}"
_ck=${_ck%,*}
else
_offset=''
fi
while read -d , -r _entry; do
[[ -n "$_entry" ]] || continue
if [[ "$_entry" == *:* ]]; then
_range=${_entry%:*}
_size=${_entry#*:}
else
_range=""
_size=${_entry}
fi
case "${_size: -1}" in
K)
_size=${_size::-1}
_size="$((_size + (_add * 1024)))K"
;;
M)
_size=${_size::-1}
_size="$((_size + _add))M"
;;
G)
_size=${_size::-1}
_size="$((_size + (_add / 1024)))G"
;;
*)
_size="$((_size + (_add * 1024 * 1024)))"
;;
esac
[[ -n "$_range" ]] && ret+="$_range:"
_ret+="$_size,"
done <<< "$_ck,"
_ret=${_ret%,}
[[ -n "$_offset" ]] && _ret+=$_offset
echo $_ret
}