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

[Julien Grall <julien@xen.org>]

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