[PATCH v5 0/2] tools/xenstore: set resource limits of xenstored

[PATCH v5 0/2] tools/xenstore: set resource limits of xenstored

[Juergen Gross]

Set some limits for xenstored in order to avoid it being killed by
OOM killer, or to run out of file descriptors.

Changes in V5:
- respect /proc/sys/fs/nr_open (Ian Jackson)

Changes in V4:
- add comments
- switch to configure open file descriptors directly

Changes in V3:
- make oom score configurable

Changes in V2:
- split into 2 patches
- set limits from start script

Juergen Gross (2):
  tools/xenstore: set oom score for xenstore daemon on Linux
  tools/xenstore: set open file descriptor limit for xenstored

 .../Linux/init.d/sysconfig.xencommons.in      | 22 ++++++++++++++++
 tools/hotplug/Linux/launch-xenstore.in        | 26 +++++++++++++++++++
 2 files changed, 48 insertions(+)

-- 
2.26.2

[PATCH v5 1/2] tools/xenstore: set oom score for xenstore daemon on Linux

[Juergen Gross]

Xenstored is absolutely mandatory for a Xen host and it can't be
restarted, so being killed by OOM-killer in case of memory shortage is
to be avoided.

Set /proc/$pid/oom_score_adj (if available) per default to -500 (this
translates to 50% of dom0 memory size) in order to allow xenstored to
use large amounts of memory without being killed.

The percentage of dom0 memory above which the oom killer is allowed to
kill xenstored can be set via XENSTORED_OOM_MEM_THRESHOLD in
xencommons.

Make sure the pid file isn't a left-over from a previous run delete it
before starting xenstored.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
---
V2:
- set oom score from launch script (Julien Grall)
- split off open file descriptor limit setting (Julien Grall)
V3:
- make oom killer threshold configurable (Julien Grall)
V4:
- extend comment (Ian Jackson)
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 9 +++++++++
 tools/hotplug/Linux/launch-xenstore.in             | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index 00cf7f91d4..b83101ab7e 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -48,6 +48,15 @@ XENSTORED_ARGS=
 # Only evaluated if XENSTORETYPE is "daemon".
 #XENSTORED_TRACE=[yes|on|1]
 
+## Type: integer
+## Default: 50
+#
+# Percentage of dom0 memory size the xenstore daemon can use before the
+# OOM killer is allowed to kill it.
+# The specified value is multiplied by -10 and echoed to
+# /proc/PID/oom_score_adj.
+#XENSTORED_OOM_MEM_THRESHOLD=50
+
 ## Type: string
 ## Default: @LIBEXEC@/boot/xenstore-stubdom.gz
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 019f9d6f4d..1747c96065 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -59,11 +59,17 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 		echo "No xenstored found"
 		exit 1
 	}
+	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
+	XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))
+
+	rm -f @XEN_RUN_DIR@/xenstored.pid
 
 	echo -n Starting $XENSTORED...
 	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
 
 	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
+	XS_PID=`cat @XEN_RUN_DIR@/xenstored.pid`
+	echo $XS_OOM_SCORE >/proc/$XS_PID/oom_score_adj
 
 	exit 0
 }
-- 
2.26.2

[PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Juergen Gross]

Add a configuration item for the maximum number of open file
descriptors xenstored should be allowed to have.

The default should be "unlimited" in order not to restrict xenstored
in the number of domains it can support, but unfortunately the kernel
is normally limiting the maximum value via /proc/sys/fs/nr_open [1],
[2]. So check that file to exist and if it does, limit the maximum
value to the one specified by /proc/sys/fs/nr_open.

As an aid for the admin configuring the value add a comment specifying
the common needs of xenstored for the different domain types.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=60fd760fb9ff7034360bab7137c917c0330628c2
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c2d64fb6cae9aae480f6a46cfe79f8d7d48b59f

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- set ulimit form launch script (Julien Grall)
- split off from original patch (Julien Grall)
V4:
- switch to directly configuring the limit of file descriptors instead
  of domains (Ian Jackson)
V5:
- use /proc/sys/fs/nr_open (Ian Jackson)
---
 .../Linux/init.d/sysconfig.xencommons.in      | 13 ++++++++++++
 tools/hotplug/Linux/launch-xenstore.in        | 20 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index b83101ab7e..433e4849af 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -32,6 +32,19 @@
 # Changing this requires a reboot to take effect.
 #XENSTORED=@XENSTORED@
 
