[RESEND PATCH v2 1/2] kdumpctl: change prepare_cmdline() to operate KDUMP_COMMANDLINE directly

[Xunlei Pang]

Since KDUMP_COMMANDLINE is a global variable, prepare_cmdline can
modify it directly instead of echoing back the result. This change
enables it to output messages.

Changed some coding styles.

Signed-off-by: Xunlei Pang 
---
v1->v2:
Changed some comments.

kdumpctl | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index cb6f004..b2068cc 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -105,10 +105,12 @@ append_cmdline()
    echo $cmdline
 }

-# This function performs a series of edits on the command line
+# This function performs a series of edits on the command line.
+# Store the final result in global $KDUMP_COMMANDLINE.
 prepare_cmdline()
 {
-	local cmdline;
+	local cmdline id
+
    if [ -z "$KDUMP_COMMANDLINE" ]; then
    	cmdline=`cat /proc/cmdline`
    	# 'root' parameter will cause kdump failure in live images
@@ -118,6 +120,7 @@ prepare_cmdline()
    else
    	cmdline=${KDUMP_COMMANDLINE}
    fi
+
    # These params should always be removed
    cmdline=`remove_cmdline_param "$cmdline" crashkernel panic_on_warn`
    # These params can be removed configurably
@@ -125,12 +128,12 @@ prepare_cmdline()

cmdline="${cmdline} ${KDUMP_COMMANDLINE_APPEND}"

-	local id=`get_bootcpu_initial_apicid`
+	id=`get_bootcpu_initial_apicid`
    if [ ! -z ${id} ] ; then
    	cmdline=`append_cmdline "${cmdline}" disable_cpu_apicid ${id}`
    fi

-	echo $cmdline
+	KDUMP_COMMANDLINE=$cmdline
 }

@@ -642,7 +645,7 @@ load_kdump()
    	fi
    fi

-	KDUMP_COMMANDLINE=`prepare_cmdline`
+	prepare_cmdline

# For secureboot enabled machines, use new kexec file based syscall.
    # Old syscall will always fail as it does not have capability to

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

Check the number of cpus for x86_64 kdump kernel to boot with.
We met an issue for x86_64: kdump runs out of vectors with the
default "nr_cpus=1", when requesting tons of irqs.

This patch detects such situation and warns users about the risk.

Signed-off-by: Xunlei Pang 
---
v1->v2:
- When detecting risky cpu vectors, we just warn users instead of
  modifying "nr_cpus=X" forcely.
- Improved code comments.
- Replaced nr_old with nr_origin, and improved some logic.

kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index b2068cc..b6fc1f9 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -105,6 +105,85 @@ append_cmdline()
    echo $cmdline
 }

+# Check the number of cpus for kdump kernel to boot with.
+# We met an issue for x86_64: kdump runs out of vectors with
+# "nr_cpus=1" when requesting tons of irqs, so here we check
+# "nr_cpus=X" and warn users if kdump probably can't work.
+check_kdump_cpus()
+{
+	local nr nr_search nr_origin nr_min nr_max
+	local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
+
+	# Special treatment for x86_64 only currently.
+	if [ $arch != "x86_64" ]; then
+		return
+	fi
+
+	# We only care about "nr_cpus=X" format for x86.
+	nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
+	if [ $nr_search -eq 0 ] ; then
+		# Do not need to process if no valid "nr_cpus=X" specified.
+		return
+	fi
+
+	# Get value X of "nr_cpus=X"
+	nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
+	# In case there are multiple "nr_cpus=X", get the mininum value.
+	for nr in $nr_search; do
+		if [ $nr -gt 0 ]; then
+			nr_origin=$nr
+			break
+		fi
+	done
+	if [ -z "$nr_origin" ]; then
+		echo "Warning: Wrong "nr_cpus=" kernel cmdline detected"
+		return
+	fi
+
+	# Online cpus in first kernel.
+	nr_max=$(nproc)
+
+	# Calculate estimated minium cpus required by irqs(vectors).
+	nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
+
+	# We roughly use 256-32(see kernel FIRST_EXTERNAL_VECTOR)=224 as
+	# maximum supported vectors can be allocated to io devices percpu.
+	# As nr_min is a ballpart figure, also some high-numbered vectors
+	# are consumed by the kernel(see FIRST_SYSTEM_VECTOR), we need a
+	# variance for safety.
+	#
+	# We got a large machine with 240 cpus, 6TB memory, 8 iommus, and
+	# 12 io-apics, 132 irqs under /proc/irq/, it can boot successfully
+	# with "nr_cpus=1". (256-32-132)=92, so choosing 64 as the variance
+	# seems ok. Then we get the max external irqs supported per cpu:
+	# (256-32-64)=160 as the dividend.
+	nr_min=$(($nr_min + 160 - 1))
+	nr_min=$(($nr_min / 160))
+	if [ $nr_min -gt 1 ]; then
+		# The system seems to have tons of interrupts. while interrupts
+		# with multiple-cpu affinity can consume multiple vectors, with
+		# one vector for each cpu within the affinity mask. Fortunately
+		# for x2apic which is widely used on large modern machines, in
+		# default case of boot, device bringup etc will use a single cpu
+		# for the interrupt affinity to minimize vector pressure.
+		#
+		# For further safety, we add one more cpu and round it up to an
+		# even number which is commonly-used.
+		nr_min=$(($nr_min + 1))
+		nr_min=$(($nr_min + $nr_min % 2))
+	fi
+
+	if [ $nr_min -gt $nr_max ]; then
+		nr_min=$nr_max
+	fi
+
+	if [ $nr_origin -ge $nr_min ]; then
+		return
+	fi
+
+	echo "Warning: CPU vectors under pressure with "nr_cpus=$nr_origin", please try "nr_cpus=$nr_min" or more"
+}
+
 # This function performs a series of edits on the command line.
 # Store the final result in global $KDUMP_COMMANDLINE.
 prepare_cmdline()
@@ -134,6 +213,8 @@ prepare_cmdline()
    fi

KDUMP_COMMANDLINE=$cmdline
+
+	check_kdump_cpus
 }

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Dave Young]

Hi, Xunlei

Thanks for the patch, a few comments replied inline.
On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> Check the number of cpus for x86_64 kdump kernel to boot with.
> We met an issue for x86_64: kdump runs out of vectors with the
> default "nr_cpus=1", when requesting tons of irqs.
>
> This patch detects such situation and warns users about the risk.
> Signed-off-by: Xunlei Pang
>
> v1->v2:
> When detecting risky cpu vectors, we just warn users instead of
> modifying "nr_cpus=X" forcely.
> Improved code comments.
> Replaced nr_old with nr_origin, and improved some logic.
>
> kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index b2068cc..b6fc1f9 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -105,6 +105,85 @@ append_cmdline()
>    echo $cmdline
>  }
>
> +# Check the number of cpus for kdump kernel to boot with.
> +# We met an issue for x86_64: kdump runs out of vectors with
> +# "nr_cpus=1" when requesting tons of irqs, so here we check
> +# "nr_cpus=X" and warn users if kdump probably can't work.
> +check_kdump_cpus()
> +{
> local nr nr_search nr_origin nr_min nr_max
> local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> # Special treatment for x86_64 only currently.
> if [ $arch != "x86_64" ]; then
> return
> fi
> # We only care about "nr_cpus=X" format for x86.
> nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
> if [ $nr_search -eq 0 ] ; then
> # Do not need to process if no valid "nr_cpus=X" specified.

This comment sounds not necessary..
> return
> fi
> # Get value X of "nr_cpus=X"

Ditto.
> nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)

