Pkg xen devel - Sep 2020 - [PATCH] debian/scripts: Optimize scripts

Fewer fork()s and execve()s quickly add up to significant savings.  I'm
concerned Debian is slowly headed towards recreating SunOS^WSolaris
5.7^W2.7^W7 and the layers and layers of scripts which killed
performance.

As these runtime scripts are heavily used, avoid all uses of external
programs by them.

Signed-off-by: Elliott Mitchell <ehem+debian at m5p.com>
---
The gains from this should be rather larger than from going over the
build scripts.  Since `xen-utils-wrapper` is used to wrap pretty well
all of the Xen userspace programs this could be surprisingly large.

General approach is to be more optimistic about things working
successfully and only handling errors closer to the end.  This also
switches to `read` which is a shell built-in from `cat` which is not.
This also brings retrieval of values from /sys into the script itself,
rather than calling an external helper.

I notice a computer of mine needed ~2800 processes to restore 5 VMs.
This is based on the delta from low-numbered xvd process to high-numbered
xvd process.  Perhaps this isn't a huge number, but I note prior to this
simply running `xl list` caused the forking of 4 processes, whereas with
this it now takes 1.

Yes, it is possible to do too much using scripts.
---
 debian/scripts/xen-utils-wrapper | 39 +++++++++++++++++++++++++++++---
 debian/scripts/xen-version       |  6 +++--
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/debian/scripts/xen-utils-wrapper b/debian/scripts/xen-utils-wrapper
index 4d27a62d33..451cdb59f3 100755
--- a/debian/scripts/xen-utils-wrapper
+++ b/debian/scripts/xen-utils-wrapper
@@ -1,6 +1,39 @@
 #!/bin/sh -e
 
-COMMAND="$(basename $0)"
-DIR=$(/usr/lib/xen-common/bin/xen-dir)
+# This portion is fast-path, avoid external programs
+
+COMMAND="${0##*/}"
+
+DIR=/sys/hypervisor/version
+[ -e $DIR/major ] && read major < $DIR/major
+[ -e $DIR/minor ] && read minor < $DIR/minor
+VERSION="$major.$minor"
+
+DIR="/usr/lib/xen-$VERSION/bin"
+
+exec "$DIR/$COMMAND" "$@" || fail=$?
+
+
+# Certainly didn't work, now report failures, slower-path
+
+error() {
+	err="$1"
+	shift
+	echo "ERROR: " "$@" >&2
+	exit $err
+}
+
+[ -e "/sys/hypervisor/type" ] || error 1 "Can't find
hypervisor information in sysfs!"
+
+read type < /sys/hypervisor/type
+if [ "$type" != xen ]; then
+	[ -n "$type" ] || error 1 "Can't read hypervisor type from
sysfs!"
+	error 1 "Hypervisor is not xen but '$type'!"
+fi
+
+if [ ! -d "/usr/lib/xen-$VERSION" ]; then
+	error 127 "Can't find version $VERSION of xen utils (maybe
xen-utils-$VERSION was already removed before rebooting out of Xen $VERSION),
bailing out!"
+fi
+
+error 127 "Failed to execute $COMMAND: return $fail"
 
-exec "$DIR/bin/$COMMAND" "$@"
diff --git a/debian/scripts/xen-version b/debian/scripts/xen-version
index 492070a43b..5c42ad7351 100755
--- a/debian/scripts/xen-version
+++ b/debian/scripts/xen-version
@@ -6,10 +6,12 @@ error() {
 }
 
 if [ -e "/sys/hypervisor/type" ]; then
-    type="$(cat /sys/hypervisor/type)"
+    read type < /sys/hypervisor/type
     if [ "$type" = xen ]; then
         DIR=/sys/hypervisor/version
-        VERSION="$(cat $DIR/major).$(cat $DIR/minor)"
+        read major < $DIR/major
+        read minor < $DIR/minor
+        VERSION="$major.$minor"
     elif [ -z "$type" ]; then
         error "Can't read hypervisor type from sysfs!"
     else
-- 
2.20.1





-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Sometimes it really helps to be able to guess what is going on behind the
scenes to produce the right result.  As such below is a v2 of this patch.
I provided my guesses for the numbers, this time I've got actual data.
The appropriate command is:

strace -f -o trace.out -e trace=%process /usr/sbin/xl list

