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

[PATCH v4 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 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        |  8 +++++++
 2 files changed, 30 insertions(+)

-- 
2.26.2

[PATCH v4 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>
---
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 v4 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 prlimit
command requires specification of a real value for the number of files,
so use 262144 as the default value. As an aid for the admin configuring
the value add a comment specifying the common needs of xenstored for
the different domain types.

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)
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 13 +++++++++++++
 tools/hotplug/Linux/launch-xenstore.in             |  2 ++
 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 b83101ab7e..ad020b7a50 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: integer
+## Default: 262144
+#
+# 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 default of
+# 262144 allows for about 8 open files for the theoretical maximum of 32752
+# domains.
+# Only evaluated if XENSTORETYPE is "daemon".
+#XENSTORED_MAX_OPEN_FDS=262144
+
 ## Type: string
 ## Default: ""
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 1747c96065..2bc41bb4f0 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=262144
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
 	[ -x "$XENSTORED" ] || {
 		echo "No xenstored found"
@@ -70,6 +71,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

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

[Andrew Cooper]

On 27/09/2021 11:48, Juergen Gross wrote:
> 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 prlimit
> command requires specification of a real value for the number of files,
> so use 262144 as the default value.

Citation needed.

prlimit -nunlimited

prlimit --nofile=unlimited

both work fine, and strace confirms they issue correct system calls.

Support for "unlimited" as a parameter has existed for the entire
lifetime of the utility,
https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84

~Andrew

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

[Juergen Gross]

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

On 27.09.21 16:13, Andrew Cooper wrote:
> On 27/09/2021 11:48, Juergen Gross wrote:
>> 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 prlimit
>> command requires specification of a real value for the number of files,
>> so use 262144 as the default value.
> 
> Citation needed.
> 
> prlimit -nunlimited
> 
> prlimit --nofile=unlimited
> 
> both work fine, and strace confirms they issue correct system calls.

Not on my test system:

# prlimit --pid 734 --nofile=unlimited
prlimit: failed to set the NOFILE resource limit: Operation not permitted
# prlimit --pid 734 --nofile=262144
#

> Support for "unlimited" as a parameter has existed for the entire
> lifetime of the utility,
> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84

Yes, but not all systems seem to support raising the limit to
"unlimited".


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 v4 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Andrew Cooper]

On 27/09/2021 15:24, Juergen Gross wrote:
> On 27.09.21 16:13, Andrew Cooper wrote:
>> On 27/09/2021 11:48, Juergen Gross wrote:
>>> 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 prlimit
>>> command requires specification of a real value for the number of files,
>>> so use 262144 as the default value.
>>
>> Citation needed.
>>
>> prlimit -nunlimited
>>
>> prlimit --nofile=unlimited
>>
>> both work fine, and strace confirms they issue correct system calls.
>
> Not on my test system:
>
> # prlimit --pid 734 --nofile=unlimited
> prlimit: failed to set the NOFILE resource limit: Operation not permitted
> # prlimit --pid 734 --nofile=262144
> #

What does strace say in both of these cases?

>
>> Support for "unlimited" as a parameter has existed for the entire
>> lifetime of the utility,
>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>
>
> Yes, but not all systems seem to support raising the limit to
> "unlimited".

That's as maybe, but

prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)

is a Linux issue, not a prlimit bug.

~Andrew

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

[Ian Jackson]

Juergen Gross writes ("[PATCH v4 1/2] tools/xenstore: set oom score for xenstore daemon on Linux"):
> 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.

Reviewed-by: Ian Jackson <iwj@xenproject.org>

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

[Juergen Gross]

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