Is it ok to check $nr_search -eq 0 here and drop the previous chunk?
> # In case there are multiple "nr_cpus=X", get the mininum value.
> for nr in $nr_search; do
> if [ $nr -gt 0 ]; then
> 	nr_origin=$nr
> 	break
> fi
> done

What is the convention in kernel for these duplicated params, is it
using the last one? We'd better to use same logic as kernel uses..
> if [ -z "$nr_origin" ]; then
> echo "Warning: Wrong \"nr_cpus=\" kernel cmdline detected"

In previous code "Do not need to process if no valid nr_cpus=X
specified", we could just return without warning. And is it possible to
detect this earlier without an additional if else?
> return
> fi
> # Online cpus in first kernel.
> nr_max=$(nproc)

The manpage says it is for:
"print the number of processing units available"
So does it means some policy like cgroup can control the value? If so
maybe we should get the number from other place like /proc/cpuinfo.
> # Calculate estimated minium cpus required by irqs(vectors).
> nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
> # We roughly use 256-32(see kernel FIRST_EXTERNAL_VECTOR)=224 as
> # maximum supported vectors can be allocated to io devices percpu.
> # As nr_min is a ballpart figure, also some high-numbered vectors
> # are consumed by the kernel(see FIRST_SYSTEM_VECTOR), we need a
> # variance for safety.
> #
> # We got a large machine with 240 cpus, 6TB memory, 8 iommus, and
> # 12 io-apics, 132 irqs under /proc/irq/, it can boot successfully
> # with "nr_cpus=1". (256-32-132)=92, so choosing 64 as the variance
> # seems ok. Then we get the max external irqs supported per cpu:
> # (256-32-64)=160 as the dividend.
> nr_min=$(($nr_min + 160 - 1))
> nr_min=$(($nr_min / 160))
> if [ $nr_min -gt 1 ]; then
> # The system seems to have tons of interrupts. while interrupts
> # with multiple-cpu affinity can consume multiple vectors, with
> # one vector for each cpu within the affinity mask. Fortunately
> # for x2apic which is widely used on large modern machines, in
> # default case of boot, device bringup etc will use a single cpu
> # for the interrupt affinity to minimize vector pressure.
> #
> # For further safety, we add one more cpu and round it up to an
> # even number which is commonly-used.
> nr_min=$(($nr_min + 1))
> nr_min=$(($nr_min + $nr_min % 2))
> fi
> if [ $nr_min -gt $nr_max ]; then
> nr_min=$nr_max
> fi
> if [ $nr_origin -ge $nr_min ]; then
> return
> fi
> echo "Warning: CPU vectors under pressure with "nr_cpus=$nr_origin", please try "nr_cpus=$nr_min" or more"

Drop "please" and the tech details should be better. How about below:
Warning: nr_cpus=$nr_origin is not enough for kdump kernel boot-up, try
nr_cpus=$nr_min or larger numbers instead.
> +}
>
> # This function performs a series of edits on the command line.
>  # Store the final result in global $KDUMP_COMMANDLINE.
>  prepare_cmdline()
> @@ -134,6 +213,8 @@ prepare_cmdline()
>    fi
>
> KDUMP_COMMANDLINE=$cmdline
> check_kdump_cpus
>
> }
>
> -- 
> 1.8.3.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

On 01/06/2017 at 04:46 PM, Dave Young wrote:
> Hi, Xunlei
>
> Thanks for the patch, a few comments replied inline.
> On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> > Check the number of cpus for x86_64 kdump kernel to boot with.
> > We met an issue for x86_64: kdump runs out of vectors with the
> > default "nr_cpus=1", when requesting tons of irqs.
> >
> > This patch detects such situation and warns users about the risk.
> > Signed-off-by: Xunlei Pang
> >
> > v1->v2:
> > When detecting risky cpu vectors, we just warn users instead of
> > modifying "nr_cpus=X" forcely.
> > Improved code comments.
> > Replaced nr_old with nr_origin, and improved some logic.
> >
> > kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index b2068cc..b6fc1f9 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -105,6 +105,85 @@ append_cmdline()
> >    echo $cmdline
> >  }
> >
> > +# Check the number of cpus for kdump kernel to boot with.
> > +# We met an issue for x86_64: kdump runs out of vectors with
> > +# "nr_cpus=1" when requesting tons of irqs, so here we check
> > +# "nr_cpus=X" and warn users if kdump probably can't work.
> > +check_kdump_cpus()
> > +{
> > local nr nr_search nr_origin nr_min nr_max
> > local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> > # Special treatment for x86_64 only currently.
> > if [ $arch != "x86_64" ]; then
> > return
> > fi
> > # We only care about "nr_cpus=X" format for x86.
> > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
> > if [ $nr_search -eq 0 ] ; then
> > # Do not need to process if no valid "nr_cpus=X" specified.
>
> This comment sounds not necessary..

ok, will remove it.
> > return
> > fi
> > # Get value X of "nr_cpus=X"
>
> Ditto.

ok
> > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
>
> Is it ok to check $nr_search -eq 0 here and drop the previous chunk?

It's on purpose, a little different if there is wrong "nr_cpus=" without numbers.
> > # In case there are multiple "nr_cpus=X", get the mininum value.
> > for nr in $nr_search; do
> > if [ $nr -gt 0 ]; then
> > 	nr_origin=$nr
> > 	break
> > fi
> > done
>
> What is the convention in kernel for these duplicated params, is it
> using the last one? We'd better to use same logic as kernel uses..

The one with the lowest valid number will be used.
> > if [ -z "$nr_origin" ]; then
> > echo "Warning: Wrong \"nr_cpus=\" kernel cmdline detected"
>
> In previous code "Do not need to process if no valid nr_cpus=X
> specified", we could just return without warning. And is it possible to
> detect this earlier without an additional if else?

This is for detecting "nr_cpus=0".
> > return
> > fi
> > # Online cpus in first kernel.
> > nr_max=$(nproc)
>
> The manpage says it is for:
> "print the number of processing units available"
> So does it means some policy like cgroup can control the value? If so
> maybe we should get the number from other place like /proc/cpuinfo.

Good point, will modify it.
> > # Calculate estimated minium cpus required by irqs(vectors).
> > nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
> > # We roughly use 256-32(see kernel FIRST_EXTERNAL_VECTOR)=224 as
> > # maximum supported vectors can be allocated to io devices percpu.
> > # As nr_min is a ballpart figure, also some high-numbered vectors
> > # are consumed by the kernel(see FIRST_SYSTEM_VECTOR), we need a
> > # variance for safety.
> > #
> > # We got a large machine with 240 cpus, 6TB memory, 8 iommus, and
> > # 12 io-apics, 132 irqs under /proc/irq/, it can boot successfully
> > # with "nr_cpus=1". (256-32-132)=92, so choosing 64 as the variance
> > # seems ok. Then we get the max external irqs supported per cpu:
> > # (256-32-64)=160 as the dividend.
> > nr_min=$(($nr_min + 160 - 1))
> > nr_min=$(($nr_min / 160))
> > if [ $nr_min -gt 1 ]; then
> > # The system seems to have tons of interrupts. while interrupts
> > # with multiple-cpu affinity can consume multiple vectors, with
> > # one vector for each cpu within the affinity mask. Fortunately
> > # for x2apic which is widely used on large modern machines, in
> > # default case of boot, device bringup etc will use a single cpu
> > # for the interrupt affinity to minimize vector pressure.
> > #
> > # For further safety, we add one more cpu and round it up to an
> > # even number which is commonly-used.
> > nr_min=$(($nr_min + 1))
> > nr_min=$(($nr_min + $nr_min % 2))
> > fi
> > if [ $nr_min -gt $nr_max ]; then
> > nr_min=$nr_max
> > fi
> > if [ $nr_origin -ge $nr_min ]; then
> > return
> > fi
> > echo "Warning: CPU vectors under pressure with "nr_cpus=$nr_origin", please try "nr_cpus=$nr_min" or more"
>
> Drop "please" and the tech details should be better. How about below:
> Warning: nr_cpus=$nr_origin is not enough for kdump kernel boot-up, try
> nr_cpus=$nr_min or larger numbers instead.

ok, will update. Thanks!

Regards,
Xunlei
> > +}
> >
> > # This function performs a series of edits on the command line.
> >  # Store the final result in global $KDUMP_COMMANDLINE.
> >  prepare_cmdline()
> > @@ -134,6 +213,8 @@ prepare_cmdline()
> >    fi
> >
> > KDUMP_COMMANDLINE=$cmdline
> > check_kdump_cpus
> >
> > }
> >
> > -- 
> > 1.8.3.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Dave Young]

