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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx>
> > > ---
> > > 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