These are patches that change the usage of *.limit_in_bytes files.
[1/2] memcg: add interface to reset limits
[2/2] memcg: allow mem.limit bigger than memsw.limit iff unlimited
They are based on mmotm-2009-06-02-16-11.
Any comments would be welcome.
Thanks,
Daisuke Nishimura.
Setting mem.limit or memsw.limit to 0 has no meaning
in actual use(no process can run in such condition).
We don't have interface to reset mem.limit or memsw.limit now,
so let's reset the mem.limit or memsw.limit to default(unlimited)
when they are being set to 0.
Signed-off-by: Daisuke Nishimura <[email protected]>
---
Documentation/cgroups/memory.txt | 1 +
include/linux/res_counter.h | 2 ++
kernel/res_counter.c | 2 +-
mm/memcontrol.c | 2 ++
4 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 1a60887..e1c69f3 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -204,6 +204,7 @@ We can alter the memory limit:
NOTE: We can use a suffix (k, K, m, M, g or G) to indicate values in kilo,
mega or gigabytes.
+NOTE: We can write "0" to reset the *.limit_in_bytes(unlimited).
# cat /cgroups/0/memory.limit_in_bytes
4194304
diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index 4c5bcf6..511f42f 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -49,6 +49,8 @@ struct res_counter {
struct res_counter *parent;
};
+#define RESOURCE_MAX (unsigned long long)LLONG_MAX
+
/**
* Helpers to interact with userspace
* res_counter_read_u64() - returns the value of the specified member.
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index bf8e753..0a45778 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -18,7 +18,7 @@
void res_counter_init(struct res_counter *counter, struct res_counter *parent)
{
spin_lock_init(&counter->lock);
- counter->limit = (unsigned long long)LLONG_MAX;
+ counter->limit = RESOURCE_MAX;
counter->parent = parent;
}
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a83e039..6629ed2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2040,6 +2040,8 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
ret = res_counter_memparse_write_strategy(buffer, &val);
if (ret)
break;
+ if (!val)
+ val = RESOURCE_MAX;
if (type == _MEM)
ret = mem_cgroup_resize_limit(memcg, val);
else
Now users cannot set mem.limit bigger than memsw.limit.
This patch allows mem.limit bigger than memsw.limit iff mem.limit==unlimited.
By this, users can set memsw.limit without setting mem.limit.
I think it's usefull if users want to limit memsw only.
They must set mem.limit first and memsw.limit to the same value now for this purpose.
They can save the first step by this patch.
Signed-off-by: Daisuke Nishimura <[email protected]>
---
mm/memcontrol.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6629ed2..2b63cb1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1742,11 +1742,12 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
/*
* Rather than hide all in some function, I do this in
* open coded manner. You see what this really does.
- * We have to guarantee mem->res.limit < mem->memsw.limit.
+ * We have to guarantee mem->res.limit < mem->memsw.limit,
+ * except for mem->res.limit == RESOURCE_MAX(unlimited) case.
*/
mutex_lock(&set_limit_mutex);
memswlimit = res_counter_read_u64(&memcg->memsw, RES_LIMIT);
- if (memswlimit < val) {
+ if (val != RESOURCE_MAX && memswlimit < val) {
ret = -EINVAL;
mutex_unlock(&set_limit_mutex);
break;
@@ -1789,11 +1790,12 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
/*
* Rather than hide all in some function, I do this in
* open coded manner. You see what this really does.
- * We have to guarantee mem->res.limit < mem->memsw.limit.
+ * We have to guarantee mem->res.limit < mem->memsw.limit,
+ * except for mem->res.limit == RESOURCE_MAX(unlimited) case.
*/
mutex_lock(&set_limit_mutex);
memlimit = res_counter_read_u64(&memcg->res, RES_LIMIT);
- if (memlimit > val) {
+ if (memlimit != RESOURCE_MAX && memlimit > val) {
ret = -EINVAL;
mutex_unlock(&set_limit_mutex);
break;
On Wed, 3 Jun 2009 11:50:27 +0900
Daisuke Nishimura <[email protected]> wrote:
> Now users cannot set mem.limit bigger than memsw.limit.
> This patch allows mem.limit bigger than memsw.limit iff mem.limit==unlimited.
>
> By this, users can set memsw.limit without setting mem.limit.
> I think it's usefull if users want to limit memsw only.
> They must set mem.limit first and memsw.limit to the same value now for this purpose.
> They can save the first step by this patch.
>
I don't like this. No benefits to users.
The user should know when they set memsw.limit they have to set memory.limit.
This just complicates things.
If you want to do this, add an interface as
memory.all.limit_in_bytes (or some better name)
and allow to set memory.limit and memory.memsw.limit _at once_.
But I'm not sure it's worth to try. Saving user's few steps by the kenerl patch ?
Thanks,
-Kame
> Signed-off-by: Daisuke Nishimura <[email protected]>
> ---
> mm/memcontrol.c | 10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6629ed2..2b63cb1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1742,11 +1742,12 @@ static int mem_cgroup_resize_limit(struct mem_cgroup *memcg,
> /*
> * Rather than hide all in some function, I do this in
> * open coded manner. You see what this really does.
> - * We have to guarantee mem->res.limit < mem->memsw.limit.
> + * We have to guarantee mem->res.limit < mem->memsw.limit,
> + * except for mem->res.limit == RESOURCE_MAX(unlimited) case.
> */
> mutex_lock(&set_limit_mutex);
> memswlimit = res_counter_read_u64(&memcg->memsw, RES_LIMIT);
> - if (memswlimit < val) {
> + if (val != RESOURCE_MAX && memswlimit < val) {
> ret = -EINVAL;
> mutex_unlock(&set_limit_mutex);
> break;
> @@ -1789,11 +1790,12 @@ static int mem_cgroup_resize_memsw_limit(struct mem_cgroup *memcg,
> /*
> * Rather than hide all in some function, I do this in
> * open coded manner. You see what this really does.
> - * We have to guarantee mem->res.limit < mem->memsw.limit.
> + * We have to guarantee mem->res.limit < mem->memsw.limit,
> + * except for mem->res.limit == RESOURCE_MAX(unlimited) case.
> */
> mutex_lock(&set_limit_mutex);
> memlimit = res_counter_read_u64(&memcg->res, RES_LIMIT);
> - if (memlimit > val) {
> + if (memlimit != RESOURCE_MAX && memlimit > val) {
> ret = -EINVAL;
> mutex_unlock(&set_limit_mutex);
> break;
>
On Wed, 3 Jun 2009 11:49:08 +0900
Daisuke Nishimura <[email protected]> wrote:
> Setting mem.limit or memsw.limit to 0 has no meaning
> in actual use(no process can run in such condition).
>
> We don't have interface to reset mem.limit or memsw.limit now,
> so let's reset the mem.limit or memsw.limit to default(unlimited)
> when they are being set to 0.
>
Maybe good. But when I proposed this kind of patch, it was rejected.
(try to add RES_ININITY)
please wait acks from others.
But from me,
Acked-by: KAMEZAWA Hiroyuki <[email protected]>
> Signed-off-by: Daisuke Nishimura <[email protected]>
> ---
> Documentation/cgroups/memory.txt | 1 +
> include/linux/res_counter.h | 2 ++
> kernel/res_counter.c | 2 +-
> mm/memcontrol.c | 2 ++
> 4 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> index 1a60887..e1c69f3 100644
> --- a/Documentation/cgroups/memory.txt
> +++ b/Documentation/cgroups/memory.txt
> @@ -204,6 +204,7 @@ We can alter the memory limit:
>
> NOTE: We can use a suffix (k, K, m, M, g or G) to indicate values in kilo,
> mega or gigabytes.
> +NOTE: We can write "0" to reset the *.limit_in_bytes(unlimited).
>
> # cat /cgroups/0/memory.limit_in_bytes
> 4194304
> diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> index 4c5bcf6..511f42f 100644
> --- a/include/linux/res_counter.h
> +++ b/include/linux/res_counter.h
> @@ -49,6 +49,8 @@ struct res_counter {
> struct res_counter *parent;
> };
>
> +#define RESOURCE_MAX (unsigned long long)LLONG_MAX
> +
> /**
> * Helpers to interact with userspace
> * res_counter_read_u64() - returns the value of the specified member.
> diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> index bf8e753..0a45778 100644
> --- a/kernel/res_counter.c
> +++ b/kernel/res_counter.c
> @@ -18,7 +18,7 @@
> void res_counter_init(struct res_counter *counter, struct res_counter *parent)
> {
> spin_lock_init(&counter->lock);
> - counter->limit = (unsigned long long)LLONG_MAX;
> + counter->limit = RESOURCE_MAX;
> counter->parent = parent;
> }
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a83e039..6629ed2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2040,6 +2040,8 @@ static int mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
> ret = res_counter_memparse_write_strategy(buffer, &val);
> if (ret)
> break;
> + if (!val)
> + val = RESOURCE_MAX;
> if (type == _MEM)
> ret = mem_cgroup_resize_limit(memcg, val);
> else
>
Daisuke Nishimura wrote:
> Setting mem.limit or memsw.limit to 0 has no meaning
> in actual use(no process can run in such condition).
>
I wrote a test program that set mem.limit to 0 to test
oom in memcg, and now it is in LTP, though I can modify
it accordingly.
> We don't have interface to reset mem.limit or memsw.limit now,
> so let's reset the mem.limit or memsw.limit to default(unlimited)
> when they are being set to 0.
>
The idea of having a way to set the limit to unlimited is good,
but how about allow this by writing -1 to mem.limit?
On Wed, 3 Jun 2009 12:52:28 +0900, KAMEZAWA Hiroyuki <[email protected]> wrote:
> On Wed, 3 Jun 2009 11:50:27 +0900
> Daisuke Nishimura <[email protected]> wrote:
>
> > Now users cannot set mem.limit bigger than memsw.limit.
> > This patch allows mem.limit bigger than memsw.limit iff mem.limit==unlimited.
> >
> > By this, users can set memsw.limit without setting mem.limit.
> > I think it's usefull if users want to limit memsw only.
> > They must set mem.limit first and memsw.limit to the same value now for this purpose.
> > They can save the first step by this patch.
> >
>
> I don't like this. No benefits to users.
> The user should know when they set memsw.limit they have to set memory.limit.
> This just complicates things.
>
Hmm, I think there is a user who cares only limitting logical memory(mem+swap),
not physical memory, and wants kswapd to reclaim physical memory when congested.
At least, I'm a such user.
Do you disagree even if I add a file like "memory.allow_limit_memsw_only" ?
Thanks,
Daisuke Nishimura.
> If you want to do this, add an interface as
> memory.all.limit_in_bytes (or some better name)
> and allow to set memory.limit and memory.memsw.limit _at once_.
>
> But I'm not sure it's worth to try. Saving user's few steps by the kenerl patch ?
>
> Thanks,
> -Kame
>
>
On Wed, 03 Jun 2009 13:16:27 +0800, Li Zefan <[email protected]> wrote:
> Daisuke Nishimura wrote:
> > Setting mem.limit or memsw.limit to 0 has no meaning
> > in actual use(no process can run in such condition).
> >
>
> I wrote a test program that set mem.limit to 0 to test
> oom in memcg, and now it is in LTP, though I can modify
> it accordingly.
>
Thank you for your information, there is an acutual user then.
> > We don't have interface to reset mem.limit or memsw.limit now,
> > so let's reset the mem.limit or memsw.limit to default(unlimited)
> > when they are being set to 0.
> >
>
> The idea of having a way to set the limit to unlimited is good,
> but how about allow this by writing -1 to mem.limit?
>
O.K.
I'll try it.
Thanks,
Daisuke Nishimura.
On Wed, 3 Jun 2009 14:01:02 +0900
Daisuke Nishimura <[email protected]> wrote:
> On Wed, 3 Jun 2009 12:52:28 +0900, KAMEZAWA Hiroyuki <[email protected]> wrote:
> > On Wed, 3 Jun 2009 11:50:27 +0900
> > Daisuke Nishimura <[email protected]> wrote:
> >
> > > Now users cannot set mem.limit bigger than memsw.limit.
> > > This patch allows mem.limit bigger than memsw.limit iff mem.limit==unlimited.
> > >
> > > By this, users can set memsw.limit without setting mem.limit.
> > > I think it's usefull if users want to limit memsw only.
> > > They must set mem.limit first and memsw.limit to the same value now for this purpose.
> > > They can save the first step by this patch.
> > >
> >
> > I don't like this. No benefits to users.
> > The user should know when they set memsw.limit they have to set memory.limit.
> > This just complicates things.
> >
> Hmm, I think there is a user who cares only limitting logical memory(mem+swap),
> not physical memory, and wants kswapd to reclaim physical memory when congested.
> At least, I'm a such user.
>
> Do you disagree even if I add a file like "memory.allow_limit_memsw_only" ?
>
We can it _now_.
Thanks,
-Kame
On Wed, 3 Jun 2009 14:01:02 +0900
Daisuke Nishimura <[email protected]> wrote:
> On Wed, 3 Jun 2009 12:52:28 +0900, KAMEZAWA Hiroyuki <[email protected]> wrote:
> > On Wed, 3 Jun 2009 11:50:27 +0900
> > Daisuke Nishimura <[email protected]> wrote:
> >
> > > Now users cannot set mem.limit bigger than memsw.limit.
> > > This patch allows mem.limit bigger than memsw.limit iff mem.limit==unlimited.
> > >
> > > By this, users can set memsw.limit without setting mem.limit.
> > > I think it's usefull if users want to limit memsw only.
> > > They must set mem.limit first and memsw.limit to the same value now for this purpose.
> > > They can save the first step by this patch.
> > >
> >
> > I don't like this. No benefits to users.
> > The user should know when they set memsw.limit they have to set memory.limit.
> > This just complicates things.
> >
> Hmm, I think there is a user who cares only limitting logical memory(mem+swap),
> not physical memory, and wants kswapd to reclaim physical memory when congested.
> At least, I'm a such user.
>
> Do you disagree even if I add a file like "memory.allow_limit_memsw_only" ?
>
How about removing memory.limit < memsw.limit condition completely ?
Thanks,
-Kame
>
> Thanks,
> Daisuke Nishimura.
>
> > If you want to do this, add an interface as
> > memory.all.limit_in_bytes (or some better name)
> > and allow to set memory.limit and memory.memsw.limit _at once_.
> >
> > But I'm not sure it's worth to try. Saving user's few steps by the kenerl patch ?
> >
> > Thanks,
> > -Kame
> >
> >
>
On Wed, 3 Jun 2009 17:46:41 +0900, KAMEZAWA Hiroyuki <[email protected]> wrote:
> On Wed, 3 Jun 2009 14:01:02 +0900
> Daisuke Nishimura <[email protected]> wrote:
>
> > On Wed, 3 Jun 2009 12:52:28 +0900, KAMEZAWA Hiroyuki <[email protected]> wrote:
> > > On Wed, 3 Jun 2009 11:50:27 +0900
> > > Daisuke Nishimura <[email protected]> wrote:
> > >
> > > > Now users cannot set mem.limit bigger than memsw.limit.
> > > > This patch allows mem.limit bigger than memsw.limit iff mem.limit==unlimited.
> > > >
> > > > By this, users can set memsw.limit without setting mem.limit.
> > > > I think it's usefull if users want to limit memsw only.
> > > > They must set mem.limit first and memsw.limit to the same value now for this purpose.
> > > > They can save the first step by this patch.
> > > >
> > >
> > > I don't like this. No benefits to users.
> > > The user should know when they set memsw.limit they have to set memory.limit.
> > > This just complicates things.
> > >
> > Hmm, I think there is a user who cares only limitting logical memory(mem+swap),
> > not physical memory, and wants kswapd to reclaim physical memory when congested.
> > At least, I'm a such user.
> >
> > Do you disagree even if I add a file like "memory.allow_limit_memsw_only" ?
> >
>
> How about removing memory.limit < memsw.limit condition completely ?
>
It might be good idea.
IMHO, there is no critical reason it must be checked by kernel, but I'm not sure.
All I wanted to do was "let users who cares only about memsw.limit
ignore mem.limit completely". That's why, I treated only the "unlimited"(not set
mem.limit) case as special.
But, as you say, there is no reason it must be implemented in kernel.
(We can use a middle-ware or something.)
I'll drop this and consider more.
Thanks,
Daisuke Nishimura.
> > > We don't have interface to reset mem.limit or memsw.limit now,
> > > so let's reset the mem.limit or memsw.limit to default(unlimited)
> > > when they are being set to 0.
> > >
> >
> > The idea of having a way to set the limit to unlimited is good,
> > but how about allow this by writing -1 to mem.limit?
> >
> O.K.
> I'll try it.
>
This is the updated version.
===
From: Daisuke Nishimura <[email protected]>
We don't have interface to reset mem.limit or memsw.limit now.
This patch allows to reset mem.limit or memsw.limit when they are
being set to -1.
Signed-off-by: Daisuke Nishimura <[email protected]>
---
Documentation/cgroups/memory.txt | 1 +
include/linux/res_counter.h | 2 ++
kernel/res_counter.c | 12 +++++++++++-
3 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index 1a60887..1631690 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -204,6 +204,7 @@ We can alter the memory limit:
NOTE: We can use a suffix (k, K, m, M, g or G) to indicate values in kilo,
mega or gigabytes.
+NOTE: We can write "-1" to reset the *.limit_in_bytes(unlimited).
# cat /cgroups/0/memory.limit_in_bytes
4194304
diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index 4c5bcf6..511f42f 100644
--- a/include/linux/res_counter.h
+++ b/include/linux/res_counter.h
@@ -49,6 +49,8 @@ struct res_counter {
struct res_counter *parent;
};
+#define RESOURCE_MAX (unsigned long long)LLONG_MAX
+
/**
* Helpers to interact with userspace
* res_counter_read_u64() - returns the value of the specified member.
diff --git a/kernel/res_counter.c b/kernel/res_counter.c
index bf8e753..e1338f0 100644
--- a/kernel/res_counter.c
+++ b/kernel/res_counter.c
@@ -18,7 +18,7 @@
void res_counter_init(struct res_counter *counter, struct res_counter *parent)
{
spin_lock_init(&counter->lock);
- counter->limit = (unsigned long long)LLONG_MAX;
+ counter->limit = RESOURCE_MAX;
counter->parent = parent;
}
@@ -133,6 +133,16 @@ int res_counter_memparse_write_strategy(const char *buf,
unsigned long long *res)
{
char *end;
+
+ /* return RESOURCE_MAX(unlimited) if "-1" is specified */
+ if (*buf == '-') {
+ *res = simple_strtoull(buf + 1, &end, 10);
+ if (*res != 1 || *end != '\0')
+ return -EINVAL;
+ *res = RESOURCE_MAX;
+ return 0;
+ }
+
/* FIXME - make memparse() take const char* args */
*res = memparse((char *)buf, &end);
if (*end != '\0')
On Fri, 5 Jun 2009 22:22:45 +0900
Daisuke Nishimura <[email protected]> wrote:
> We don't have interface to reset mem.limit or memsw.limit now.
>
> This patch allows to reset mem.limit or memsw.limit when they are
> being set to -1.
>
> ...
>
> @@ -133,6 +133,16 @@ int res_counter_memparse_write_strategy(const char *buf,
> unsigned long long *res)
> {
> char *end;
> +
> + /* return RESOURCE_MAX(unlimited) if "-1" is specified */
> + if (*buf == '-') {
> + *res = simple_strtoull(buf + 1, &end, 10);
> + if (*res != 1 || *end != '\0')
> + return -EINVAL;
> + *res = RESOURCE_MAX;
> + return 0;
> + }
The test for (*end != '\0') would be unneeded if strict_strtoull() had
been used.
> +
> /* FIXME - make memparse() take const char* args */
> *res = memparse((char *)buf, &end);
> if (*end != '\0')