[PATCH] kdump-lib.sh: introduce functions to return recommened mem size

[Pingfan Liu]

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&gt;
---
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+ )
+	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
+		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
+			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()
+{
+	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

[Kairui Song]

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&gt; 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&gt;
>  ---
>  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.

>  +       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"

>  +               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?

>  +{
>  +       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

[piliu]

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&gt; 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&gt;
> > ---
> > 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
>