[systemd-devel] RFC : PATCH: initial implementation of system wide rlimit

[Frederic Crozat]

Le lundi 26 mars 2012 à 18:51 +0200, Lennart Poettering a écrit :
> On Mon, 26.03.12 14:25, Frederic Crozat (fcrozat at suse.com) wrote:
> 
> Heya,
> 
> > - currently, the limits are set on both rlim_cur and rlim_max (due to
> > reuse of config_parse_limit code). I'd like to extend the
> > implementation to support separate values for rlim_max and rlim_cur.
> > But I'm not 100% sure how we should express that in configuration
> > file. Should we use a additional keyword (LimitCPU / LimitSoftCPU ) or
> > a dual value (LimitCPU=100;10) ?
> 
> I decided not to expose separate values for soft/non-soft on purpose. I
> have the suspicion that they only make sense for user sessions, not so
> much for system services,
> 
> I am mostly waiting for a usecase here. If somebody makes a good case
> for hwo they should be useful in system services we can add support for
> them, but otherwise I am not convinced.
> 
> We generally only expose kernel features we think are useful, but if
> they aren't they are better hidden away.

Looking at the defaults shipped with openSUSE, some limits are set to
"sane" soft value  (80% for soft virtual limit, 85% for soft resident
limit, etc..) and some default have different soft / hard value (fdlimit
= 8192 (hard), 1024 (soft) ; locklimit = 256KB (hard), 64KB (soft) ).
With the current implementation, we couldn't ensure such duality. 

I'll ask around if people have other use cases.

Another interesting thing I didn't port from SUSE is being able to
express some of those values as percentage of system memory and not as
absolute value.

> > +        for (i = 0; i < RLIMIT_NLIMITS; i++) {
> > +                if (!default_rlimit[i])
> > +                        if (!(default_rlimit[i]= new(struct rlimit, 1)))
> > +                                break;
> > +                getrlimit(i,default_rlimit[i]);
> > +        }
> 
> For new code we try to write
> 
> x = new(foobar, 1);
> if (!x) {
>         ...
> }
> 
> rather than:
> 
> if (!(x = new(foobar, 1))) {
>         ...
> }

Sorry, I'm not yet used to it ;)

> Also, you need to check for OOM here. And then, if it is clear that
> getrlimit() will always return 0 here, then we should make that clear by
> enclosing this in "assert_se(getrlimit(...) == 0)" or so.

I'll fix that.

> But in general: why this code anyway? We should just let the pointer
> point to NULL if there is not rlimit set. I.e. only those array entries
> with non-default values should actually point to something.

I wanted to show the correct system values (default, when not set in
systemd) when using systemctl show. Otherwise, they might look as
infinite value (I was getting that, but I might had a bug there).

> >  
> >          fn = arg_running_as == MANAGER_SYSTEM ? SYSTEM_CONFIG_FILE : USER_CONFIG_FILE;
> >          f = fopen(fn, "re");
> > @@ -1400,6 +1424,15 @@ int main(int argc, char *argv[]) {
> >          m->swap_auto = arg_swap_auto;
> >          m->default_std_output = arg_default_std_output;
> >          m->default_std_error = arg_default_std_error;
> > +        for (j = 0; j < RLIMIT_NLIMITS; j++) {
> > +                if (!m->rlimit[j])
> > +                        if (!(m->rlimit[j]= new(struct rlimit, 1)))
> > +                                break;
> > +
> > +                (m->rlimit[j])->rlim_cur = default_rlimit[j]->rlim_cur;
> > +                (m->rlimit[j])->rlim_max = default_rlimit[j]->rlim_max;
> > +        }
> 
> Need to handle OOM. memdup() might be nicer to use here. (In fact, might
> be worth introducing newdup() here as a type-safe macro, i.e. a
> combination of new() and memdup()).

I'll see what I can do.


-- 
Frederic Crozat <fcrozat at suse.com>
SUSE