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>
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?
}
+# 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/
+# kernel.
All in all I'm not really sure what you mean with "consistent with the
reserve_crashkernel".
+# 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
Thanks
Philipp
get_recommend_size()
{
local mem_size=$1