On 01/06/17 at 05:10pm, Xunlei Pang wrote:
> On 01/06/2017 at 04:46 PM, Dave Young wrote:
> > Hi, Xunlei
> >
> > Thanks for the patch, a few comments replied inline.
> > On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> > > Check the number of cpus for x86_64 kdump kernel to boot with.
> > > We met an issue for x86_64: kdump runs out of vectors with the
> > > default "nr_cpus=1", when requesting tons of irqs.
> > >
> > > This patch detects such situation and warns users about the risk.
> > > Signed-off-by: Xunlei Pang
> > >
> > > v1->v2:
> > > When detecting risky cpu vectors, we just warn users instead of
> > > modifying "nr_cpus=X" forcely.
> > > Improved code comments.
> > > Replaced nr_old with nr_origin, and improved some logic.
> > >
> > > kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 81 insertions(+)
> > >
> > > diff --git a/kdumpctl b/kdumpctl
> > > index b2068cc..b6fc1f9 100755
> > > --- a/kdumpctl
> > > +++ b/kdumpctl
> > > @@ -105,6 +105,85 @@ append_cmdline()
> > >    echo $cmdline
> > >  }
> > >
> > > +# Check the number of cpus for kdump kernel to boot with.
> > > +# We met an issue for x86_64: kdump runs out of vectors with
> > > +# "nr_cpus=1" when requesting tons of irqs, so here we check
> > > +# "nr_cpus=X" and warn users if kdump probably can't work.
> > > +check_kdump_cpus()
> > > +{
> > > local nr nr_search nr_origin nr_min nr_max
> > > local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> > > # Special treatment for x86_64 only currently.
> > > if [ $arch != "x86_64" ]; then
> > > return
> > > fi
> > > # We only care about "nr_cpus=X" format for x86.
> > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
> > > if [ $nr_search -eq 0 ] ; then
> > > # Do not need to process if no valid "nr_cpus=X" specified.
> >
> > This comment sounds not necessary..
>
> ok, will remove it.
> > > return
> > > fi
> > > # Get value X of "nr_cpus=X"
> >
> > Ditto.
>
> ok
> > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
> >
> > Is it ok to check $nr_search -eq 0 here and drop the previous chunk?
>
> It's on purpose, a little different if there is wrong "nr_cpus=" without numbers.

How about only check for "nr_cpus=1" as we set it as default, if one set
as other value, they must have tested it. So that we do not need these
corner cases checking, then all the error handling can be dropped.
> > > # In case there are multiple "nr_cpus=X", get the mininum value.
> > > for nr in $nr_search; do
> > > if [ $nr -gt 0 ]; then
> > > 	nr_origin=$nr
> > > 	break
> > > fi
> > > done
> >
> > What is the convention in kernel for these duplicated params, is it
> > using the last one? We'd better to use same logic as kernel uses..
>
> The one with the lowest valid number will be used.
> > > if [ -z "$nr_origin" ]; then
> > > echo "Warning: Wrong \"nr_cpus=\" kernel cmdline detected"
> >
> > In previous code "Do not need to process if no valid nr_cpus=X
> > specified", we could just return without warning. And is it possible to
> > detect this earlier without an additional if else?
>
> This is for detecting "nr_cpus=0".
> > > return
> > > fi
> > > # Online cpus in first kernel.
> > > nr_max=$(nproc)
> >
> > The manpage says it is for:
> > "print the number of processing units available"
> > So does it means some policy like cgroup can control the value? If so
> > maybe we should get the number from other place like /proc/cpuinfo.
>
> Good point, will modify it.
> > > # Calculate estimated minium cpus required by irqs(vectors).
> > > nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
> > > # We roughly use 256-32(see kernel FIRST_EXTERNAL_VECTOR)=224 as
> > > # maximum supported vectors can be allocated to io devices percpu.
> > > # As nr_min is a ballpart figure, also some high-numbered vectors
> > > # are consumed by the kernel(see FIRST_SYSTEM_VECTOR), we need a
> > > # variance for safety.
> > > #
> > > # We got a large machine with 240 cpus, 6TB memory, 8 iommus, and
> > > # 12 io-apics, 132 irqs under /proc/irq/, it can boot successfully
> > > # with "nr_cpus=1". (256-32-132)=92, so choosing 64 as the variance
> > > # seems ok. Then we get the max external irqs supported per cpu:
> > > # (256-32-64)=160 as the dividend.
> > > nr_min=$(($nr_min + 160 - 1))
> > > nr_min=$(($nr_min / 160))
> > > if [ $nr_min -gt 1 ]; then
> > > # The system seems to have tons of interrupts. while interrupts
> > > # with multiple-cpu affinity can consume multiple vectors, with
> > > # one vector for each cpu within the affinity mask. Fortunately
> > > # for x2apic which is widely used on large modern machines, in
> > > # default case of boot, device bringup etc will use a single cpu
> > > # for the interrupt affinity to minimize vector pressure.
> > > #
> > > # For further safety, we add one more cpu and round it up to an
> > > # even number which is commonly-used.
> > > nr_min=$(($nr_min + 1))
> > > nr_min=$(($nr_min + $nr_min % 2))
> > > fi
> > > if [ $nr_min -gt $nr_max ]; then
> > > nr_min=$nr_max
> > > fi
> > > if [ $nr_origin -ge $nr_min ]; then
> > > return
> > > fi
> > > echo "Warning: CPU vectors under pressure with "nr_cpus=$nr_origin", please try "nr_cpus=$nr_min" or more"
> >
> > Drop "please" and the tech details should be better. How about below:
> > Warning: nr_cpus=$nr_origin is not enough for kdump kernel boot-up, try
> > nr_cpus=$nr_min or larger numbers instead.
>
> ok, will update. Thanks!
>
> Regards,
> Xunlei
> > > +}
> > >
> > > # This function performs a series of edits on the command line.
> > >  # Store the final result in global $KDUMP_COMMANDLINE.
> > >  prepare_cmdline()
> > > @@ -134,6 +213,8 @@ prepare_cmdline()
> > >    fi
> > >
> > > KDUMP_COMMANDLINE=$cmdline
> > > check_kdump_cpus
> > >
> > > }
> > >
> > > -- 
> > > 1.8.3.1
> > > _______________________________________________
> > > kexec mailing list -- [e-mail]
> > > To unsubscribe send an email to [e-mail]

Thanks
Dave

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

