[systemd-devel] [PATCH] core: add startup resource control option

[Lennart Poettering]

On Tue, 25.03.14 01:03, WaLyong Cho (walyong.cho at samsung.com) wrote:

>          /* Figure out which controllers we need */
>  
> -        if (c->cpu_accounting || c->cpu_shares != 1024)
> +        if (c->cpu_accounting ||
> +            c->startup_cpu_shares != 1024 ||
> +            (manager_state(m) != MANAGER_STARTING && c->cpu_shares !=
> 1024))

This looks incorrect. Shouldn't it be like this:

...

if (c->cpu_accounting ||
    (manager->state(m) == MANAGER_STARTING ? c->startup_cpu_shares : c->cpu_shares) != 1024)

...

>                  mask |= CGROUP_CPUACCT | CGROUP_CPU;
>  
>          if (c->blockio_accounting ||
> -            c->blockio_weight != 1000 ||
> +            c->startup_blockio_weight != 1000 ||
> +            (manager_state(m) != MANAGER_STARTING &&
> c->blockio_weight != 1000) ||

Similar here...

> +        c = unit_get_cgroup_context(u);
> +        if (manager_state(u->manager) == MANAGER_STARTING &&
> +            (c->startup_cpu_shares_set || c->startup_blockio_weight_set)) {
> +                r = set_put(u->manager->startup_units, u);
> +                if (r < 0) {
> +                        if (r == -EEXIST)
> +                                r = 0;
> +                        else
> +                                return r;
> +                }
> +        }


Hmm, this should not be done when we realize things I think, but when we
load the configuration, for example in unit_load(), next to where we add
the automatic deps for things...

> +                if (mode != UNIT_CHECK) {
> +                        c->startup_cpu_shares = ul;
> +                        unit_write_drop_in_private_format(u, mode,
> name, "StartupCPUShares=%lu", ul);

You need to set the boolean here, too...

> +                if (mode != UNIT_CHECK) {
> +                        c->startup_blockio_weight = ul;
> +                        unit_write_drop_in_private_format(u, mode, name, "StartupBlockIOWeight=%lu", ul);
> +                }

Same here....

>  
> +        while (!set_isempty(m->startup_units)) {
> +                u = set_steal_first(m->startup_units);
> +                cgroup_context_apply(m, unit_get_cgroup_context(u), unit_get_cgroup_mask(u), u->cgroup_path);
> +        }

I'd really leave the set around and hence iterate through it here...

Lennart

-- 
Lennart Poettering, Red Hat