Hi Philipp,
On Tue, May 10, 2022 at 03:30:52PM +0200, Philipp Rudo wrote:
Hi Coiby,
On Sat, 7 May 2022 16:48:55 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Recently, it's found 'kdumpctl estimate' returns 512M while the system
> reserves 1024M kdump memory in a case. This happens because the ranges
> in /proc/iomem are inclusively. For example, "0-1: System RAM" means 2
> bytes of system memory other than 1 byte. Fix this error by adding one
> more byte.
good catch! All in all the patch looks good. A few comments on the
comment. Plus the whole function could be simplified...
Nevertheless
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
Thanks for reviewing the patch!
> Note some documentations are added as well.
>
> Reported-by: Ruowen Qin <ruqin(a)redhat.com>
> Fixes: 1813189 ("kdump-lib.sh: introduce functions to return recommened mem
size")
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> kdump-lib.sh | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index 557eff6..f34f477 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -788,19 +788,26 @@ prepare_cmdline()
> echo "$cmdline"
> }
>
> -#get system memory size in the unit of GB
> +#get system memory size i.e. memblock.memory.total_size in the unit of GB
> get_system_size()
> {
> result=$(grep "System RAM" /proc/iomem | awk -F ":" '{
print $1 }' | tr "[:lower:]" "[:upper:]" | paste -sd+)
> result="+$result"
> # replace '-' with '+0x' and '+' with '-0x'
> - sum=$(echo "$result" | sed -e 's/-/K0x/g' -e 's/+/-0x/g'
-e 's/K/+/g')
> + sum=$(echo "$result" | sed -e 's/-/K0x/g' -e 's/+/-0x/g'
-e 's/K/+01+/g')
> size=$(printf "%d\n" $((sum)))
> size=$((size / 1024 / 1024 / 1024))
>
> echo "$size"
the way I see it this function can be simplified to two lines
sum=$(sed -n "s/\s*\([0-9a-fA-F]\+\)-\([0-9a-fA-F]\+\) : System RAM$/+ 0x\2 - 0x\1 +
1/p" /proc/iomem)
echo $(( ($sum) / 1024 / 1024 / 1024))
or am I missing something?
This solution is much more elegant, thanks! In v2, I made two small
changes to get rid of a shellcheck warning and also to be able to
unit-test this function,
PROC_IOMEM=/proc/iomem
#get system memory size i.e. memblock.memory.total_size in the unit of GB
get_system_size()
{
sum=$(sed -n "s/\s*\([0-9a-fA-F]\+\)-\([0-9a-fA-F]\+\) : System RAM$/+ 0x\2 - 0x\1 +
1/p" $PROC_IOMEM)
echo $(( (sum) / 1024 / 1024 / 1024))
}
> }
>
> +# Return the memory size given the system memory size and
I find it a little bit confusing that you are using "memory size" here
twice with different meaning. How about this?
Return the recommended size for the reserved crashkernel memory
depending on the systems total memory size.
> +# crashkernel=range1:size1[,range2:size2,…].
> +#
> +# This functions is expected to be consisteng with the reserve_crashkernel() in
s/consisteng/consistent/
Applied to v2, thanks!
> +# kernel.
All in all I'm not really sure what you mean with "consistent with the
reserve_crashkernel".
In v2, I improve the text as follows,
# This functions is expected to be consistent with the reserve_crashkernel() in
# kernel i.e. how kernel allocates the kdump memory given the crashkernel
# parameter crashkernel=range1:size1[,range2:size2,…] and the system memory
# size.
Hopefully the meaning is clear now.
> +# Note the ranges of crashkernel should be in increasing order.
s/should be/must be/
when the function cannot handle any other order it is mandatory that
the ranges are increasing --> must be
Note I've adopted your rewritten get_recommend_size in another patch in v2
so this suggestion doesn't apply any more.
Thanks
Philipp
> get_recommend_size()
> {
> local mem_size=$1
--
Best regards,
Coiby