On 01/09/2017 at 11:10 AM, Dave Young wrote:
> On 01/06/17 at 05:10pm, Xunlei Pang wrote:
> > On 01/06/2017 at 04:46 PM, Dave Young wrote:
> > > Hi, Xunlei
> > >
> > > Thanks for the patch, a few comments replied inline.
> > > On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> > > > Check the number of cpus for x86_64 kdump kernel to boot with.
> > > > We met an issue for x86_64: kdump runs out of vectors with the
> > > > default "nr_cpus=1", when requesting tons of irqs.
> > > >
> > > > This patch detects such situation and warns users about the risk.
> > > > Signed-off-by: Xunlei Pang
> > > >
> > > > v1->v2:
> > > > When detecting risky cpu vectors, we just warn users instead of
> > > > modifying "nr_cpus=X" forcely.
> > > > Improved code comments.
> > > > Replaced nr_old with nr_origin, and improved some logic.
> > > >
> > > > kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 81 insertions(+)
> > > >
> > > > diff --git a/kdumpctl b/kdumpctl
> > > > index b2068cc..b6fc1f9 100755
> > > > --- a/kdumpctl
> > > > +++ b/kdumpctl
> > > > @@ -105,6 +105,85 @@ append_cmdline()
> > > >    echo $cmdline
> > > >  }
> > > >
> > > > +# Check the number of cpus for kdump kernel to boot with.
> > > > +# We met an issue for x86_64: kdump runs out of vectors with
> > > > +# "nr_cpus=1" when requesting tons of irqs, so here we check
> > > > +# "nr_cpus=X" and warn users if kdump probably can't work.
> > > > +check_kdump_cpus()
> > > > +{
> > > > local nr nr_search nr_origin nr_min nr_max
> > > > local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> > > > # Special treatment for x86_64 only currently.
> > > > if [ $arch != "x86_64" ]; then
> > > > return
> > > > fi
> > > > # We only care about "nr_cpus=X" format for x86.
> > > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
> > > > if [ $nr_search -eq 0 ] ; then
> > > > # Do not need to process if no valid "nr_cpus=X" specified.
> > >
> > > This comment sounds not necessary..
> >
> > ok, will remove it.
> > > > return
> > > > fi
> > > > # Get value X of "nr_cpus=X"
> > >
> > > Ditto.
> >
> > ok
> > > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
> > >
> > > Is it ok to check $nr_search -eq 0 here and drop the previous chunk?
> >
> > It's on purpose, a little different if there is wrong "nr_cpus=" without numbers.
>
> How about only check for "nr_cpus=1" as we set it as default, if one set
> as other value, they must have tested it. So that we do not need these
> corner cases checking, then all the error handling can be dropped.

ok, we can peek sysconfig/kdump, if there is nr_cpus=1, then activate the check.

Regards,
Xunlei

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

On 01/09/2017 at 11:10 AM, Dave Young wrote:
> On 01/06/17 at 05:10pm, Xunlei Pang wrote:
> > On 01/06/2017 at 04:46 PM, Dave Young wrote:
> > > Hi, Xunlei
> > >
> > > Thanks for the patch, a few comments replied inline.
> > > On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> > > > Check the number of cpus for x86_64 kdump kernel to boot with.
> > > > We met an issue for x86_64: kdump runs out of vectors with the
> > > > default "nr_cpus=1", when requesting tons of irqs.
> > > >
> > > > This patch detects such situation and warns users about the risk.
> > > > Signed-off-by: Xunlei Pang
> > > >
> > > > v1->v2:
> > > > When detecting risky cpu vectors, we just warn users instead of
> > > > modifying "nr_cpus=X" forcely.
> > > > Improved code comments.
> > > > Replaced nr_old with nr_origin, and improved some logic.
> > > >
> > > > kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 81 insertions(+)
> > > >
> > > > diff --git a/kdumpctl b/kdumpctl
> > > > index b2068cc..b6fc1f9 100755
> > > > --- a/kdumpctl
> > > > +++ b/kdumpctl
> > > > @@ -105,6 +105,85 @@ append_cmdline()
> > > >    echo $cmdline
> > > >  }
> > > >
> > > > +# Check the number of cpus for kdump kernel to boot with.
> > > > +# We met an issue for x86_64: kdump runs out of vectors with
> > > > +# "nr_cpus=1" when requesting tons of irqs, so here we check
> > > > +# "nr_cpus=X" and warn users if kdump probably can't work.
> > > > +check_kdump_cpus()
> > > > +{
> > > > local nr nr_search nr_origin nr_min nr_max
> > > > local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> > > > # Special treatment for x86_64 only currently.
> > > > if [ $arch != "x86_64" ]; then
> > > > return
> > > > fi
> > > > # We only care about "nr_cpus=X" format for x86.
> > > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
> > > > if [ $nr_search -eq 0 ] ; then
> > > > # Do not need to process if no valid "nr_cpus=X" specified.
> > >
> > > This comment sounds not necessary..
> >
> > ok, will remove it.
> > > > return
> > > > fi
> > > > # Get value X of "nr_cpus=X"
> > >
> > > Ditto.
> >
> > ok
> > > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
> > >
> > > Is it ok to check $nr_search -eq 0 here and drop the previous chunk?
> >
> > It's on purpose, a little different if there is wrong "nr_cpus=" without numbers.
>
> How about only check for "nr_cpus=1" as we set it as default, if one set
> as other value, they must have tested it. So that we do not need these
> corner cases checking, then all the error handling can be dropped.

Hi Dave,

What do you the following change?

---
 kdumpctl | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index b2068cc..4411ec5 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -105,6 +105,78 @@ append_cmdline()
     echo $cmdline
 }

+# Check the number of cpus for kdump kernel to boot with.
+# We met an issue for x86_64: kdump runs out of vectors with
+# "nr_cpus=1" when requesting tons of irqs, so here we check
+# "nr_cpus=1" and warn users if kdump probably can't work.
+check_kdump_cpus()
+{
+    local nr_origin nr_min nr_max
+    local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE_APPEND
+
+    # Special treatment for x86_64 only currently.
+    if [ $arch != "x86_64" ]; then
+        return
+    fi
+
+    # We only care about the default "nr_cpus=1".
+    echo $cmdline | grep -E -q "nr_cpus=1 |nr_cpus=1$"
+    if [ $? -ne 0 ]; then
+        return
+    fi
+
+    nr_origin=1
+
+    # Online cpus in first kernel.
+    nr_max=$(grep -c '^processor' /proc/cpuinfo)
+
+    # To calculate the estimated minimal cpus required by device interrupts.
+    nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
+
+    # The total number of vectors percpu is 256 defined by x86 architecture.
+    # The available vectors can be allocated to io devices percpu starts
+    # from FIRST_EXTERNAL_VECTOR(see kernel code), and some high-numbered
+    # ones are consumed by some system interrupts. As a result, the vectors
+    # for io device are within [FIRST_EXTERNAL_VECTOR, FIRST_SYSTEM_VECTOR),
+    # with one known exception, 0x80 within the range is reserved specially
+    # as the syscall vector.
+    #
+    # FIRST_EXTERNAL_VECTOR is invariably 32, while FIRST_SYSTEM_VECTOR can
+    # vary between different kernel versions. E.g. FIRST_SYSTEM_VECTOR gets
+    # 0xef(with CONFIG_X86_LOCAL_APIC on)for linux-4.10, that is 17 vectors
+    # reserved, considering it may increase in the future and the special
+    # vectors, we use a flexible variance and assume there are 32 reserved
+    # from FIRST_EXTERNAL_VECTOR. Then the max vectors for device interrupts
+    # percpu is: (256-32)-32=192.
+    #
+    # For "nr_cpus=1", irq and vector have the 1:1 mapping.
+    nr_min=$(($nr_min + 192 - 1))
+    nr_min=$(($nr_min / 192))
+    if [ $nr_min -gt 1 ]; then
+        # The system seems to have tons of interrupts. while interrupts with
+        # multiple-cpu affinity can consume multiple vectors(i.e. 1:M mapping),
+        # with one vector for each cpu within the affinity mask. Fortunately
+        # for x2apic which is widely used on large modern machines, in default
+        # case of boot, device bringup etc will use a single cpu for interrupt
+        # affinity to minimize vector pressure.
+        #
+        # For further safety, we add one more cpu and round it up to an even
+        # number which is commonly-used.
+        nr_min=$(($nr_min + 1))
+        nr_min=$(($nr_min + $nr_min % 2))
+    fi
+
+    if [ $nr_min -gt $nr_max ]; then
+        nr_min=$nr_max
+    fi
+
+    if [ $nr_origin -ge $nr_min ]; then
+        return
+    fi
+
+    echo "Warning: nr_cpus=$nr_origin may not be enough for kdump boot, try nr_cpus=$nr_min or larger instead"
+}
+
 # This function performs a series of edits on the command line.
 # Store the final result in global $KDUMP_COMMANDLINE.
 prepare_cmdline()
