11:19 p.m.
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.
Note
1. the function has been simplified as well.
2. define PROC_IOMEM as /proc/iomem for the sake of unit tests
Reported-by: Ruowen Qin <ruqin(a)redhat.com>
Fixes: 1813189 ("kdump-lib.sh: introduce functions to return recommened mem
size")
Suggested-by: Philipp Rudo <prudo(a)redhat.com>
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
kdump-lib.sh | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh
index 557eff6..ed28df3 100755
--- a/kdump-lib.sh
+++ b/kdump-lib.sh
@@ -788,17 +788,12 @@ prepare_cmdline()
echo "$cmdline"
}
-#get system memory size in the unit of GB
+PROC_IOMEM=/proc/iomem
+#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')
- size=$(printf "%d\n" $((sum)))
- size=$((size / 1024 / 1024 / 1024))
-
- echo "$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))
}
get_recommend_size()
--
2.35.3
11:19 p.m.
New subject: [PATCH v2 2/3] improve get_recommend_size
This patch rewrites get_recommend_size to get rid of the following
limitations,
1. only supports ranges in crashkernel sorted in increasing order
2. the first entry of crashkernel should have only a single digit and
it's in gigabytes
Suggested-by: Philipp Rudo <prudo(a)redhat.com>
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
kdump-lib.sh | 47 +++++++++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh
index ed28df3..eba2c38 100755
--- a/kdump-lib.sh
+++ b/kdump-lib.sh
@@ -796,33 +796,40 @@ get_system_size()
echo $(( (sum) / 1024 / 1024 / 1024))
}
+# Return the recommended size for the reserved crashkernel memory
+# depending on the system memory size.
+#
+# 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.
get_recommend_size()
{
local mem_size=$1
local _ck_cmdline=$2
- local OLDIFS="$IFS"
+ local range start start_unit end end_unit size
- start=${_ck_cmdline::1}
- if [[ $mem_size -lt $start ]]; then
- echo "0M"
- return
- fi
- IFS=','
- for i in $_ck_cmdline; do
- end=$(echo "$i" | awk -F "-" '{ print $2 }' | awk -F
":" '{ print $1 }')
- recommend=$(echo "$i" | awk -F "-" '{ print $2 }' | awk -F
":" '{ print $2 }')
- size=${end::-1}
- unit=${end: -1}
- if [[ $unit == 'T' ]]; then
- size=$((size * 1024))
- fi
- if [[ $mem_size -lt $size ]]; then
- echo "$recommend"
- IFS="$OLDIFS"
+ while read -r -d , range; do
+ # need to use non-default IFS as double spaces are used as a
+ # single delimiter while commas aren't...
+ IFS=, read start start_unit end end_unit size <<< \
+ "$(echo "$range" | sed -n
"s/\([0-9]\+\)\([GT]\?\)-\([0-9]*\)\([GT]\?\):\([0-9]\+[MG]\)/\1,\2,\3,\4,\5/p")"
+
+ # aka. 102400T
+ end=${end:-104857600}
+ [[ "$end_unit" == T ]] && end=$((end * 1024))
+ [[ "$start_unit" == T ]] && start=$((start * 1024))
+
+ if [[ $mem_size -ge $start ]] && [[ $mem_size -lt $end ]]; then
+ echo "$size"
return
fi
- done
- IFS="$OLDIFS"
+
+ # append a ',' as read expects the 'file' to end with a delimiter
+ done <<< "$_ck_cmdline,"
+
+ # no matching range found
+ echo "0M"
}
# get default crashkernel
--
2.35.3
5:08 a.m.
New subject: [PATCH v2 2/3] improve get_recommend_size
On Thu, 12 May 2022 12:19:23 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
This patch rewrites get_recommend_size to get rid of the following
limitations,
1. only supports ranges in crashkernel sorted in increasing order
2. the first entry of crashkernel should have only a single digit and
it's in gigabytes
Suggested-by: Philipp Rudo <prudo(a)redhat.com>
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
kdump-lib.sh | 47 +++++++++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh
index ed28df3..eba2c38 100755
--- a/kdump-lib.sh
+++ b/kdump-lib.sh
@@ -796,33 +796,40 @@ get_system_size()
echo $(( (sum) / 1024 / 1024 / 1024))
}
+# Return the recommended size for the reserved crashkernel memory
+# depending on the system memory size.
+#
+# 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.
the comment is much better now. But I'm still curious why to reference
reserve_crashkernel here. For me this function needs to copy the
behavior of parse_crashkernel_mem. So I would rather reference this
functions.
Anyway, it's only a small nit
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
get_recommend_size()
{
local mem_size=$1
local _ck_cmdline=$2
- local OLDIFS="$IFS"
+ local range start start_unit end end_unit size
- start=${_ck_cmdline::1}
- if [[ $mem_size -lt $start ]]; then
- echo "0M"
- return
- fi
- IFS=','
- for i in $_ck_cmdline; do
- end=$(echo "$i" | awk -F "-" '{ print $2 }' | awk -F
":" '{ print $1 }')
- recommend=$(echo "$i" | awk -F "-" '{ print $2 }' | awk -F
":" '{ print $2 }')
- size=${end::-1}
- unit=${end: -1}
- if [[ $unit == 'T' ]]; then
- size=$((size * 1024))
- fi
- if [[ $mem_size -lt $size ]]; then
- echo "$recommend"
- IFS="$OLDIFS"
+ while read -r -d , range; do
+ # need to use non-default IFS as double spaces are used as a
+ # single delimiter while commas aren't...
+ IFS=, read start start_unit end end_unit size <<< \
+ "$(echo "$range" | sed -n
"s/\([0-9]\+\)\([GT]\?\)-\([0-9]*\)\([GT]\?\):\([0-9]\+[MG]\)/\1,\2,\3,\4,\5/p")"
+
+ # aka. 102400T
+ end=${end:-104857600}
+ [[ "$end_unit" == T ]] && end=$((end * 1024))
+ [[ "$start_unit" == T ]] && start=$((start * 1024))
+
+ if [[ $mem_size -ge $start ]] && [[ $mem_size -lt $end ]]; then
+ echo "$size"
return
fi
- done
- IFS="$OLDIFS"
+
+ # append a ',' as read expects the 'file' to end with a delimiter
+ done <<< "$_ck_cmdline,"
+
+ # no matching range found
+ echo "0M"
}
# get default crashkernel
10:09 p.m.
New subject: [PATCH v2 2/3] improve get_recommend_size
On Thu, May 12, 2022 at 12:08:46PM +0200, Philipp Rudo wrote:
On Thu, 12 May 2022 12:19:23 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> This patch rewrites get_recommend_size to get rid of the following
> limitations,
> 1. only supports ranges in crashkernel sorted in increasing order
> 2. the first entry of crashkernel should have only a single digit and
> it's in gigabytes
>
> Suggested-by: Philipp Rudo <prudo(a)redhat.com>
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> kdump-lib.sh | 47 +++++++++++++++++++++++++++--------------------
> 1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index ed28df3..eba2c38 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -796,33 +796,40 @@ get_system_size()
> echo $(( (sum) / 1024 / 1024 / 1024))
> }
>
> +# Return the recommended size for the reserved crashkernel memory
> +# depending on the system memory size.
> +#
> +# 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.
the comment is much better now. But I'm still curious why to reference
reserve_crashkernel here. For me this function needs to copy the
behavior of parse_crashkernel_mem. So I would rather reference this
functions.
Yes, parse_crashkernel_mem is better. I referenced it because the
original commit mentioned reserve_crashkernel. I've merged this patch
with your suggestion applied. Thanks!
Anyway, it's only a small nit
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
> get_recommend_size()
> {
> local mem_size=$1
> local _ck_cmdline=$2
> - local OLDIFS="$IFS"
> + local range start start_unit end end_unit size
>
> - start=${_ck_cmdline::1}
> - if [[ $mem_size -lt $start ]]; then
> - echo "0M"
> - return
> - fi
> - IFS=','
> - for i in $_ck_cmdline; do
> - end=$(echo "$i" | awk -F "-" '{ print $2 }' | awk -F
":" '{ print $1 }')
> - recommend=$(echo "$i" | awk -F "-" '{ print $2 }' |
awk -F ":" '{ print $2 }')
> - size=${end::-1}
> - unit=${end: -1}
> - if [[ $unit == 'T' ]]; then
> - size=$((size * 1024))
> - fi
> - if [[ $mem_size -lt $size ]]; then
> - echo "$recommend"
> - IFS="$OLDIFS"
> + while read -r -d , range; do
> + # need to use non-default IFS as double spaces are used as a
> + # single delimiter while commas aren't...
> + IFS=, read start start_unit end end_unit size <<< \
> + "$(echo "$range" | sed -n
"s/\([0-9]\+\)\([GT]\?\)-\([0-9]*\)\([GT]\?\):\([0-9]\+[MG]\)/\1,\2,\3,\4,\5/p")"
> +
> + # aka. 102400T
> + end=${end:-104857600}
> + [[ "$end_unit" == T ]] && end=$((end * 1024))
> + [[ "$start_unit" == T ]] && start=$((start * 1024))
> +
> + if [[ $mem_size -ge $start ]] && [[ $mem_size -lt $end ]]; then
> + echo "$size"
> return
> fi
> - done
> - IFS="$OLDIFS"
> +
> + # append a ',' as read expects the 'file' to end with a delimiter
> + done <<< "$_ck_cmdline,"
> +
> + # no matching range found
> + echo "0M"
> }
>
> # get default crashkernel
--
Best regards,
Coiby
11:19 p.m.
New subject: [PATCH v2 3/3] unit tests: add tests for get_system_size and get_recommend_size
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
spec/kdump-lib_spec.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 spec/kdump-lib_spec.sh
diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh
new file mode 100644
index 0000000..ca8f79e
--- /dev/null
+++ b/spec/kdump-lib_spec.sh
@@ -0,0 +1,51 @@
+#!/bin/bash
+Describe 'kdump-lib'
+ Include ./kdump-lib.sh
+
+ Describe 'get_system_size()'
+
+ PROC_IOMEM=$(mktemp -t spec_test_proc_iomem_file.XXXXXXXXXX)
+
+ cleanup() {
+ rm -rf "$PROC_IOMEM"
+ }
+
+ AfterAll 'cleanup'
+
+ ONE_GIGABYTE='000000-3fffffff : System RAM'
+ Parameters
+ 1
+ 3
+ End
+
+ It 'should return correct system RAM size'
+ echo -n >$PROC_IOMEM
+ for _ in $(seq 1 "$1"); do echo "$ONE_GIGABYTE"
>>$PROC_IOMEM; done
+ When call get_system_size
+ The output should equal "$1"
+ End
+
+ End
+
+ Describe 'get_recommend_size()'
+ # Testing stragety:
+ # 1. inclusive for the lower bound of the range of crashkernel
+ # 2. exclusive for the upper bound of the range of crashkernel
+ # 3. supports ranges not sorted in increasing order
+
+ ck="4G-64G:256M,2G-4G:192M,64G-1T:512M,1T-:12345M"
+ Parameters
+ 1 0M
+ 2 192M
+ 64 512M
+ 1024 12345M
+ "$((64 * 1024))" 12345M
+ End
+
+ It 'should handle all cases correctly'
+ When call get_recommend_size "$1" $ck
+ The output should equal "$2"
+ End
+ End
+
+End
--
2.35.3
5:09 a.m.
New subject: [PATCH v2 3/3] unit tests: add tests for get_system_size and
get_recommend_size
Hi Coiby,
On Thu, 12 May 2022 12:19:24 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
looks good. Thanks!
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
---
spec/kdump-lib_spec.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 spec/kdump-lib_spec.sh
diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh
new file mode 100644
index 0000000..ca8f79e
--- /dev/null
+++ b/spec/kdump-lib_spec.sh
@@ -0,0 +1,51 @@
+#!/bin/bash
+Describe 'kdump-lib'
+ Include ./kdump-lib.sh
+
+ Describe 'get_system_size()'
+
+ PROC_IOMEM=$(mktemp -t spec_test_proc_iomem_file.XXXXXXXXXX)
+
+ cleanup() {
+ rm -rf "$PROC_IOMEM"
+ }
+
+ AfterAll 'cleanup'
+
+ ONE_GIGABYTE='000000-3fffffff : System RAM'
+ Parameters
+ 1
+ 3
+ End
+
+ It 'should return correct system RAM size'
+ echo -n >$PROC_IOMEM
+ for _ in $(seq 1 "$1"); do echo "$ONE_GIGABYTE"
>>$PROC_IOMEM; done
+ When call get_system_size
+ The output should equal "$1"
+ End
+
+ End
+
+ Describe 'get_recommend_size()'
+ # Testing stragety:
+ # 1. inclusive for the lower bound of the range of crashkernel
+ # 2. exclusive for the upper bound of the range of crashkernel
+ # 3. supports ranges not sorted in increasing order
+
+ ck="4G-64G:256M,2G-4G:192M,64G-1T:512M,1T-:12345M"
+ Parameters
+ 1 0M
+ 2 192M
+ 64 512M
+ 1024 12345M
+ "$((64 * 1024))" 12345M
+ End
+
+ It 'should handle all cases correctly'
+ When call get_recommend_size "$1" $ck
+ The output should equal "$2"
+ End
+ End
+
+End
10:11 p.m.
New subject: [PATCH v2 3/3] unit tests: add tests for get_system_size and
get_recommend_size
On Thu, May 12, 2022 at 12:09:40PM +0200, Philipp Rudo wrote:
Hi Coiby,
On Thu, 12 May 2022 12:19:24 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
looks good. Thanks!
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
Patch merged after fixing two shellcheck warnings caused by not quoting
$PROC_IOMEM, thanks for reviewing the patch!
> ---
> spec/kdump-lib_spec.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 spec/kdump-lib_spec.sh
>
> diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh
> new file mode 100644
> index 0000000..ca8f79e
> --- /dev/null
> +++ b/spec/kdump-lib_spec.sh
> @@ -0,0 +1,51 @@
> +#!/bin/bash
> +Describe 'kdump-lib'
> + Include ./kdump-lib.sh
> +
> + Describe 'get_system_size()'
> +
> + PROC_IOMEM=$(mktemp -t spec_test_proc_iomem_file.XXXXXXXXXX)
> +
> + cleanup() {
> + rm -rf "$PROC_IOMEM"
> + }
> +
> + AfterAll 'cleanup'
> +
> + ONE_GIGABYTE='000000-3fffffff : System RAM'
> + Parameters
> + 1
> + 3
> + End
> +
> + It 'should return correct system RAM size'
> + echo -n >$PROC_IOMEM
> + for _ in $(seq 1 "$1"); do echo "$ONE_GIGABYTE"
>>$PROC_IOMEM; done
> + When call get_system_size
> + The output should equal "$1"
> + End
> +
> + End
> +
> + Describe 'get_recommend_size()'
> + # Testing stragety:
> + # 1. inclusive for the lower bound of the range of crashkernel
> + # 2. exclusive for the upper bound of the range of crashkernel
> + # 3. supports ranges not sorted in increasing order
> +
> + ck="4G-64G:256M,2G-4G:192M,64G-1T:512M,1T-:12345M"
> + Parameters
> + 1 0M
> + 2 192M
> + 64 512M
> + 1024 12345M
> + "$((64 * 1024))" 12345M
> + End
> +
> + It 'should handle all cases correctly'
> + When call get_recommend_size "$1" $ck
> + The output should equal "$2"
> + End
> + End
> +
> +End
--
Best regards,
Coiby
5:05 a.m.
On Thu, 12 May 2022 12:19:22 +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.
Note
1. the function has been simplified as well.
2. define PROC_IOMEM as /proc/iomem for the sake of unit tests
Reported-by: Ruowen Qin <ruqin(a)redhat.com>
Fixes: 1813189 ("kdump-lib.sh: introduce functions to return recommened mem
size")
Suggested-by: Philipp Rudo <prudo(a)redhat.com>
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
great work!
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
---
kdump-lib.sh | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh
index 557eff6..ed28df3 100755
--- a/kdump-lib.sh
+++ b/kdump-lib.sh
@@ -788,17 +788,12 @@ prepare_cmdline()
echo "$cmdline"
}
-#get system memory size in the unit of GB
+PROC_IOMEM=/proc/iomem
+#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')
- size=$(printf "%d\n" $((sum)))
- size=$((size / 1024 / 1024 / 1024))
-
- echo "$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))
}
get_recommend_size()
10:09 p.m.
On Thu, May 12, 2022 at 12:05:53PM +0200, Philipp Rudo wrote:
On Thu, 12 May 2022 12:19:22 +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.
>
> Note
> 1. the function has been simplified as well.
> 2. define PROC_IOMEM as /proc/iomem for the sake of unit tests
>
> Reported-by: Ruowen Qin <ruqin(a)redhat.com>
> Fixes: 1813189 ("kdump-lib.sh: introduce functions to return recommened mem
size")
> Suggested-by: Philipp Rudo <prudo(a)redhat.com>
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
great work!
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
Patch merged, thanks!
> ---
> kdump-lib.sh | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index 557eff6..ed28df3 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -788,17 +788,12 @@ prepare_cmdline()
> echo "$cmdline"
> }
>
> -#get system memory size in the unit of GB
> +PROC_IOMEM=/proc/iomem
> +#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')
> - size=$(printf "%d\n" $((sum)))
> - size=$((size / 1024 / 1024 / 1024))
> -
> - echo "$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))
> }
>
> get_recommend_size()
--
Best regards,
Coiby
713
days inactive
714
days old
8 comments
2 participants
participants (2)
-
Coiby Xu -
Philipp Rudo