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

[PATCH v2 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 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

 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 7 +++++++
 tools/hotplug/Linux/launch-xenstore.in             | 6 ++++++
 2 files changed, 13 insertions(+)

-- 
2.26.2

[PATCH v2 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) to -500 in order to allow
xenstored to use large amounts of memory without being killed.

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)
---
 tools/hotplug/Linux/launch-xenstore.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 019f9d6f4d..3ad71e3d08 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -59,11 +59,14 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 		echo "No xenstored found"
 		exit 1
 	}
+	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 -500 >/proc/$XS_PID/oom_score_adj
 
 	exit 0
 }
-- 
2.26.2

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

[Juergen Gross]

Add a configuration item for the maximum number of domains xenstored
should support and set the limit of open file descriptors accordingly.

For HVM domains there are up to 5 socket connections per domain (2 by
the xl daemon process, and 3 by qemu). So set the ulimit for xenstored
to 5 * XENSTORED_MAX_DOMAINS + 100 (the "+ 100" is for some headroom,
like logging, event channel device, etc.).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- set ulimit form launch script (Julien Grall)
- split off from original patch (Julien Grall)
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 7 +++++++
 tools/hotplug/Linux/launch-xenstore.in             | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index 00cf7f91d4..516cd97092 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -32,6 +32,13 @@
 # Changing this requires a reboot to take effect.
 #XENSTORED=@XENSTORED@
 
+## Type: integer
+## Default: 32768
+#
+# Select maximum number of domains supported by xenstored.
+# Only evaluated if XENSTORETYPE is "daemon".
+#XENSTORED_MAX_N_DOMAINS=32768
+
 ## Type: string
 ## Default: ""
 #
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 3ad71e3d08..89149f98ee 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -54,12 +54,14 @@ 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_N_DOMAINS" ] && XENSTORED_MAX_N_DOMAINS=32768
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
 	[ -x "$XENSTORED" ] || {
 		echo "No xenstored found"
 		exit 1
 	}
 	rm -f @XEN_RUN_DIR@/xenstored.pid
+	N_FILES=$(($XENSTORED_MAX_N_DOMAINS * 5 + 100))
 
 	echo -n Starting $XENSTORED...
 	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
@@ -67,6 +69,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 -500 >/proc/$XS_PID/oom_score_adj
+	prlimit --pid $XS_PID --nofile=$N_FILES
 
 	exit 0
 }
-- 
2.26.2

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

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

Am Tue,  8 Jun 2021 07:58:39 +0200
schrieb Juergen Gross <jgross@suse.com>:

> +#XENSTORED_MAX_N_DOMAINS=32768

This will break fillup.
Provide an empty variable like it is done for a few others in that file.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Juergen Gross]

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

On 08.06.21 18:39, Olaf Hering wrote:
> Am Tue,  8 Jun 2021 07:58:39 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> +#XENSTORED_MAX_N_DOMAINS=32768
> 
> This will break fillup.

Why? You realize that above is a comment just documenting the default?

> Provide an empty variable like it is done for a few others in that file.

I'm following the pattern of basically all variables in that file, BTW.


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

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

Am Fri, 11 Jun 2021 07:01:31 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Why? You realize that above is a comment just documenting the default?

That depends on the context. See https://bugzilla.opensuse.org/show_bug.cgi?id=1185682 for a reason why it should become an empty variable. But yes, we can patch that one too.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Juergen Gross]

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

On 11.06.21 07:46, Olaf Hering wrote:
> Am Fri, 11 Jun 2021 07:01:31 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> Why? You realize that above is a comment just documenting the default?
> 
> That depends on the context. See https://bugzilla.opensuse.org/show_bug.cgi?id=1185682 for a reason why it should become an empty variable. But yes, we can patch that one too.
Isn't that a bug in fillup or the related rpm-macro? A variable set in
the existing /etc/sysconfig/xencommons file only should be preserved.

In general I think we should be consistent in the file.

In case there is no downside for other distributions I'd recommend to
switch all variables to your suggested pattern.

If there are disadvantages for others we should keep the current
pattern as changing it now would break existing installations.

Any thoughts?


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

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

Am Fri, 11 Jun 2021 09:07:24 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Isn't that a bug in fillup or the related rpm-macro? 

No. Fillup expects a certain pattern: a bunch of comments and a single key=var right after that. With such format it might be able to adjust the comment and leave the key=var as it is. Without key=var it will see it as a stale comment, and removes the entire section of comments during the next package update.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Juergen Gross]

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

Ping?