On 27.09.21 16:40, Andrew Cooper wrote:
> On 27/09/2021 15:24, Juergen Gross wrote:
>> On 27.09.21 16:13, Andrew Cooper wrote:
>>> On 27/09/2021 11:48, Juergen Gross wrote:
>>>> 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 prlimit
>>>> command requires specification of a real value for the number of files,
>>>> so use 262144 as the default value.
>>>
>>> Citation needed.
>>>
>>> prlimit -nunlimited
>>>
>>> prlimit --nofile=unlimited
>>>
>>> both work fine, and strace confirms they issue correct system calls.
>>
>> Not on my test system:
>>
>> # prlimit --pid 734 --nofile=unlimited
>> prlimit: failed to set the NOFILE resource limit: Operation not permitted
>> # prlimit --pid 734 --nofile=262144
>> #
> 
> What does strace say in both of these cases?

prlimit64(734, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY, 
rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
write(2, "prlimit: ", 9prlimit: )                = 9

vs.

prlimit64(734, RLIMIT_NOFILE, {rlim_cur=256*1024, rlim_max=256*1024}, 
NULL) = 0

> 
>>
>>> Support for "unlimited" as a parameter has existed for the entire
>>> lifetime of the utility,
>>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>>
>>
>> Yes, but not all systems seem to support raising the limit to
>> "unlimited".
> 
> That's as maybe, but
> 
> prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
> 
> is a Linux issue, not a prlimit bug.

Nevertheless it isn't a good idea to use this setting in case it isn't
supported in all Linux distros/versions we care about.


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 v4 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Andrew Cooper]

On 27/09/2021 15:52, Juergen Gross wrote:
> On 27.09.21 16:40, Andrew Cooper wrote:
>> On 27/09/2021 15:24, Juergen Gross wrote:
>>> On 27.09.21 16:13, Andrew Cooper wrote:
>>>> On 27/09/2021 11:48, Juergen Gross wrote:
>>>>> 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
>>>>> prlimit
>>>>> command requires specification of a real value for the number of
>>>>> files,
>>>>> so use 262144 as the default value.
>>>>
>>>> Citation needed.
>>>>
>>>> prlimit -nunlimited
>>>>
>>>> prlimit --nofile=unlimited
>>>>
>>>> both work fine, and strace confirms they issue correct system calls.
>>>
>>> Not on my test system:
>>>
>>> # prlimit --pid 734 --nofile=unlimited
>>> prlimit: failed to set the NOFILE resource limit: Operation not
>>> permitted
>>> # prlimit --pid 734 --nofile=262144
>>> #
>>
>> What does strace say in both of these cases?
>
> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
> write(2, "prlimit: ", 9prlimit: )                = 9
>
> vs.
>
> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=256*1024, rlim_max=256*1024},
> NULL) = 0
>
>>
>>>
>>>> Support for "unlimited" as a parameter has existed for the entire
>>>> lifetime of the utility,
>>>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>>>
>>>>
>>>
>>> Yes, but not all systems seem to support raising the limit to
>>> "unlimited".
>>
>> That's as maybe, but
>>
>> prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
>> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
>>
>> is a Linux issue, not a prlimit bug.
>
> Nevertheless it isn't a good idea to use this setting in case it isn't
> supported in all Linux distros/versions we care about.

Ok, but at a minimum you need s/prlimit/Linux/ in the commit message.

~Andrew

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

[Juergen Gross]

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

