Hi Kairui,
Thank you for the review.
I adopt all of your suggestion, and will send out v2.
On 1/27/21 4:17 PM, Kairui Song wrote:
Hi Pingfan, this patch is great and looks good to me, just a few
nitpick:
On Thu, Jan 21, 2021 at 10:40 AM Pingfan Liu <piliu(a)redhat.com> wrote:
>
> There is requirement to decide the recommended memory size for the current
> system. Ant the algorithm is based on /proc/iomem, so it can align with the
> algorithm used by reserve_crashkernel() in kernel.
>
> Signed-off-by: Pingfan Liu <piliu(a)redhat.com>
> ---
> V1 -> V2:
> embeded in kdump-lib.sh
> add check against /proc/iomem
> kdump-lib.sh | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index d2801da..6bb285d 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -823,3 +823,71 @@ prepare_cmdline()
>
> echo ${cmdline}
> }
> +
> +#get system memory size in the unit of GB
> +get_system_size()
> +{
> + result=$( cat /proc/iomem | grep "System RAM" | awk -F
":" {' print $1 '} | tr [:lower:] [:upper:] | paste -sd+ )
Maybe better remove the space in the beginning of the subshell, and
replace awk {'print $1'} with awk '{print $1}'.
This statement seems can confuse the shell, I run into a strange error
while testing this patch, after remove the leading and tailing space,
and fix the brace issue, it works perfectly.
Will fix all of such issue of each awk expression.
> + result="+$result"
> + # replace '-' with '+0x' and '+' with '-0x'
> + sum=$( echo $result | sed -e 's/-/K0x/g' | sed -e 's/+/-0x/g'
| sed -e 's/K/+/g' )
> + size=$(printf "%d\n" $(($sum)))
> + let size=$size/1024/1024/1024
> +
> + echo $size
> +}
> +
> +get_recommend_size()
> +{
> + local mem_size=$1
> + local _ck_cmdline=$2
> +
> + last_sz=""
> + last_unit=""
> +
> + IFS=','
> + for i in $_ck_cmdline
> + do
Better keep the old IFS value somewhere and restore later, eg.
local OLDIFS="$IFS"
IFS=,
for i in $_ck_cmdline; do
....
done
IFS="$OLDIFS"
Yes, you are right. unset IFS does not work
> + end=$( echo $i | awk -F "-" {'
print $2 '} | awk -F ":" {' print $1 '} )
> + recommend=$( echo $i | awk -F "-" {' print $2 '} |
awk -F ":" {' print $2 '} )
Same as above, better keep the brace inside the quote, shell may not
recognize it under some rare condition.
> + size=${end: : -1}
> + unit=${end: -1}
> + if [ $unit == 'T' ]; then
> + let size=$size*1024
> + fi
> + if [ $mem_size -lt $size ]; then
> + echo $recommend
> + unset IFS
> + return
> + fi
> + done
> + unset IFS
> +}
> +
> +# return recommended size based on current system RAM size
> +kdump_api_get_arch_recommend_size()
After a second thought, is this name a bit long? Maybe just
kdump_get_arch_recommend_size is good enough?
OK
Thanks,
Pingfan
> +{
> + if ! [[ -r "/proc/iomem" ]] ; then
> + echo "Error, can not access /proc/iomem."
> + return 1
> + fi
> + arch=$( lscpu | grep Architecture | awk -F ":" {' print $2
'} | tr [:lower:] [:upper:] )
> +
> + if [ $arch == "X86_64" ] || [ $arch == "S390" ]; then
> + ck_cmdline="1G-4G:160M,4G-64G:192M,64G-1T:256M,1T-:512M"
> + elif [ $arch == "ARM64" ]; then
> + ck_cmdline="2G-:448M"
> + elif [ $arch == "PPC64LE" ]; then
> + if is_fadump_capable; 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"
> + else
> +
ck_cmdline="2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G"
> + fi
> + fi
> +
> + ck_cmdline=$( echo $ck_cmdline | sed -e 's/-:/-102400T:/g' )
> + sys_mem=$(get_system_size)
> + result=$( get_recommend_size $sys_mem "$ck_cmdline" )
> + echo $result
> + return 0
> +}
> --
> 2.29.2
>
--
Best Regards,
Kairui Song