On Mon, May 22, 2023 at 10:59 PM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Pingfan,
On Thu, 18 May 2023 13:26:42 +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 rules.
> At present, the 64K kernel has the keyword '64k' in its suffix.
>
> The base line for 64K is decided based on 4K. The diff 120M is picked up
> since on a high end machine without smmu enabled, the diff of MemFree is
> 82M.
>
> As for the smmu case, a huge difference in the memory consumption lies
> between 64k and 4k driver. And it should be calculated separatedly.
>
> Signed-off-by: Pingfan Liu <piliu(a)redhat.com>
> ---
> kdump-lib.sh | 21 ++++++++++++++++++++-
> kdumpctl | 24 ++++++++++++++++++++++++
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index f56a011..e1d8da9 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -966,6 +966,7 @@ _crashkernel_add()
>
> # get default crashkernel
> # $1 dump mode, if not specified, dump_mode will be judged by is_fadump_capable
> +# $2 kernel-release, if not specified, got by _get_kdump_kernel_version
> kdump_get_arch_recommend_crashkernel()
> {
> local _arch _ck_cmdline _dump_mode
> @@ -985,8 +986,26 @@ 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
> + local _running_kernel
> + local _delta="0"
> +
> + # Base line for 4K variant kernel. The formula is based on x86 plus
extra = 64M
> _ck_cmdline="1G-4G:256M,4G-64G:320M,64G-:576M"
> + if [[ -z "$2" ]]; then
> + _running_kernel=$(_get_kdump_kernel_version)
> + else
> + _running_kernel=$2
> + fi
> +
> + # the naming convention of 64k variant suffixes with +64k, e.g.
"vmlinuz-5.14.0-312.el9.aarch64+64k"
> + if echo "$_running_kernel" | grep 64k; then
^^^^
grep -q ? Otherwise the match will be
printed to stdout.
Adopt, thanks.
> + # 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"
> + _delta+=100
^^^
In the patch description you wrote that
the value gets increased by 120M. Which
one is correct?
Nice catch. I will fix the description.
Furthermore, I was sloppy when providing the example. While the line
above is valid bash code it won't work as intended. In particular when
you are using += in bash it evaluates both sides as strings, _not_ as
mathematical expressions. For example
$ a=1
$ a+=1
$ echo $a
will give '11', _not_ '2'. The correct syntax for what you want is
Oops, the C grammar is so deeply rooted in my mind that I am not aware
of it either, although I read such a topic just a few days ago.
$ (( _delta += 100 ))
(Note: the spaces after/before (( / )) are required. The other ones are
optional.)
Sorry but I can not find any format restrictions around ((...)), and I
have done some test.
> + else
> + _delta+=0
> + fi
> + _ck_cmdline=$(_crashkernel_add "$_ck_cmdline"
"$_delta")
> 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"
> diff --git a/kdumpctl b/kdumpctl
> index 2bec428..cbdc45a 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1623,6 +1623,7 @@ reset_crashkernel_for_installed_kernel()
> {
> local _installed_kernel _running_kernel _crashkernel _crashkernel_running
> local _dump_mode_running _fadump_val_running
> + local _arch
>
> # During package install, only try to reset crashkernel for osbuild
> # thus to avoid calling grubby when installing os via anaconda
> @@ -1651,6 +1652,29 @@ reset_crashkernel_for_installed_kernel()
> _dump_mode_running=$(get_dump_mode_by_kernel "$_running_kernel")
> _fadump_val_running=$(get_grub_kernel_boot_parameter "$_kernel"
fadump)
>
> + _arch=$(uname -m)
> + # On aarch64, there are 4K and 64K kernel, which demands different crashkernel
value
> + if [[ $_arch == "aarch64" ]]; then
> + local _installed_64K _running_64K
> +
> + _installed_64K=0
> + #echo "$_installed_kernel" | grep 64k
> + if echo "$_installed_kernel" | grep 64k; then
> + _installed_64K=1
> + fi
> + if echo "$_running_kernel" | grep 64k; then
> + _running_64K=1
> + fi
> +
> + # Otherwise, fall through to common case
> + if [[ $_installed_64K != "$_running_64K" ]]; then
> + # if the difference is 4K VS 64K
> + _crashkernel_running=$(kdump_get_arch_recommend_crashkernel
"kdump" "$_installed_kernel")
> + _update_kernel_cmdline "$_installed_kernel"
"$_crashkernel_running" "$_dump_mode_running"
"$_fadump_val_running"
> + return
> + fi
> + fi
> +
> if [[ $_crashkernel != "$_crashkernel_running" ]]; then
> if _update_kernel_cmdline "$_installed_kernel"
"$_crashkernel_running" "$_dump_mode_running"
"$_fadump_val_running"; then
> echo "kexec-tools has reset $_installed_kernel to use the
new default crashkernel value $_crashkernel_running"
Coiby sent the patch "Simplify the management of the kernel parameter
crashkernel" to the mailing list where he removes this part of
reset_crashkernel_for_installed_kernel. I believe you can drop
the changes to kdumpctl.
It is a great relief. I will rebase onto it.
Again, appreciate for your help.
Regards,
Pingfan