@@ -134,6 +206,8 @@ prepare_cmdline()
     fi

KDUMP_COMMANDLINE=$cmdline
+
+    check_kdump_cpus
 }

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Dave Young]

Hi, Xunlei,

I did not notice this mail..
On 01/12/17 at 10:22am, Xunlei Pang wrote:
> On 01/09/2017 at 11:10 AM, Dave Young wrote:
> > On 01/06/17 at 05:10pm, Xunlei Pang wrote:
> > > On 01/06/2017 at 04:46 PM, Dave Young wrote:
> > > > Hi, Xunlei
> > > >
> > > > Thanks for the patch, a few comments replied inline.
> > > > On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> > > > > Check the number of cpus for x86_64 kdump kernel to boot with.
> > > > > We met an issue for x86_64: kdump runs out of vectors with the
> > > > > default "nr_cpus=1", when requesting tons of irqs.
> > > > >
> > > > > This patch detects such situation and warns users about the risk.
> > > > > Signed-off-by: Xunlei Pang
> > > > >
> > > > > v1->v2:
> > > > > When detecting risky cpu vectors, we just warn users instead of
> > > > > modifying "nr_cpus=X" forcely.
> > > > > Improved code comments.
> > > > > Replaced nr_old with nr_origin, and improved some logic.
> > > > >
> > > > > kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 81 insertions(+)
> > > > >
> > > > > diff --git a/kdumpctl b/kdumpctl
> > > > > index b2068cc..b6fc1f9 100755
> > > > > --- a/kdumpctl
> > > > > +++ b/kdumpctl
> > > > > @@ -105,6 +105,85 @@ append_cmdline()
> > > > >    echo $cmdline
> > > > >  }
> > > > >
> > > > > +# Check the number of cpus for kdump kernel to boot with.
> > > > > +# We met an issue for x86_64: kdump runs out of vectors with
> > > > > +# "nr_cpus=1" when requesting tons of irqs, so here we check
> > > > > +# "nr_cpus=X" and warn users if kdump probably can't work.
> > > > > +check_kdump_cpus()
> > > > > +{
> > > > > local nr nr_search nr_origin nr_min nr_max
> > > > > local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> > > > > # Special treatment for x86_64 only currently.
> > > > > if [ $arch != "x86_64" ]; then
> > > > > return
> > > > > fi
> > > > > # We only care about "nr_cpus=X" format for x86.
> > > > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
> > > > > if [ $nr_search -eq 0 ] ; then
> > > > > # Do not need to process if no valid "nr_cpus=X" specified.
> > > >
> > > > This comment sounds not necessary..
> > >
> > > ok, will remove it.
> > > > > return
> > > > > fi
> > > > > # Get value X of "nr_cpus=X"
> > > >
> > > > Ditto.
> > >
> > > ok
> > > > > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
> > > >
> > > > Is it ok to check $nr_search -eq 0 here and drop the previous chunk?
> > >
> > > It's on purpose, a little different if there is wrong "nr_cpus=" without numbers.
> >
> > How about only check for "nr_cpus=1" as we set it as default, if one set
> > as other value, they must have tested it. So that we do not need these
> > corner cases checking, then all the error handling can be dropped.
>
> Hi Dave,
>
> What do you the following change?
>
> kdumpctl | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index b2068cc..4411ec5 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -105,6 +105,78 @@ append_cmdline()
>      echo $cmdline
>  }
>
> +# Check the number of cpus for kdump kernel to boot with.
> +# We met an issue for x86_64: kdump runs out of vectors with
> +# "nr_cpus=1" when requesting tons of irqs, so here we check
> +# "nr_cpus=1" and warn users if kdump probably can't work.
> +check_kdump_cpus()
> +{
> local nr_origin nr_min nr_max
> local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE_APPEND
> # Special treatment for x86_64 only currently.
> if [ $arch != "x86_64" ]; then
>    return
> fi
> # We only care about the default "nr_cpus=1".
> echo $cmdline | grep -E -q "nr_cpus=1 |nr_cpus=1$"
> if [ $? -ne 0 ]; then
>    return
> fi

I replied early but not catch the $ issue, maybe
grep -e "nr_cpus=1[[:space:]]*"

For the below parts about irq calculation, I would leave to Pratyush or
Bao to reivew..
> nr_origin=1
> # Online cpus in first kernel.
> nr_max=$(grep -c '^processor' /proc/cpuinfo)
> # To calculate the estimated minimal cpus required by device interrupts.
> nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
> # The total number of vectors percpu is 256 defined by x86 architecture.
> # The available vectors can be allocated to io devices percpu starts
> # from FIRST_EXTERNAL_VECTOR(see kernel code), and some high-numbered
> # ones are consumed by some system interrupts. As a result, the vectors
> # for io device are within [FIRST_EXTERNAL_VECTOR, FIRST_SYSTEM_VECTOR),
> # with one known exception, 0x80 within the range is reserved specially
> # as the syscall vector.
> #
> # FIRST_EXTERNAL_VECTOR is invariably 32, while FIRST_SYSTEM_VECTOR can
> # vary between different kernel versions. E.g. FIRST_SYSTEM_VECTOR gets
> # 0xef(with CONFIG_X86_LOCAL_APIC on)for linux-4.10, that is 17 vectors
> # reserved, considering it may increase in the future and the special
> # vectors, we use a flexible variance and assume there are 32 reserved
> # from FIRST_EXTERNAL_VECTOR. Then the max vectors for device interrupts
> # percpu is: (256-32)-32=192.
> #
> # For "nr_cpus=1", irq and vector have the 1:1 mapping.
> nr_min=$(($nr_min + 192 - 1))
> nr_min=$(($nr_min / 192))
> if [ $nr_min -gt 1 ]; then
>    # The system seems to have tons of interrupts. while interrupts with
>    # multiple-cpu affinity can consume multiple vectors(i.e. 1:M mapping),
>    # with one vector for each cpu within the affinity mask. Fortunately
>    # for x2apic which is widely used on large modern machines, in default
>    # case of boot, device bringup etc will use a single cpu for interrupt
>    # affinity to minimize vector pressure.
>    #
>    # For further safety, we add one more cpu and round it up to an even
>    # number which is commonly-used.
>    nr_min=$(($nr_min + 1))
>    nr_min=$(($nr_min + $nr_min % 2))
> fi
> if [ $nr_min -gt $nr_max ]; then
>    nr_min=$nr_max
> fi
> if [ $nr_origin -ge $nr_min ]; then
>    return
> fi
> echo "Warning: nr_cpus=$nr_origin may not be enough for kdump boot, try nr_cpus=$nr_min or larger instead"
>
> +}
>
> # This function performs a series of edits on the command line.
>  # Store the final result in global $KDUMP_COMMANDLINE.
>  prepare_cmdline()
> @@ -134,6 +206,8 @@ prepare_cmdline()
>      fi
>  KDUMP_COMMANDLINE=$cmdline
> check_kdump_cpus
>
> }
>
> -- 
> 1.8.3.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