The system calls of interest are clone(), which is the Linux system call
behind fork(); and execve() which is the one which actually executes a
program.  On modern flavors of Unix (like Linux) fork() has become *much*
cheaper, memory pages will only be copied as they are modified.  Despite
the lower cost, clone() is an *expensive* system call, allocating a new
process table entry and extensively modifying page tables.  execve()
isn't nearly as expensive, but does hint at complexity.

With the version presently in git, strace reported 6 clone() calls and
7 execve() calls.  With the attached version, strace reported 0 clone()
calls and 2 execve() calls.  This suggests a substantial reduction in
overhead by using the attached.

You may dislike not using -e for the first part, but that was reviewed
carefully to ensure any failures would be handled correctly.

The only potential difference is if a hypervisor using identical
versioning and files were present (presented
/sys/hypervisor/version/major and /sys/hypervisor/version/minor which
matched a version of Xen which was installed), one might get an error out
of the Xen utility instead of the script (ie `xl` could complain instead
of the script).  Sharing both major and minor versions for a Debian
release seems unlikely and would likely diverge quickly.  As such I
consider the gains to *heavily* outweigh this potential downside.

>From 7a5d43be30657fdb70c18050dc3d9e941a564e46 Mon Sep 17 00:00:00 2001
From: Elliott Mitchell <ehem+debian at m5p.com>
Date: Sun, 20 Sep 2020 21:28:53 -0700
Subject: [PATCH] debian/scripts: Optimize scripts

Fewer fork()s and execve()s quickly add up to significant savings.  I'm
concerned Debian is slowly headed towards recreating SunOS^WSolaris
5.7^W2.7^W7 and the layers and layers of scripts which killed
performance.

As these runtime scripts are heavily used, avoid all uses of external
programs by them.

Signed-off-by: Elliott Mitchell <ehem+debian at m5p.com>
---
 debian/scripts/xen-utils-wrapper | 43 +++++++++++++++++++++++++++++---
 debian/scripts/xen-version       |  6 +++--
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/debian/scripts/xen-utils-wrapper b/debian/scripts/xen-utils-wrapper
index 4d27a62d33..a38ccc25fb 100755
--- a/debian/scripts/xen-utils-wrapper
+++ b/debian/scripts/xen-utils-wrapper
@@ -1,6 +1,41 @@
-#!/bin/sh -e
+#!/bin/sh
 
-COMMAND="$(basename $0)"
-DIR=$(/usr/lib/xen-common/bin/xen-dir)
+# This portion is fast-path, avoid external programs, NOTE: NOT -e HERE!
+
+COMMAND="${0##*/}"
+
+DIR=/sys/hypervisor/version
+read major 2> /dev/null < $DIR/major
+read minor 2> /dev/null < $DIR/minor
+VERSION="$major.$minor"
+
+DIR="/usr/lib/xen-$VERSION/bin"
+
+exec "$DIR/$COMMAND" "$@"
+fail=$?
+
+
+# Certainly didn't work, now report failures, slower-path
+set -e
+
+error() {
+	err="$1"
+	shift
+	echo "ERROR: " "$@" >&2
+	exit $err
+}
+
+[ -e "/sys/hypervisor/type" ] || error 1 "Can't find
hypervisor information in sysfs!"
+
+read type < /sys/hypervisor/type
+if [ "$type" != xen ]; then
+	[ -n "$type" ] || error 1 "Can't read hypervisor type from
sysfs!"
+	error 1 "Hypervisor is not xen but '$type'!"
+fi
+
+if [ ! -d "/usr/lib/xen-$VERSION" ]; then
+	error 127 "Can't find version $VERSION of xen utils (maybe
xen-utils-$VERSION was already removed before rebooting out of Xen $VERSION),
bailing out!"
+fi
+
+error 127 "Failed to execute $COMMAND: return $fail"
 
-exec "$DIR/bin/$COMMAND" "$@"
diff --git a/debian/scripts/xen-version b/debian/scripts/xen-version
index 492070a43b..5c42ad7351 100755
--- a/debian/scripts/xen-version
+++ b/debian/scripts/xen-version
@@ -6,10 +6,12 @@ error() {
 }
 
 if [ -e "/sys/hypervisor/type" ]; then
-    type="$(cat /sys/hypervisor/type)"
+    read type < /sys/hypervisor/type
     if [ "$type" = xen ]; then
         DIR=/sys/hypervisor/version
-        VERSION="$(cat $DIR/major).$(cat $DIR/minor)"
+        read major < $DIR/major
+        read minor < $DIR/minor
+        VERSION="$major.$minor"
     elif [ -z "$type" ]; then
         error "Can't read hypervisor type from sysfs!"
     else