On 08.06.21 07:58, Juergen Gross wrote:
> Set some limits for xenstored in order to avoid it being killed by
> OOM killer, or to run out of file descriptors.
> 
> 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
> 
>   tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 7 +++++++
>   tools/hotplug/Linux/launch-xenstore.in             | 6 ++++++
>   2 files changed, 13 insertions(+)
> 


[-- 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 v2 1/2] tools/xenstore: set oom score for xenstore daemon on Linux

[Julien Grall]

Hi Juergen,

On 08/06/2021 06:58, Juergen Gross wrote:
> 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) to -500 in order to allow
> xenstored to use large amounts of memory without being killed.
> 
> Make sure the pid file isn't a left-over from a previous run delete it
> before starting xenstored.

This sentence is a bit confusing to read. Do you mean "*To* make 
sure....*,* delete it before"?

> 
> 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)
> ---
>   tools/hotplug/Linux/launch-xenstore.in | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
> index 019f9d6f4d..3ad71e3d08 100644
> --- a/tools/hotplug/Linux/launch-xenstore.in
> +++ b/tools/hotplug/Linux/launch-xenstore.in
> @@ -59,11 +59,14 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>   		echo "No xenstored found"
>   		exit 1
>   	}
> +	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 -500 >/proc/$XS_PID/oom_score_adj

NIT: It would be worth considering to introduce a variable so this can 
be set from the configuration file.

With or without it:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Juergen Gross]

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

On 08.07.21 19:40, Julien Grall wrote:
> Hi Juergen,
> 
> On 08/06/2021 06:58, Juergen Gross wrote:
>> 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) to -500 in order to allow
>> xenstored to use large amounts of memory without being killed.
>>
>> Make sure the pid file isn't a left-over from a previous run delete it
>> before starting xenstored.
> 
> This sentence is a bit confusing to read. Do you mean "*To* make 
> sure....*,* delete it before"?

Yes, will change it.

> 
>>
>> 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)
>> ---
>>   tools/hotplug/Linux/launch-xenstore.in | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/hotplug/Linux/launch-xenstore.in 
>> b/tools/hotplug/Linux/launch-xenstore.in
>> index 019f9d6f4d..3ad71e3d08 100644
>> --- a/tools/hotplug/Linux/launch-xenstore.in
>> +++ b/tools/hotplug/Linux/launch-xenstore.in
>> @@ -59,11 +59,14 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons 
>> && . @CONFIG_DIR@/@CONFIG_LEAF
>>           echo "No xenstored found"
>>           exit 1
>>       }
>> +    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 -500 >/proc/$XS_PID/oom_score_adj
> 
> NIT: It would be worth considering to introduce a variable so this can 
> be set from the configuration file.

Do you have any scenario in mind where this would be beneficial?

I'm not against it, but I'm wondering why anybody would want that
to be configurable.

> 
> With or without it:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

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 v2 1/2] tools/xenstore: set oom score for xenstore daemon on Linux

[Julien Grall]

Hi Juergen,

On 09/07/2021 13:34, Juergen Gross wrote:
> On 08.07.21 19:40, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 08/06/2021 06:58, Juergen Gross wrote:
>>> 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) to -500 in order to allow
>>> xenstored to use large amounts of memory without being killed.
>>>
>>> Make sure the pid file isn't a left-over from a previous run delete it
>>> before starting xenstored.
>>
>> This sentence is a bit confusing to read. Do you mean "*To* make 
>> sure....*,* delete it before"?
> 
> Yes, will change it.
> 
>>
>>>
>>> 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)
>>> ---
>>>   tools/hotplug/Linux/launch-xenstore.in | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in 
>>> b/tools/hotplug/Linux/launch-xenstore.in
>>> index 019f9d6f4d..3ad71e3d08 100644
>>> --- a/tools/hotplug/Linux/launch-xenstore.in
>>> +++ b/tools/hotplug/Linux/launch-xenstore.in
>>> @@ -59,11 +59,14 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons 
>>> && . @CONFIG_DIR@/@CONFIG_LEAF
>>>           echo "No xenstored found"
>>>           exit 1
>>>       }
>>> +    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 -500 >/proc/$XS_PID/oom_score_adj
>>
>> NIT: It would be worth considering to introduce a variable so this can 
>> be set from the configuration file.
> 
> Do you have any scenario in mind where this would be beneficial?
> 
> I'm not against it, but I'm wondering why anybody would want that
> to be configurable.

So from the commit message (and browsing a bit), I don't understand how 
-500 would fit every case. IOW why not -600/-700...?

If it is a random value, then we should consider to allow the user to 
modify it easily. If the value has a specific meaning, then I think this 
ought to be written in the commit message and possibly launch-xenstore.in.

Cheers,

-- 
Julien Grall