+## Type: string
+## Default: unlimited
+#
+# Select maximum number of file descriptors xenstored is allowed to have
+# opened at one time.
+# For each HVM domain xenstored might need up to 5 open file descriptors,
+# PVH and PV domains will require up to 3 open file descriptors. Additionally
+# 20-30 file descriptors will be opened for internal uses.
+# The specified value (including "unlimited") will be capped by the contents
+# of /proc/sys/fs/nr_open if existing.
+# Only evaluated if XENSTORETYPE is "daemon".
+#XENSTORED_MAX_OPEN_FDS=unlimited
+
 ## Type: string
 ## Default: ""
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 1747c96065..7a0334d880 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -54,6 +54,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 
 [ "$XENSTORETYPE" = "daemon" ] && {
 	[ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log"
+	[ -z "$XENSTORED_MAX_OPEN_FDS" ] && XENSTORED_MAX_OPEN_FDS=unlimited
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
 	[ -x "$XENSTORED" ] || {
 		echo "No xenstored found"
@@ -62,6 +63,24 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	[ -z "$XENSTORED_OOM_MEM_THRESHOLD" ] || XENSTORED_OOM_MEM_THRESHOLD=50
 	XS_OOM_SCORE=-$(($XENSTORED_OOM_MEM_THRESHOLD * 10))
 
+	[ "$XENSTORED_MAX_OPEN_FDS" = "unlimited" ] || {
+		[ -z "${XENSTORED_MAX_OPEN_FDS//[0-9]}" ] &&
+		[ -n "$XENSTORED_MAX_OPEN_FDS" ] || {
+			echo "XENSTORED_MAX_OPEN_FDS=$XENSTORED_MAX_OPEN_FDS invalid"
+			echo "Setting to default \"unlimited\"."
+			XENSTORED_MAX_OPEN_FDS=unlimited
+		}
+	}
+	[ -r /proc/sys/fs/nr_open ] && {
+		MAX_FDS=`cat /proc/sys/fs/nr_open`
+		[ "$XENSTORED_MAX_OPEN_FDS" = "unlimited" ] && XENSTORED_MAX_OPEN_FDS=$MAX_FDS
+		[ $XENSTORED_MAX_OPEN_FDS -gt $MAX_FDS ] && {
+			echo "XENSTORED_MAX_OPEN_FDS exceeds system limit."
+			echo "Setting to \"$MAX_FDS\"."
+			XENSTORED_MAX_OPEN_FDS=$MAX_FDS
+		}
+	}
+
 	rm -f @XEN_RUN_DIR@/xenstored.pid
 
 	echo -n Starting $XENSTORED...
@@ -70,6 +89,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
 	XS_PID=`cat @XEN_RUN_DIR@/xenstored.pid`
 	echo $XS_OOM_SCORE >/proc/$XS_PID/oom_score_adj
+	prlimit --pid $XS_PID --nofile=$XENSTORED_MAX_OPEN_FDS
 
 	exit 0
 }
-- 
2.26.2

[PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Ian Jackson]

Juergen Gross writes ("[PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> Add a configuration item for the maximum number of open file
> descriptors xenstored should be allowed to have.
> 
> The default should be "unlimited" in order not to restrict xenstored
> in the number of domains it can support, but unfortunately the kernel
> is normally limiting the maximum value via /proc/sys/fs/nr_open [1],
> [2]. So check that file to exist and if it does, limit the maximum
> value to the one specified by /proc/sys/fs/nr_open.
> 
> As an aid for the admin configuring the value add a comment specifying
> the common needs of xenstored for the different domain types.
...
>  	echo -n Starting $XENSTORED...
> @@ -70,6 +89,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>  	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
>  	XS_PID=`cat @XEN_RUN_DIR@/xenstored.pid`
>  	echo $XS_OOM_SCORE >/proc/$XS_PID/oom_score_adj
> +	prlimit --pid $XS_PID --nofile=$XENSTORED_MAX_OPEN_FDS

Thanks for this.  I have one comment/question, which I regret making
rather late:

I am uncomfortable with the use of prlimit here, because identifying
processes by pid is typically inherently not 100% reliable.

AIUI you are using it here because perhaps otherwise you would have to
mess about with both systemd and non-systemd approaches.  But in fact
this script "launch-xenstore" is simply a parent of xenstore.  It is
run either by systemd or from the init script, and it runs $XENSTORED
directly (so not via systemd or another process supervisor).

fd limits are inherited, so I think you can use ulimit rather than
prlimit ?

If you use ulimit I think you must set the hard and soft limits,
which requires two calls.

If you can't use ulimit then we should try to make some argument that
the prlimit can't target the wrong process eg due to a
misconfiguration or stale pid file or soemthing.  I think I see a way
that such an argument could be construted but it would be better just
to use ulimit.

Ian.

Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2395 bytes --]

On 28.09.21 14:02, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
>> Add a configuration item for the maximum number of open file
>> descriptors xenstored should be allowed to have.
>>
>> The default should be "unlimited" in order not to restrict xenstored
>> in the number of domains it can support, but unfortunately the kernel
>> is normally limiting the maximum value via /proc/sys/fs/nr_open [1],
>> [2]. So check that file to exist and if it does, limit the maximum
>> value to the one specified by /proc/sys/fs/nr_open.
>>
>> As an aid for the admin configuring the value add a comment specifying
>> the common needs of xenstored for the different domain types.
> ...
>>   	echo -n Starting $XENSTORED...
>> @@ -70,6 +89,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>>   	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
>>   	XS_PID=`cat @XEN_RUN_DIR@/xenstored.pid`
>>   	echo $XS_OOM_SCORE >/proc/$XS_PID/oom_score_adj
>> +	prlimit --pid $XS_PID --nofile=$XENSTORED_MAX_OPEN_FDS
> 
> Thanks for this.  I have one comment/question, which I regret making
> rather late:
> 
> I am uncomfortable with the use of prlimit here, because identifying
> processes by pid is typically inherently not 100% reliable.
> 
> AIUI you are using it here because perhaps otherwise you would have to
> mess about with both systemd and non-systemd approaches.  But in fact
> this script "launch-xenstore" is simply a parent of xenstore.  It is
> run either by systemd or from the init script, and it runs $XENSTORED
> directly (so not via systemd or another process supervisor).
> 
> fd limits are inherited, so I think you can use ulimit rather than
> prlimit ?
> 
> If you use ulimit I think you must set the hard and soft limits,
> which requires two calls.
> 
> If you can't use ulimit then we should try to make some argument that
> the prlimit can't target the wrong process eg due to a
> misconfiguration or stale pid file or soemthing.  I think I see a way
> that such an argument could be construted but it would be better just
> to use ulimit.

Hmm, maybe I should just use:

prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
    $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> Hmm, maybe I should just use:
> 
> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
>     $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS

I guess that would probably work (although it involves another
exec) but I don't understand what's wrong with ulimit, which is a
shell builtin.

I think this script has to run only on Linux and all reasonable Linux
/bin/sh have `ulimit`.  (I have checked dash and bash.)

So I think just

  ulimit -n $XENSTORED_MAX_OPEN_FDS

  $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS

will DTRT.  You could also do this

  ulimit -n $XENSTORED_MAX_OPEN_FDS || true

which will arrange that if, somehow, this fails, the system is likely
to continue to mostly-work despite the error.  Whether that would be
desirable is a matter of taste I think.

(I have RTFM again, and setting -H and -S separately is not needed;
omitting -H or -S means to set both.)

Ian.

Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 721 bytes --]

On 28.09.21 17:26, Ian Jackson wrote:
> Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
>> Hmm, maybe I should just use:
>>
>> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
>>      $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> 
> I guess that would probably work (although it involves another
> exec) but I don't understand what's wrong with ulimit, which is a
> shell builtin.

My main concern with ulimit is that this would set the open file limit
for _all_ children of the script. I don't think right now this is a real
problem, but it feels wrong to me (systemd-notify ought to be fine, but
you never know ...).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> On 28.09.21 17:26, Ian Jackson wrote:
> > Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> >> Hmm, maybe I should just use:
> >>
> >> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
> >>      $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> > 
> > I guess that would probably work (although it involves another
> > exec) but I don't understand what's wrong with ulimit, which is a
> > shell builtin.
> 
> My main concern with ulimit is that this would set the open file limit
> for _all_ children of the script. I don't think right now this is a real
> problem, but it feels wrong to me (systemd-notify ought to be fine, but
> you never know ...).

Oh, I see.  Yes, that is a good point.

So, I think your suggest (quoted above) is good.

Thanks,
Ian.