-- 
2.20.1



-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debian-scripts-Optimize-scripts.patch
Type: text/x-diff
Size: 2708 bytes
Desc: not available
URL:
<http://alioth-lists.debian.net/pipermail/pkg-xen-devel/attachments/20200922/adf18b6a/attachment-0001.patch>

Well.  I continued playing with `xen-utils-wrapper` to ensure things
work like they were supposed to.  Apparently the `exec` command does
*NOT* behave in the obvious fashion.  On failure, the shell exits instead
of acting like the command failed.  As such an invocation along the lines
of below is needed.  I'm pretty sure this does actually work to
distinctly reduce the overhead of the wrapper while retaining
functionality.


From: Elliott Mitchell <ehem+debian at m5p.com>
Date: Sun, 20 Sep 2020 21:28:53 -0700
Subject: [PATCH] debian/scripts: Optimize scripts

Fewer fork()s and execve()s quickly add up to significant savings.  I'm
concerned Debian is slowly headed towards recreating SunOS^WSolaris
5.7^W2.7^W7 and the layers and layers of scripts which killed
performance.

As these runtime scripts are heavily used, avoid all uses of external
programs by them.

Signed-off-by: Elliott Mitchell <ehem+debian at m5p.com>
---
 debian/scripts/xen-utils-wrapper | 42 +++++++++++++++++++++++++++++---
 debian/scripts/xen-version       |  6 +++--
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/debian/scripts/xen-utils-wrapper b/debian/scripts/xen-utils-wrapper
index 4d27a62d33..4750156d71 100755
--- a/debian/scripts/xen-utils-wrapper
+++ b/debian/scripts/xen-utils-wrapper
@@ -1,6 +1,40 @@
-#!/bin/sh -e
+#!/bin/sh
 
-COMMAND="$(basename $0)"
-DIR=$(/usr/lib/xen-common/bin/xen-dir)
+# This portion is fast-path, avoid external programs, NOTE: NOT -e HERE!
+
+COMMAND="${0##*/}"
+
+DIR=/sys/hypervisor/version
+read major 2> /dev/null < $DIR/major
+read minor 2> /dev/null < $DIR/minor
+VERSION="$major.$minor"
+
+DIR="/usr/lib/xen-$VERSION/bin"
+
+[ -x "$DIR/$COMMAND" ] && exec "$DIR/$COMMAND"
"$@"
+
+
+# Certainly didn't work, now report failures, slower-path
+set -e
+
+error() {
+	err="$1"
+	shift
+	echo "ERROR: " "$@" >&2
+	exit $err
+}
+
+[ -e "/sys/hypervisor/type" ] || error 1 "Can't find
hypervisor information in sysfs!"
+
+read type < /sys/hypervisor/type
+if [ "$type" != xen ]; then
+	[ -n "$type" ] || error 1 "Can't read hypervisor type from
sysfs!"
+	error 1 "Hypervisor is not xen but '$type'!"
+fi
+
+if [ ! -d "/usr/lib/xen-$VERSION" ]; then
+	error 127 "Can't find version $VERSION of xen utils (maybe
xen-utils-$VERSION was already removed before rebooting out of Xen $VERSION),
bailing out!"
+fi
+
+error 127 "Unable to execute $COMMAND"
 
-exec "$DIR/bin/$COMMAND" "$@"
diff --git a/debian/scripts/xen-version b/debian/scripts/xen-version
index 492070a43b..5c42ad7351 100755
--- a/debian/scripts/xen-version
+++ b/debian/scripts/xen-version
@@ -6,10 +6,12 @@ error() {
 }
 
 if [ -e "/sys/hypervisor/type" ]; then
-    type="$(cat /sys/hypervisor/type)"
+    read type < /sys/hypervisor/type
     if [ "$type" = xen ]; then
         DIR=/sys/hypervisor/version
-        VERSION="$(cat $DIR/major).$(cat $DIR/minor)"
+        read major < $DIR/major
+        read minor < $DIR/minor
+        VERSION="$major.$minor"
     elif [ -z "$type" ]; then
         error "Can't read hypervisor type from sysfs!"
     else
-- 
2.20.1




-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debian-scripts-Optimize-scripts.patch
Type: text/x-diff
Size: 2639 bytes
Desc: not available
URL:
<http://alioth-lists.debian.net/pipermail/pkg-xen-devel/attachments/20200923/b23bfe9e/attachment.patch>