Thanks
Dave

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Pratyush Anand]

Hi Xunlei,

Thanks a lot for making the things better and automating wherever it is 
possible :-)

On Friday 06 January 2017 12:07 PM, Xunlei Pang wrote:
> Check the number of cpus for x86_64 kdump kernel to boot with.
> We met an issue for x86_64: kdump runs out of vectors with the
> default "nr_cpus=1", when requesting tons of irqs.
>
> This patch detects such situation and warns users about the risk.
> Signed-off-by: Xunlei Pang
>
> v1->v2:
> When detecting risky cpu vectors, we just warn users instead of
> modifying "nr_cpus=X" forcely.
> Improved code comments.
> Replaced nr_old with nr_origin, and improved some logic.
>
> kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index b2068cc..b6fc1f9 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -105,6 +105,85 @@ append_cmdline()
>    echo $cmdline
>  }
>
> +# Check the number of cpus for kdump kernel to boot with.
> +# We met an issue for x86_64: kdump runs out of vectors with
> +# "nr_cpus=1" when requesting tons of irqs, so here we check
> +# "nr_cpus=X" and warn users if kdump probably can't work.
> +check_kdump_cpus()
> +{
> local nr nr_search nr_origin nr_min nr_max
> local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> # Special treatment for x86_64 only currently.
> if [ $arch != "x86_64" ]; then
> return
> fi
> # We only care about "nr_cpus=X" format for x86.
> nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)

may be we can just have
nr_cpus=$(echo $cmdline | grep -o "nr_cpus=[0-9]*")
> if [ $nr_search -eq 0 ] ; then

and the  can check for [[ -z $nr_cpus ]]
> # Do not need to process if no valid "nr_cpus=X" specified.
> return
> fi
> # Get value X of "nr_cpus=X"
> nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)

and same nr_cpus can be reused here then.
> # In case there are multiple "nr_cpus=X", get the mininum value.
> for nr in $nr_search; do
> if [ $nr -gt 0 ]; then
> 	nr_origin=$nr
> 	break
> fi
> done
> if [ -z "$nr_origin" ]; then
> echo "Warning: Wrong \"nr_cpus=\" kernel cmdline detected"
> return
> fi
> # Online cpus in first kernel.
> nr_max=$(nproc)
> # Calculate estimated minium cpus required by irqs(vectors).
> nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
> # We roughly use 256-32(see kernel FIRST_EXTERNAL_VECTOR)=224 as
> # maximum supported vectors can be allocated to io devices percpu.
> # As nr_min is a ballpart figure, also some high-numbered vectors
> # are consumed by the kernel(see FIRST_SYSTEM_VECTOR), we need a
> # variance for safety.
> #
> # We got a large machine with 240 cpus, 6TB memory, 8 iommus, and
> # 12 io-apics, 132 irqs under /proc/irq/, it can boot successfully
> # with "nr_cpus=1". (256-32-132)=92, so choosing 64 as the variance

why to guess that. If I have not missed anything then it seems that 
number of vectors needed by kernel is fixed.
 From arch/x86/include/asm/irq_vectors.h :

vector 128 seems fixed for system call.
If we have CONFIG_X86_LOCAL_APIC, then vector 0xef to 0xff are used by 
kernel. So, this variance should have fixed value as 1 for 
!CONFIG_X86_LOCAL_APIC and as 18 otherwise, no? So, may be we can take 
as 18 for all cases.
> # seems ok. Then we get the max external irqs supported per cpu:
> # (256-32-64)=160 as the dividend.
> nr_min=$(($nr_min + 160 - 1))
> nr_min=$(($nr_min / 160))
> if [ $nr_min -gt 1 ]; then
> # The system seems to have tons of interrupts. while interrupts
> # with multiple-cpu affinity can consume multiple vectors, with
> # one vector for each cpu within the affinity mask. Fortunately
> # for x2apic which is widely used on large modern machines, in
> # default case of boot, device bringup etc will use a single cpu
> # for the interrupt affinity to minimize vector pressure.
> #
> # For further safety, we add one more cpu and round it up to an
> # even number which is commonly-used.
> nr_min=$(($nr_min + 1))
> nr_min=$(($nr_min + $nr_min % 2))
> fi
> if [ $nr_min -gt $nr_max ]; then
> nr_min=$nr_max
> fi
> if [ $nr_origin -ge $nr_min ]; then
> return
> fi
> echo "Warning: CPU vectors under pressure with "nr_cpus=$nr_origin", please try "nr_cpus=$nr_min" or more"
>
> +}
>
> # This function performs a series of edits on the command line.
>  # Store the final result in global $KDUMP_COMMANDLINE.
>  prepare_cmdline()
> @@ -134,6 +213,8 @@ prepare_cmdline()
>    fi
>
> KDUMP_COMMANDLINE=$cmdline
> check_kdump_cpus
>
> }

~Pratyush

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

On 01/06/2017 at 06:11 PM, Pratyush Anand wrote:
> Hi Xunlei,
>
> Thanks a lot for making the things better and automating wherever it is possible :-)
>
> On Friday 06 January 2017 12:07 PM, Xunlei Pang wrote:
> > Check the number of cpus for x86_64 kdump kernel to boot with.
> > We met an issue for x86_64: kdump runs out of vectors with the
> > default "nr_cpus=1", when requesting tons of irqs.
> >
> > This patch detects such situation and warns users about the risk.
> > Signed-off-by: Xunlei Pang
> >
> > v1->v2:
> > When detecting risky cpu vectors, we just warn users instead of
> > modifying "nr_cpus=X" forcely.
> > Improved code comments.
> > Replaced nr_old with nr_origin, and improved some logic.
> >
> > kdumpctl | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index b2068cc..b6fc1f9 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -105,6 +105,85 @@ append_cmdline()
> >      echo $cmdline
> >  }
> >
> > +# Check the number of cpus for kdump kernel to boot with.
> > +# We met an issue for x86_64: kdump runs out of vectors with
> > +# "nr_cpus=1" when requesting tons of irqs, so here we check
> > +# "nr_cpus=X" and warn users if kdump probably can't work.
> > +check_kdump_cpus()
> > +{
> > local nr nr_search nr_origin nr_min nr_max
> > local arch=$(uname -m) cmdline=$KDUMP_COMMANDLINE
> > # Special treatment for x86_64 only currently.
> > if [ $arch != "x86_64" ]; then
> >    return
> > fi
> > # We only care about "nr_cpus=X" format for x86.
> > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | wc -l)
>
> may be we can just have
> nr_cpus=$(echo $cmdline | grep -o "nr_cpus=[0-9]*")
> > if [ $nr_search -eq 0 ] ; then
>
> and the  can check for [[ -z $nr_cpus ]]
> >    # Do not need to process if no valid "nr_cpus=X" specified.
> >    return
> > fi
> > # Get value X of "nr_cpus=X"
> > nr_search=$(echo $cmdline | grep -o "nr_cpus=[0-9]*" | cut -d "=" -f2 | grep "[0-9]" | sort)
>
> and same nr_cpus can be reused here then.