On 27.09.21 16:54, Andrew Cooper wrote:
> On 27/09/2021 15:52, Juergen Gross wrote:
>> On 27.09.21 16:40, Andrew Cooper wrote:
>>> On 27/09/2021 15:24, Juergen Gross wrote:
>>>> On 27.09.21 16:13, Andrew Cooper wrote:
>>>>> On 27/09/2021 11:48, Juergen Gross wrote:
>>>>>> 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
>>>>>> prlimit
>>>>>> command requires specification of a real value for the number of
>>>>>> files,
>>>>>> so use 262144 as the default value.
>>>>>
>>>>> Citation needed.
>>>>>
>>>>> prlimit -nunlimited
>>>>>
>>>>> prlimit --nofile=unlimited
>>>>>
>>>>> both work fine, and strace confirms they issue correct system calls.
>>>>
>>>> Not on my test system:
>>>>
>>>> # prlimit --pid 734 --nofile=unlimited
>>>> prlimit: failed to set the NOFILE resource limit: Operation not
>>>> permitted
>>>> # prlimit --pid 734 --nofile=262144
>>>> #
>>>
>>> What does strace say in both of these cases?
>>
>> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
>> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
>> write(2, "prlimit: ", 9prlimit: )                = 9
>>
>> vs.
>>
>> prlimit64(734, RLIMIT_NOFILE, {rlim_cur=256*1024, rlim_max=256*1024},
>> NULL) = 0
>>
>>>
>>>>
>>>>> Support for "unlimited" as a parameter has existed for the entire
>>>>> lifetime of the utility,
>>>>> https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
>>>>>
>>>>>
>>>>
>>>> Yes, but not all systems seem to support raising the limit to
>>>> "unlimited".
>>>
>>> That's as maybe, but
>>>
>>> prlimit64(0, RLIMIT_NOFILE, {rlim_cur=RLIM64_INFINITY,
>>> rlim_max=RLIM64_INFINITY}, NULL) = -1 EPERM (Operation not permitted)
>>>
>>> is a Linux issue, not a prlimit bug.
>>
>> Nevertheless it isn't a good idea to use this setting in case it isn't
>> supported in all Linux distros/versions we care about.
> 
> Ok, but at a minimum you need s/prlimit/Linux/ in the commit message.

Okay.

Should I send out another version, or can this be done when committing?


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 v4 2/2] tools/xenstore: set open file descriptor limit for xenstored

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> On 27.09.21 16:13, Andrew Cooper wrote:
> > both work fine, and strace confirms they issue correct system calls.
> 
> Not on my test system:
> 
> # prlimit --pid 734 --nofile=unlimited
> prlimit: failed to set the NOFILE resource limit: Operation not permitted
> # prlimit --pid 734 --nofile=262144
> #
> 
> > Support for "unlimited" as a parameter has existed for the entire
> > lifetime of the utility,
> > https://github.com/karelzak/util-linux/commit/6bac2825af7216c5471148e219dbcf62ec5ede84
> 
> Yes, but not all systems seem to support raising the limit to
> "unlimited".

This is strange.  Are you running in some kind of restricted
environment ?  <strike>Or maybe prlimit is trying to adjust the soft
limit (only) but failing to remove the hard limit too ?  I confess
that I never use prlimit, just ulimit.</strike>

I just tried this on my laptop:

root(ian)@zealot:~> ulimit -Hn 1048577
bash: ulimit: open files: cannot modify limit: Operation not permitted
root(ian)@zealot:~> ulimit -Hn 1048576
root(ian)@zealot:~>

????

1048576 is 0x100000.
1048577 is 0x100001.

The intertubes caused me to check sysctl fs.file-max (1591013),
/etc/security/limits.conf (nothing uncommented).  Eventually a helpful
person pointed me to /proc/sys/fs/nr_open.

root(ian)@zealot:~> cat /proc/sys/fs/nr_open
1048576

This is completely deranged.  The internet is full of people working
around this (if you are unluckloy you try to set nofile unlimited in
some file in /etc/security and then you can't log in!).

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

^ that explains why things are like this.  Oh woe is us what madness
have we collectively perpetrated.

I suggest the following workaround for our scripts: try to read
/proc/sys/fs/nr_open and use the value from there if there is one;
otherwise use "unlimited".

I think that is better than sort-of-guessing 262144.  What you do you
think ?

Thanks,
Ian.

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

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH v4 2/2] tools/xenstore: set open file descriptor limit for xenstored"):
> Should I send out another version, or can this be done when committing?

In any case I think you should send another version.

If you don't agree with my suggestion to check /proc/sys/fs/nr_open,
then you could usefully explain in the commit message why we're not
doing that.  If you do agree please include the links from my mail.

Ian.