On Sat 17-01-15 10:21:47, Johannes Weiner wrote:
> The "none" name for the low-boundary 0 and the high-boundary maximum
> value can be confusing.
>
> Just leave the low boundary at 0, and give the highest-possible
> boundary value the name "max" that means the same for controls.
max might be confusing as well because it matches with the knob name.
max_resource or max_memory sounds better to me.
Btw. I would separate page_counter_memparse change out and
replace the original 'mm: page_counter: pull "-1" handling out of
page_counter_memparse()' by it.
> Reported-by: Michal Hocko <mho...@suse.cz>
> Signed-off-by: Johannes Weiner <han...@cmpxchg.org>
> ---
> Documentation/cgroups/unified-hierarchy.txt | 5 +--
> include/linux/page_counter.h | 3 +-
> mm/memcontrol.c | 58
> ++++++++++-------------------
> mm/page_counter.c | 9 ++++-
> 4 files changed, 31 insertions(+), 44 deletions(-)
>
> diff --git a/Documentation/cgroups/unified-hierarchy.txt
> b/Documentation/cgroups/unified-hierarchy.txt
> index 643af9bb9a07..171aa23c113e 100644
> --- a/Documentation/cgroups/unified-hierarchy.txt
> +++ b/Documentation/cgroups/unified-hierarchy.txt
> @@ -404,9 +404,8 @@ supported and the interface files "release_agent" and
> be understood as an underflow into the highest possible value, -2 or
> -10M etc. do not work, so it's not consistent.
>
> - memory.low and memory.high will indicate "none" if the boundary is
> - not configured, and a configured boundary can be unset by writing
> - "none" into these files as well.
> + memory.low, memory.high, and memory.max will use the string "max" to
> + indicate and configure the highest possible value.
>
> 5. Planned Changes
>
> diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
> index 955421575d16..17fa4f8de3a6 100644
> --- a/include/linux/page_counter.h
> +++ b/include/linux/page_counter.h
> @@ -41,7 +41,8 @@ int page_counter_try_charge(struct page_counter *counter,
> struct page_counter **fail);
> void page_counter_uncharge(struct page_counter *counter, unsigned long
> nr_pages);
> int page_counter_limit(struct page_counter *counter, unsigned long limit);
> -int page_counter_memparse(const char *buf, unsigned long *nr_pages);
> +int page_counter_memparse(const char *buf, const char *max,
> + unsigned long *nr_pages);
>
> static inline void page_counter_reset_watermark(struct page_counter *counter)
> {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7adccee9fecb..718bc6bb5837 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3419,13 +3419,9 @@ static ssize_t mem_cgroup_write(struct
> kernfs_open_file *of,
> int ret;
>
> buf = strstrip(buf);
> - if (!strcmp(buf, "-1")) {
> - nr_pages = PAGE_COUNTER_MAX;
> - } else {
> - ret = page_counter_memparse(buf, &nr_pages);
> - if (ret)
> - return ret;
> - }
> + ret = page_counter_memparse(buf, "-1", &nr_pages);
> + if (ret)
> + return ret;
>
> switch (MEMFILE_ATTR(of_cft(of)->private)) {
> case RES_LIMIT:
> @@ -3795,13 +3791,9 @@ static int __mem_cgroup_usage_register_event(struct
> mem_cgroup *memcg,
> unsigned long usage;
> int i, size, ret;
>
> - if (!strcmp(args, "-1")) {
> - threshold = PAGE_COUNTER_MAX;
> - } else {
> - ret = page_counter_memparse(args, &threshold);
> - if (ret)
> - return ret;
> - }
> + ret = page_counter_memparse(args, "-1", &threshold);
> + if (ret)
> + return ret;
>
> mutex_lock(&memcg->thresholds_lock);
>
> @@ -5246,8 +5238,8 @@ static int memory_low_show(struct seq_file *m, void *v)
> struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(m));
> unsigned long low = ACCESS_ONCE(memcg->low);
>
> - if (low == 0)
> - seq_puts(m, "none\n");
> + if (low == PAGE_COUNTER_MAX)
> + seq_puts(m, "max\n");
> else
> seq_printf(m, "%llu\n", (u64)low * PAGE_SIZE);
>
> @@ -5262,13 +5254,9 @@ static ssize_t memory_low_write(struct
> kernfs_open_file *of,
> int err;
>
> buf = strstrip(buf);
> - if (!strcmp(buf, "none")) {
> - low = 0;
> - } else {
> - err = page_counter_memparse(buf, &low);
> - if (err)
> - return err;
> - }
> + err = page_counter_memparse(buf, "max", &low);
> + if (err)
> + return err;
>
> memcg->low = low;
>
> @@ -5281,7 +5269,7 @@ static int memory_high_show(struct seq_file *m, void *v)
> unsigned long high = ACCESS_ONCE(memcg->high);
>
> if (high == PAGE_COUNTER_MAX)
> - seq_puts(m, "none\n");
> + seq_puts(m, "max\n");
> else
> seq_printf(m, "%llu\n", (u64)high * PAGE_SIZE);
>
> @@ -5296,13 +5284,9 @@ static ssize_t memory_high_write(struct
> kernfs_open_file *of,
> int err;
>
> buf = strstrip(buf);
> - if (!strcmp(buf, "none")) {
> - high = PAGE_COUNTER_MAX;
> - } else {
> - err = page_counter_memparse(buf, &high);
> - if (err)
> - return err;
> - }
> + err = page_counter_memparse(buf, "max", &high);
> + if (err)
> + return err;
>
> memcg->high = high;
>
> @@ -5315,7 +5299,7 @@ static int memory_max_show(struct seq_file *m, void *v)
> unsigned long max = ACCESS_ONCE(memcg->memory.limit);
>
> if (max == PAGE_COUNTER_MAX)
> - seq_puts(m, "none\n");
> + seq_puts(m, "max\n");
> else
> seq_printf(m, "%llu\n", (u64)max * PAGE_SIZE);
>
> @@ -5330,13 +5314,9 @@ static ssize_t memory_max_write(struct
> kernfs_open_file *of,
> int err;
>
> buf = strstrip(buf);
> - if (!strcmp(buf, "none")) {
> - max = PAGE_COUNTER_MAX;
> - } else {
> - err = page_counter_memparse(buf, &max);
> - if (err)
> - return err;
> - }
> + err = page_counter_memparse(buf, "max", &max);
> + if (err)
> + return err;
>
> err = mem_cgroup_resize_limit(memcg, max);
> if (err)
> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index 0d4f9daf68bd..11b4beda14ba 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c
> @@ -166,16 +166,23 @@ int page_counter_limit(struct page_counter *counter,
> unsigned long limit)
> /**
> * page_counter_memparse - memparse() for page counter limits
> * @buf: string to parse
> + * @max: string meaning maximum possible value
> * @nr_pages: returns the result in number of pages
> *
> * Returns -EINVAL, or 0 and @nr_pages on success. @nr_pages will be
> * limited to %PAGE_COUNTER_MAX.
> */
> -int page_counter_memparse(const char *buf, unsigned long *nr_pages)
> +int page_counter_memparse(const char *buf, const char *max,
> + unsigned long *nr_pages)
> {
> char *end;
> u64 bytes;
>
> + if (!strcmp(buf, max)) {
> + *nr_pages = PAGE_COUNTER_MAX;
> + return 0;
> + }
> +
> bytes = memparse(buf, &end);
> if (*end != '\0')
> return -EINVAL;
> --
> 2.2.0
>
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