I've improved the logic, please see v3.
> > # In case there are multiple "nr_cpus=X", get the mininum value.
> > for nr in $nr_search; do
> >    if [ $nr -gt 0 ]; then
> >        nr_origin=$nr
> >        break
> >    fi
> > done
> > if [ -z "$nr_origin" ]; then
> >    echo "Warning: Wrong \"nr_cpus=\" kernel cmdline detected"
> >    return
> > fi
> > # Online cpus in first kernel.
> > nr_max=$(nproc)
> > # Calculate estimated minium cpus required by irqs(vectors).
> > nr_min=$(ls /proc/irq/ -l | grep ^d | wc -l)
> > # We roughly use 256-32(see kernel FIRST_EXTERNAL_VECTOR)=224 as
> > # maximum supported vectors can be allocated to io devices percpu.
> > # As nr_min is a ballpart figure, also some high-numbered vectors
> > # are consumed by the kernel(see FIRST_SYSTEM_VECTOR), we need a
> > # variance for safety.
> > #
> > # We got a large machine with 240 cpus, 6TB memory, 8 iommus, and
> > # 12 io-apics, 132 irqs under /proc/irq/, it can boot successfully
> > # with "nr_cpus=1". (256-32-132)=92, so choosing 64 as the variance
>
> why to guess that. If I have not missed anything then it seems that number of vectors needed by kernel is fixed.
> From arch/x86/include/asm/irq_vectors.h :
>
> vector 128 seems fixed for system call.
> If we have CONFIG_X86_LOCAL_APIC, then vector 0xef to 0xff are used by kernel. So, this variance should have fixed value as 1 for !CONFIG_X86_LOCAL_APIC and as 18 otherwise, no? So, may be we can take as 18 for all cases.

Hmm, I pondered quite a lot here, because it's hard to decide one exact value we should rely on :-)

Yes, FIRST_EXTERNAL_VECTOR(32) is defined by the x86 architecture, and it is unlikely to change.
While FIRST_SYSTEM_VECTOR is assigned by kernel, it may vary for different kernel versions. e.g.
the latest kernel version and linux-3.10 have different FIRST_SYSTEM_VECTOR values, there could
be more system vectors added in the kernel in the future.

Also there are other rare kernel internal reserved vectors, e.g. 0x80 is reserved for system call vector.

Additionally, there may be cases that one irq has explicit affinity, then multiple vectors(one for each cpu)
are allocated. So I just give a flexible variance, we can't precisely know if it can boot or not without knowing
all the details, but if it is above the threshold we selected, kdump has a high possibility to boot fail.

For example, if we choose threshold 256-32-18-1=205 and a system with 204 external device interrupts,
among them there are several(say 6) has 0x3 affinity explicitly set, then it will consume 6 more vectors in
total that is 210, so the actual calculated nr_min should be 2 instead of 1.

But if you think we played a little too careful with variance 64, I guess 32 should be good enough, after all
there is one more cpu added for multiple affinity cases:
    if [ $nr_min -gt 1 ]; then
        nr_min=$(($nr_min + 1))
        nr_min=$(($nr_min + $nr_min % 2))
    fi

Regards,
Xunlei
> > # seems ok. Then we get the max external irqs supported per cpu:
> > # (256-32-64)=160 as the dividend.
> > nr_min=$(($nr_min + 160 - 1))
> > nr_min=$(($nr_min / 160))
> > if [ $nr_min -gt 1 ]; then
> >    # The system seems to have tons of interrupts. while interrupts
> >    # with multiple-cpu affinity can consume multiple vectors, with
> >    # one vector for each cpu within the affinity mask. Fortunately
> >    # for x2apic which is widely used on large modern machines, in
> >    # default case of boot, device bringup etc will use a single cpu
> >    # for the interrupt affinity to minimize vector pressure.
> >    #
> >    # For further safety, we add one more cpu and round it up to an
> >    # even number which is commonly-used.
> >    nr_min=$(($nr_min + 1))
> >    nr_min=$(($nr_min + $nr_min % 2))
> > fi
> > if [ $nr_min -gt $nr_max ]; then
> >    nr_min=$nr_max
> > fi
> > if [ $nr_origin -ge $nr_min ]; then
> >    return
> > fi
> > echo "Warning: CPU vectors under pressure with "nr_cpus=$nr_origin", please try "nr_cpus=$nr_min" or more"
> >
> > +}
> >
> > # This function performs a series of edits on the command line.
> >  # Store the final result in global $KDUMP_COMMANDLINE.
> >  prepare_cmdline()
> > @@ -134,6 +213,8 @@ prepare_cmdline()
> >      fi
> >  KDUMP_COMMANDLINE=$cmdline
> > check_kdump_cpus
> >
> > }
>
> ~Pratyush

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Pratyush Anand]

On Friday 06 January 2017 05:18 PM, Xunlei Pang wrote:
> > why to guess that. If I have not missed anything then it seems that number of vectors needed by kernel is fixed.
> > From arch/x86/include/asm/irq_vectors.h :
> >
> > vector 128 seems fixed for system call.
> > If we have CONFIG_X86_LOCAL_APIC, then vector 0xef to 0xff are used by kernel. So, this variance should have fixed value as 1 for !CONFIG_X86_LOCAL_APIC and as 18 otherwise, no? So, may be we can take as 18 for all cases.
>
> Hmm, I pondered quite a lot here, because it's hard to decide one exact value we should rely on :-)
>
> Yes, FIRST_EXTERNAL_VECTOR(32) is defined by the x86 architecture, and it is unlikely to change.
> While FIRST_SYSTEM_VECTOR is assigned by kernel, it may vary for different kernel versions. e.g.
> the latest kernel version and linux-3.10 have different FIRST_SYSTEM_VECTOR values, there could
> be more system vectors added in the kernel in the future.
>
> Also there are other rare kernel internal reserved vectors, e.g. 0x80 is reserved for system call vector.
>
> Additionally, there may be cases that one irq has explicit affinity, then multiple vectors(one for each cpu)
> are allocated. So I just give a flexible variance, we can't precisely know if it can boot or not without knowing
> all the details, but if it is above the threshold we selected, kdump has a high possibility to boot fail.
>
> For example, if we choose threshold 256-32-18-1=205 and a system with 204 external device interrupts,
> among them there are several(say 6) has 0x3 affinity explicitly set, then it will consume 6 more vectors in
> total that is 210, so the actual calculated nr_min should be 2 instead of 1.

I am not sure, how does explicit affinity work. Does not an affinity 
0x03 means that interrupt can be routed to *either* cpu0 or cpu1? Now 
suppose all the irq line of cpu0 has been occupied and we request a new 
interrupt with affinity 0x03, would not that be routed to cpu2? If that 
can be routed then probably we do not need to take this factor in 
account while calculating minimum number of CPUs.
> But if you think we played a little too careful with variance 64, I guess 32 should be good enough, after all
> there is one more cpu added for multiple affinity cases:
>     if [ $nr_min -gt 1 ]; then
>         nr_min=$(($nr_min + 1))
>         nr_min=$(($nr_min + $nr_min % 2))

~Pratyush

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

