From 9558e85fbee62701add31208e96dd105a820fb24 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Jun 2020 20:06:18 +0200 Subject: Revert "Fix memory protection default setting" --- src/core/dbus-cgroup.c | 164 +++++++++++------------------------- src/core/load-fragment.c | 22 +++-- src/shared/bus-unit-util.c | 17 +--- src/test/test-cgroup-unit-default.c | 2 +- 4 files changed, 61 insertions(+), 144 deletions(-) diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index 8587a712e2..080f6fb1ae 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -707,112 +707,14 @@ static int bus_cgroup_set_boolean( return 1; \ } -#define BUS_DEFINE_SET_CGROUP_PROTECTION(function, mask, scale) \ - static int bus_cgroup_set_##function( \ - Unit *u, \ - const char *name, \ - uint64_t *p, \ - bool *s, \ - sd_bus_message *message, \ - UnitWriteFlags flags, \ - sd_bus_error *error) { \ - \ - uint64_t v = CGROUP_LIMIT_MIN; \ - bool nonempty = true; \ - char type; \ - int r; \ - \ - assert(p); \ - assert(s); \ - \ - r = sd_bus_message_peek_type(message, &type, NULL); \ - if (r < 0) \ - return r; \ - if (type == SD_BUS_TYPE_BOOLEAN) { \ - r = sd_bus_message_read(message, "b", &nonempty); \ - if (r < 0) \ - return r; \ - /* Bool is used to denote empty value only */ \ - if (nonempty) \ - return -EINVAL; \ - } else if (type != SD_BUS_TYPE_UINT64) { \ - return -EINVAL; \ - } else { \ - r = sd_bus_message_read(message, "t", &v); \ - if (r < 0) \ - return r; \ - } \ - \ - if (!UNIT_WRITE_FLAGS_NOOP(flags)) { \ - *p = v; \ - unit_invalidate_cgroup(u, mask); \ - if (!nonempty) { \ - *s = false; \ - unit_write_settingf(u, flags, name, \ - "%s=", name); \ - } else if (v == CGROUP_LIMIT_MAX) { \ - *s = true; \ - unit_write_settingf(u, flags, name, \ - "%s=infinity", name); \ - } else { \ - *s = true; \ - unit_write_settingf(u, flags, name, \ - "%s=%" PRIu64, name, v); \ - } \ - } \ - \ - return 1; \ - } \ - static int bus_cgroup_set_##function##_scale( \ - Unit *u, \ - const char *name, \ - uint64_t *p, \ - bool *s, \ - sd_bus_message *message, \ - UnitWriteFlags flags, \ - sd_bus_error *error) { \ - \ - uint64_t v; \ - uint32_t raw; \ - int r; \ - \ - assert(p); \ - assert(s); \ - \ - r = sd_bus_message_read(message, "u", &raw); \ - if (r < 0) \ - return r; \ - \ - v = scale(raw, UINT32_MAX); \ - if (v >= UINT64_MAX) \ - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, \ - "Value specified in %s is out of range", name); \ - \ - if (!UNIT_WRITE_FLAGS_NOOP(flags)) { \ - *p = v; \ - unit_invalidate_cgroup(u, mask); \ - \ - /* Prepare to chop off suffix */ \ - assert_se(endswith(name, "Scale")); \ - \ - uint32_t scaled = DIV_ROUND_UP((uint64_t) raw * 1000, (uint64_t) UINT32_MAX); \ - unit_write_settingf(u, flags, name, "%.*s=%" PRIu32 ".%" PRIu32 "%%", \ - (int)(strlen(name) - strlen("Scale")), name, \ - scaled / 10, scaled % 10); \ - } \ - \ - *s = true; \ - return 1; \ - } - DISABLE_WARNING_TYPE_LIMITS; BUS_DEFINE_SET_CGROUP_WEIGHT(cpu_weight, CGROUP_MASK_CPU, CGROUP_WEIGHT_IS_OK, CGROUP_WEIGHT_INVALID); BUS_DEFINE_SET_CGROUP_WEIGHT(cpu_shares, CGROUP_MASK_CPU, CGROUP_CPU_SHARES_IS_OK, CGROUP_CPU_SHARES_INVALID); BUS_DEFINE_SET_CGROUP_WEIGHT(io_weight, CGROUP_MASK_IO, CGROUP_WEIGHT_IS_OK, CGROUP_WEIGHT_INVALID); BUS_DEFINE_SET_CGROUP_WEIGHT(blockio_weight, CGROUP_MASK_BLKIO, CGROUP_BLKIO_WEIGHT_IS_OK, CGROUP_BLKIO_WEIGHT_INVALID); BUS_DEFINE_SET_CGROUP_LIMIT(memory, CGROUP_MASK_MEMORY, physical_memory_scale, 1); +BUS_DEFINE_SET_CGROUP_LIMIT(memory_protection, CGROUP_MASK_MEMORY, physical_memory_scale, 0); BUS_DEFINE_SET_CGROUP_LIMIT(swap, CGROUP_MASK_MEMORY, physical_memory_scale, 0); -BUS_DEFINE_SET_CGROUP_PROTECTION(memory_protection, CGROUP_MASK_MEMORY, physical_memory_scale); REENABLE_WARNING; static int bus_cgroup_set_tasks_max( @@ -938,17 +840,33 @@ int bus_cgroup_set_property( if (streq(name, "MemoryAccounting")) return bus_cgroup_set_boolean(u, name, &c->memory_accounting, CGROUP_MASK_MEMORY, message, flags, error); - if (streq(name, "MemoryMin")) - return bus_cgroup_set_memory_protection(u, name, &c->memory_min, &c->memory_min_set, message, flags, error); + if (streq(name, "MemoryMin")) { + r = bus_cgroup_set_memory_protection(u, name, &c->memory_min, message, flags, error); + if (r > 0) + c->memory_min_set = true; + return r; + } - if (streq(name, "MemoryLow")) - return bus_cgroup_set_memory_protection(u, name, &c->memory_low, &c->memory_low_set, message, flags, error); + if (streq(name, "MemoryLow")) { + r = bus_cgroup_set_memory_protection(u, name, &c->memory_low, message, flags, error); + if (r > 0) + c->memory_low_set = true; + return r; + } - if (streq(name, "DefaultMemoryMin")) - return bus_cgroup_set_memory_protection(u, name, &c->default_memory_min, &c->default_memory_min_set, message, flags, error); + if (streq(name, "DefaultMemoryMin")) { + r = bus_cgroup_set_memory_protection(u, name, &c->default_memory_min, message, flags, error); + if (r > 0) + c->default_memory_min_set = true; + return r; + } - if (streq(name, "DefaultMemoryLow")) - return bus_cgroup_set_memory_protection(u, name, &c->default_memory_low, &c->default_memory_low_set, message, flags, error); + if (streq(name, "DefaultMemoryLow")) { + r = bus_cgroup_set_memory_protection(u, name, &c->default_memory_low, message, flags, error); + if (r > 0) + c->default_memory_low_set = true; + return r; + } if (streq(name, "MemoryHigh")) return bus_cgroup_set_memory(u, name, &c->memory_high, message, flags, error); @@ -962,17 +880,33 @@ int bus_cgroup_set_property( if (streq(name, "MemoryLimit")) return bus_cgroup_set_memory(u, name, &c->memory_limit, message, flags, error); - if (streq(name, "MemoryMinScale")) - return bus_cgroup_set_memory_protection_scale(u, name, &c->memory_min, &c->memory_min_set, message, flags, error); + if (streq(name, "MemoryMinScale")) { + r = bus_cgroup_set_memory_protection_scale(u, name, &c->memory_min, message, flags, error); + if (r > 0) + c->memory_min_set = true; + return r; + } - if (streq(name, "MemoryLowScale")) - return bus_cgroup_set_memory_protection_scale(u, name, &c->memory_low, &c->memory_low_set, message, flags, error); + if (streq(name, "MemoryLowScale")) { + r = bus_cgroup_set_memory_protection_scale(u, name, &c->memory_low, message, flags, error); + if (r > 0) + c->memory_low_set = true; + return r; + } - if (streq(name, "DefaultMemoryMinScale")) - return bus_cgroup_set_memory_protection_scale(u, name, &c->default_memory_min, &c->default_memory_min_set, message, flags, error); + if (streq(name, "DefaultMemoryMinScale")) { + r = bus_cgroup_set_memory_protection_scale(u, name, &c->default_memory_min, message, flags, error); + if (r > 0) + c->default_memory_min_set = true; + return r; + } - if (streq(name, "DefaultMemoryLowScale")) - return bus_cgroup_set_memory_protection_scale(u, name, &c->default_memory_low, &c->default_memory_low_set, message, flags, error); + if (streq(name, "DefaultMemoryLowScale")) { + r = bus_cgroup_set_memory_protection_scale(u, name, &c->default_memory_low, message, flags, error); + if (r > 0) + c->default_memory_low_set = true; + return r; + } if (streq(name, "MemoryHighScale")) return bus_cgroup_set_memory_scale(u, name, &c->memory_high, message, flags, error); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index c1a4eb96cb..a2eede4cce 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3372,12 +3372,6 @@ int config_parse_memory_limit( uint64_t bytes = CGROUP_LIMIT_MAX; int r; - if (STR_IN_SET(lvalue, "DefaultMemoryLow", - "DefaultMemoryMin", - "MemoryLow", - "MemoryMin")) - bytes = CGROUP_LIMIT_MIN; - if (!isempty(rvalue) && !streq(rvalue, "infinity")) { r = parse_permille(rvalue); @@ -3397,20 +3391,24 @@ int config_parse_memory_limit( } } - /* Keep Memory{Low,Min} unset with empty assignment so that we fall back to DefaultMemory* which in - * contrast means zeroing the property. */ if (streq(lvalue, "DefaultMemoryLow")) { - c->default_memory_low = bytes; c->default_memory_low_set = true; + if (isempty(rvalue)) + c->default_memory_low = CGROUP_LIMIT_MIN; + else + c->default_memory_low = bytes; } else if (streq(lvalue, "DefaultMemoryMin")) { - c->default_memory_min = bytes; c->default_memory_min_set = true; + if (isempty(rvalue)) + c->default_memory_min = CGROUP_LIMIT_MIN; + else + c->default_memory_min = bytes; } else if (streq(lvalue, "MemoryMin")) { c->memory_min = bytes; - c->memory_min_set = !isempty(rvalue); + c->memory_min_set = true; } else if (streq(lvalue, "MemoryLow")) { c->memory_low = bytes; - c->memory_low_set = !isempty(rvalue); + c->memory_low_set = true; } else if (streq(lvalue, "MemoryHigh")) c->memory_high = bytes; else if (streq(lvalue, "MemoryMax")) diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 4b1145a6ba..69f79ea176 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -489,22 +489,7 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons "MemoryLimit", "TasksMax")) { - if (streq(eq, "infinity")) { - r = sd_bus_message_append(m, "(sv)", field, "t", CGROUP_LIMIT_MAX); - if (r < 0) - return bus_log_create_error(r); - return 1; - } else if (isempty(eq) && STR_IN_SET(field, "DefaultMemoryLow", - "DefaultMemoryMin", - "MemoryLow", - "MemoryMin")) { - /* We can't use CGROUP_LIMIT_MIN nor CGROUP_LIMIT_MAX to convey the empty assignment - * so marshall specially as a boolean. */ - r = sd_bus_message_append(m, "(sv)", field, "b", 0); - if (r < 0) - return bus_log_create_error(r); - return 1; - } else if (isempty(eq)) { + if (isempty(eq) || streq(eq, "infinity")) { r = sd_bus_message_append(m, "(sv)", field, "t", CGROUP_LIMIT_MAX); if (r < 0) return bus_log_create_error(r); diff --git a/src/test/test-cgroup-unit-default.c b/src/test/test-cgroup-unit-default.c index f4843374ea..372667041c 100644 --- a/src/test/test-cgroup-unit-default.c +++ b/src/test/test-cgroup-unit-default.c @@ -72,7 +72,7 @@ static int test_default_memory_low(void) { * * 3. dml-discard.slice sets DefaultMemoryLow= with no rvalue. As such, * dml-discard-empty.service should end up with a value of 0. - * dml-discard-set-ml.service sets MemoryLow=15, and as such should have that override the + * dml-discard-explicit-ml.service sets MemoryLow=70, and as such should have that override the * reset DefaultMemoryLow value. dml-discard.slice should still have an eventual memory.low of 50. * * ┌───────────┐ -- cgit v1.2.1