diff mbox series

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

Message ID 20210608055839.10313-2-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series tools/xenstore: set resource limits of xenstored | expand

Commit Message

Juergen Gross June 8, 2021, 5:58 a.m. UTC
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(+)

Comments

Julien Grall July 8, 2021, 5:40 p.m. UTC | #1
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,
Juergen Gross July 9, 2021, 12:34 p.m. UTC | #2
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
Julien Grall July 12, 2021, 8:38 a.m. UTC | #3
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,
diff mbox series

Patch

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
 }