On 01/06/2017 at 08:57 PM, Pratyush Anand wrote:
> On Friday 06 January 2017 05:18 PM, Xunlei Pang wrote:
> > > why to guess that. If I have not missed anything then it seems that number of vectors needed by kernel is fixed.
> > > From arch/x86/include/asm/irq_vectors.h :
> > >
> > > vector 128 seems fixed for system call.
> > > If we have CONFIG_X86_LOCAL_APIC, then vector 0xef to 0xff are used by kernel. So, this variance should have fixed value as 1 for !CONFIG_X86_LOCAL_APIC and as 18 otherwise, no? So, may be we can take as 18 for all cases.
> >
> > Hmm, I pondered quite a lot here, because it's hard to decide one exact value we should rely on :-)
> >
> > Yes, FIRST_EXTERNAL_VECTOR(32) is defined by the x86 architecture, and it is unlikely to change.
> > While FIRST_SYSTEM_VECTOR is assigned by kernel, it may vary for different kernel versions. e.g.
> > the latest kernel version and linux-3.10 have different FIRST_SYSTEM_VECTOR values, there could
> > be more system vectors added in the kernel in the future.
> >
> > Also there are other rare kernel internal reserved vectors, e.g. 0x80 is reserved for system call vector.
> >
> > Additionally, there may be cases that one irq has explicit affinity, then multiple vectors(one for each cpu)
> > are allocated. So I just give a flexible variance, we can't precisely know if it can boot or not without knowing
> > all the details, but if it is above the threshold we selected, kdump has a high possibility to boot fail.
> >
> > For example, if we choose threshold 256-32-18-1=205 and a system with 204 external device interrupts,
> > among them there are several(say 6) has 0x3 affinity explicitly set, then it will consume 6 more vectors in
> > total that is 210, so the actual calculated nr_min should be 2 instead of 1.
>
> I am not sure, how does explicit affinity work. Does not an affinity 0x03 means that interrupt can be routed to *either* cpu0 or cpu1? Now suppose all the irq

Yes, 0x3 means routing to either cpu0 or cpu1.
> line of cpu0 has been occupied and we request a new interrupt with affinity 0x03, would not that be routed to cpu2? If that can be routed then probably we do not need to take this factor in account while calculating minimum number of CPUs.

The vector is allocated when requesting irq or setting the irq affinity, if it finds cpu0 runs out of vectors,
the call path(request_irq, set_affinity) will return some error code.

Regards,
Xunlei
> > But if you think we played a little too careful with variance 64, I guess 32 should be good enough, after all
> > there is one more cpu added for multiple affinity cases:
> >     if [ $nr_min -gt 1 ]; then
> >         nr_min=$(($nr_min + 1))
> >         nr_min=$(($nr_min + $nr_min % 2))
>
> ~Pratyush

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Pratyush Anand]

Hi Xunlei,

On Friday 06 January 2017 06:41 PM, Xunlei Pang wrote:
> > I am not sure, how does explicit affinity work. Does not an affinity 0x03 means that interrupt can be routed to *either* cpu0 or cpu1? Now suppose all the irq
>
> Yes, 0x3 means routing to either cpu0 or cpu1.
> > line of cpu0 has been occupied and we request a new interrupt with affinity 0x03, would not that be routed to cpu2? If that can be routed then probably we do not need to take this factor in account while calculating minimum number of CPUs.
>
> The vector is allocated when requesting irq or setting the irq affinity, if it finds cpu0 runs out of vectors,
> the call path(request_irq, set_affinity) will return some error code.

I do not have much issue with any value for keeping the variance, its 
better to be "safe minimal" than keeping the "best minimal".

However, I would still like to understand this concept. What I 
understand is that requesting a vector and setting affinity are two 
mutually exclusive action.
Requesting a vector is binding a physical/virtual irq line with the 
driver (irq handler). set_affinity() will program interrupt controller 
to raise that irq to all possible CPU when hardware generates that 
interrupt.
Moreover, see kernel/irq/manage.c:__setup_irq(). It does not check 
return value of set_affinity(). So, __setup_irq() should pass even when 
set_affinity() fails, and in that case irq will be raised to the cpu as 
per hardware default affinity settings, no?

~Pratyush

[RESEND PATCH v2 2/2] kdumpctl: sanity check of nr_cpus for x86_64 in case running out of vectors

[Xunlei Pang]

On 01/09/2017 at 02:50 PM, Pratyush Anand wrote:
> Hi Xunlei,
>
> On Friday 06 January 2017 06:41 PM, Xunlei Pang wrote:
> > > I am not sure, how does explicit affinity work. Does not an affinity 0x03 means that interrupt can be routed to *either* cpu0 or cpu1? Now suppose all the irq
> >
> > Yes, 0x3 means routing to either cpu0 or cpu1.
> > > line of cpu0 has been occupied and we request a new interrupt with affinity 0x03, would not that be routed to cpu2? If that can be routed then probably we do not need to take this factor in account while calculating minimum number of CPUs.
> >
> > The vector is allocated when requesting irq or setting the irq affinity, if it finds cpu0 runs out of vectors,
> > the call path(request_irq, set_affinity) will return some error code.
>
> I do not have much issue with any value for keeping the variance, its better to be "safe minimal" than keeping the "best minimal".
>
> However, I would still like to understand this concept. What I understand is that requesting a vector and setting affinity are two mutually exclusive action.
> Requesting a vector is binding a physical/virtual irq line with the driver (irq handler). set_affinity() will program interrupt controller to raise that irq to all possible CPU when hardware generates that interrupt.
> Moreover, see kernel/irq/manage.c:__setup_irq(). It does not check return value of set_affinity(). So, __setup_irq() should pass even when set_affinity() fails, and in that case irq will be raised to the cpu as per hardware default affinity settings, no?

To be exact, when drivers allocate linux irq(with a cpumask), it will associate the x86 vectors, it's the place where can fail.
As an example, dmar_set_interrupt() --> dmar_alloc_hwirq() --> parent irq domain alloc -> x86_vector_domain_ops::x86_vector_alloc_irqs()
                                                    |
                                                    --> request_irq()

The dmar_alloc_hwirq() could fail when facing x86 vector pressure

Regards,
Xunlei

[Dave Young]

Hi Xunlei
On 01/06/17 at 02:37pm, Xunlei Pang wrote:
> Since KDUMP_COMMANDLINE is a global variable, prepare_cmdline can
> modify it directly instead of echoing back the result. This change
> enables it to output messages.
>
> Changed some coding styles.
> Signed-off-by: Xunlei Pang
>
> v1->v2:
> Changed some comments.
>
> kdumpctl | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index cb6f004..b2068cc 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -105,10 +105,12 @@ append_cmdline()
>    echo $cmdline
>  }
>
> -# This function performs a series of edits on the command line
> +# This function performs a series of edits on the command line.
> +# Store the final result in global $KDUMP_COMMANDLINE.
>  prepare_cmdline()
>  {
> local cmdline;
> local cmdline id
> if [ -z "$KDUMP_COMMANDLINE" ]; then
>   cmdline=`cat /proc/cmdline`
>   # 'root' parameter will cause kdump failure in live images
>
> @@ -118,6 +120,7 @@ prepare_cmdline()
>    else
>    	cmdline=${KDUMP_COMMANDLINE}
>    fi
> # These params should always be removed
>  cmdline=`remove_cmdline_param "$cmdline" crashkernel panic_on_warn`
>  # These params can be removed configurably
>
> @@ -125,12 +128,12 @@ prepare_cmdline()
>
> cmdline="${cmdline} ${KDUMP_COMMANDLINE_APPEND}"
> local id=`get_bootcpu_initial_apicid`
> id=`get_bootcpu_initial_apicid`
> if [ ! -z ${id} ] ; then
> cmdline=`append_cmdline "${cmdline}" disable_cpu_apicid ${id}`
> fi
> echo $cmdline
> KDUMP_COMMANDLINE=$cmdline
>
> }
>
> @@ -642,7 +645,7 @@ load_kdump()
>    	fi
>    fi
> KDUMP_COMMANDLINE=`prepare_cmdline`
>
> prepare_cmdline
>
> # For secureboot enabled machines, use new kexec file based syscall.
> # Old syscall will always fail as it does not have capability to

Looks good to me.

Acked-by: Dave Young

Thanks
Dave