[PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Ganesan Rajagopal]

We run a lot of automated tests when building our software and run into
OOM scenarios when the tests run unbounded. v1 memcg exports
memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
metric to heuristically limit the number of tests that can run in
parallel based on per test historical data.

This metric is currently not exported for v2 memcg and there is no
other easy way of getting this information. getrusage() syscall returns
"ru_maxrss" which can be used as an approximation but that's the max
RSS of a single child process across all children instead of the
aggregated max for all child processes. The only work around is to
periodically poll "memory.current" but that's not practical for
short-lived one-off cgroups.

Hence, expose memcg->watermark as "memory.peak" for v2 memcg.

Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>
---
 Documentation/admin-guide/cgroup-v2.rst |  7 +++++++
 mm/memcontrol.c                         | 13 +++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 69d7a6983f78..828ce037fb2a 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1208,6 +1208,13 @@ PAGE_SIZE multiple when read back.
 	high limit is used and monitored properly, this limit's
 	utility is limited to providing the final safety net.
 
+  memory.peak
+	A read-only single value file which exists on non-root
+	cgroups.
+
+	The max memory usage recorded for the cgroup and its
+	descendants since the creation of the cgroup.
+
   memory.oom.group
 	A read-write single value file which exists on non-root
 	cgroups.  The default value is "0".
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 725f76723220..88fa70b5d8af 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -6098,6 +6098,14 @@ static u64 memory_current_read(struct cgroup_subsys_state *css,
 	return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE;
 }
 
+static u64 memory_peak_read(struct cgroup_subsys_state *css,
+			    struct cftype *cft)
+{
+	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+
+	return (u64)memcg->memory.watermark * PAGE_SIZE;
+}
+
 static int memory_min_show(struct seq_file *m, void *v)
 {
 	return seq_puts_memcg_tunable(m,
@@ -6361,6 +6369,11 @@ static struct cftype memory_files[] = {
 		.flags = CFTYPE_NOT_ON_ROOT,
 		.read_u64 = memory_current_read,
 	},
+	{
+		.name = "peak",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.read_u64 = memory_peak_read,
+	},
 	{
 		.name = "min",
 		.flags = CFTYPE_NOT_ON_ROOT,
-- 
2.28.0

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Shakeel Butt]

On Fri, May 6, 2022 at 10:09 PM Ganesan Rajagopal <rganesan@arista.com> wrote:
>
> We run a lot of automated tests when building our software and run into
> OOM scenarios when the tests run unbounded. v1 memcg exports
> memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> metric to heuristically limit the number of tests that can run in
> parallel based on per test historical data.
>
> This metric is currently not exported for v2 memcg and there is no
> other easy way of getting this information. getrusage() syscall returns
> "ru_maxrss" which can be used as an approximation but that's the max
> RSS of a single child process across all children instead of the
> aggregated max for all child processes. The only work around is to
> periodically poll "memory.current" but that's not practical for
> short-lived one-off cgroups.
>
> Hence, expose memcg->watermark as "memory.peak" for v2 memcg.
>
> Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>

Acked-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Johannes Weiner]

On Fri, May 06, 2022 at 10:09:16PM -0700, Ganesan Rajagopal wrote:
> We run a lot of automated tests when building our software and run into
> OOM scenarios when the tests run unbounded. v1 memcg exports
> memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> metric to heuristically limit the number of tests that can run in
> parallel based on per test historical data.
> 
> This metric is currently not exported for v2 memcg and there is no
> other easy way of getting this information. getrusage() syscall returns
> "ru_maxrss" which can be used as an approximation but that's the max
> RSS of a single child process across all children instead of the
> aggregated max for all child processes. The only work around is to
> periodically poll "memory.current" but that's not practical for
> short-lived one-off cgroups.
> 
> Hence, expose memcg->watermark as "memory.peak" for v2 memcg.
> 
> Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Roman Gushchin]

On Fri, May 06, 2022 at 10:09:16PM -0700, Ganesan Rajagopal wrote:
> We run a lot of automated tests when building our software and run into
> OOM scenarios when the tests run unbounded. v1 memcg exports
> memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> metric to heuristically limit the number of tests that can run in
> parallel based on per test historical data.
> 
> This metric is currently not exported for v2 memcg and there is no
> other easy way of getting this information. getrusage() syscall returns
> "ru_maxrss" which can be used as an approximation but that's the max
> RSS of a single child process across all children instead of the
> aggregated max for all child processes. The only work around is to
> periodically poll "memory.current" but that's not practical for
> short-lived one-off cgroups.
> 
> Hence, expose memcg->watermark as "memory.peak" for v2 memcg.
> 
> Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

I've been asked a couple of times about this feature, so I think it's indeed
useful.

Thank you for adding it!

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Ganesan Rajagopal]

On Wed, May 11, 2022 at 8:18 AM Roman Gushchin <roman.gushchin@linux.dev> wrote:
>
> On Fri, May 06, 2022 at 10:09:16PM -0700, Ganesan Rajagopal wrote:
> > We run a lot of automated tests when building our software and run into
> > OOM scenarios when the tests run unbounded. v1 memcg exports
> > memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> > metric to heuristically limit the number of tests that can run in
> > parallel based on per test historical data.
> >
> > This metric is currently not exported for v2 memcg and there is no
> > other easy way of getting this information. getrusage() syscall returns
> > "ru_maxrss" which can be used as an approximation but that's the max
> > RSS of a single child process across all children instead of the
> > aggregated max for all child processes. The only work around is to
> > periodically poll "memory.current" but that's not practical for
> > short-lived one-off cgroups.
> >
> > Hence, expose memcg->watermark as "memory.peak" for v2 memcg.
> >
> > Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>
>
> Acked-by: Roman Gushchin <roman.gushchin@linux.dev>
>
> I've been asked a couple of times about this feature, so I think it's indeed
> useful.
>
> Thank you for adding it!

You're welcome and thank you for the Ack. Thank you, Shakeel and
Johannes for the review and Ack. The patch has been picked up for
mm-unstable.

Ganesan

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Michal Hocko]

On Fri 06-05-22 22:09:16, Ganesan Rajagopal wrote:
> We run a lot of automated tests when building our software and run into
> OOM scenarios when the tests run unbounded. v1 memcg exports
> memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> metric to heuristically limit the number of tests that can run in
> parallel based on per test historical data.
> 
> This metric is currently not exported for v2 memcg and there is no
> other easy way of getting this information. getrusage() syscall returns
> "ru_maxrss" which can be used as an approximation but that's the max
> RSS of a single child process across all children instead of the
> aggregated max for all child processes. The only work around is to
> periodically poll "memory.current" but that's not practical for
> short-lived one-off cgroups.
> 
> Hence, expose memcg->watermark as "memory.peak" for v2 memcg.

Yes, I can imagine that a very short lived process can easily escape
from the monitoring. The memory consumption can be still significant
though.

The v1 interface allows to reset the value by writing to the file. Have
you considered that as well?
 
> Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  Documentation/admin-guide/cgroup-v2.rst |  7 +++++++
>  mm/memcontrol.c                         | 13 +++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 69d7a6983f78..828ce037fb2a 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1208,6 +1208,13 @@ PAGE_SIZE multiple when read back.
>  	high limit is used and monitored properly, this limit's
>  	utility is limited to providing the final safety net.
>  
> +  memory.peak
> +	A read-only single value file which exists on non-root
> +	cgroups.
> +
> +	The max memory usage recorded for the cgroup and its
> +	descendants since the creation of the cgroup.
> +
>    memory.oom.group
>  	A read-write single value file which exists on non-root
>  	cgroups.  The default value is "0".
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 725f76723220..88fa70b5d8af 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -6098,6 +6098,14 @@ static u64 memory_current_read(struct cgroup_subsys_state *css,
>  	return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE;
>  }
>  
> +static u64 memory_peak_read(struct cgroup_subsys_state *css,
> +			    struct cftype *cft)
> +{
> +	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> +
> +	return (u64)memcg->memory.watermark * PAGE_SIZE;
> +}
> +
>  static int memory_min_show(struct seq_file *m, void *v)
>  {
>  	return seq_puts_memcg_tunable(m,
> @@ -6361,6 +6369,11 @@ static struct cftype memory_files[] = {
>  		.flags = CFTYPE_NOT_ON_ROOT,
>  		.read_u64 = memory_current_read,
>  	},
> +	{
> +		.name = "peak",
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +		.read_u64 = memory_peak_read,
> +	},
>  	{
>  		.name = "min",
>  		.flags = CFTYPE_NOT_ON_ROOT,
> -- 
> 2.28.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Ganesan Rajagopal]

On Wed, May 11, 2022 at 12:43 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Fri 06-05-22 22:09:16, Ganesan Rajagopal wrote:
> > We run a lot of automated tests when building our software and run into
> > OOM scenarios when the tests run unbounded. v1 memcg exports
> > memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> > metric to heuristically limit the number of tests that can run in
> > parallel based on per test historical data.
> >
> > This metric is currently not exported for v2 memcg and there is no
> > other easy way of getting this information. getrusage() syscall returns
> > "ru_maxrss" which can be used as an approximation but that's the max
> > RSS of a single child process across all children instead of the
> > aggregated max for all child processes. The only work around is to
> > periodically poll "memory.current" but that's not practical for
> > short-lived one-off cgroups.
> >
> > Hence, expose memcg->watermark as "memory.peak" for v2 memcg.
>
> Yes, I can imagine that a very short lived process can easily escape
> from the monitoring. The memory consumption can be still significant
> though.
>
> The v1 interface allows to reset the value by writing to the file. Have
> you considered that as well?

I hadn't originally but this was discussed and dropped when I posted the
first version of this patch. See
https://www.spinics.net/lists/cgroups/msg32476.html

Ganesan

>
> > Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>
>
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> > ---
> >  Documentation/admin-guide/cgroup-v2.rst |  7 +++++++
> >  mm/memcontrol.c                         | 13 +++++++++++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > index 69d7a6983f78..828ce037fb2a 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1208,6 +1208,13 @@ PAGE_SIZE multiple when read back.
> >       high limit is used and monitored properly, this limit's
> >       utility is limited to providing the final safety net.
> >
> > +  memory.peak
> > +     A read-only single value file which exists on non-root
> > +     cgroups.
> > +
> > +     The max memory usage recorded for the cgroup and its
> > +     descendants since the creation of the cgroup.
> > +
> >    memory.oom.group
> >       A read-write single value file which exists on non-root
> >       cgroups.  The default value is "0".
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 725f76723220..88fa70b5d8af 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -6098,6 +6098,14 @@ static u64 memory_current_read(struct cgroup_subsys_state *css,
> >       return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE;
> >  }
> >
> > +static u64 memory_peak_read(struct cgroup_subsys_state *css,
> > +                         struct cftype *cft)
> > +{
> > +     struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> > +
> > +     return (u64)memcg->memory.watermark * PAGE_SIZE;
> > +}
> > +
> >  static int memory_min_show(struct seq_file *m, void *v)
> >  {
> >       return seq_puts_memcg_tunable(m,
> > @@ -6361,6 +6369,11 @@ static struct cftype memory_files[] = {
> >               .flags = CFTYPE_NOT_ON_ROOT,
> >               .read_u64 = memory_current_read,
> >       },
> > +     {
> > +             .name = "peak",
> > +             .flags = CFTYPE_NOT_ON_ROOT,
> > +             .read_u64 = memory_peak_read,
> > +     },
> >       {
> >               .name = "min",
> >               .flags = CFTYPE_NOT_ON_ROOT,
> > --
> > 2.28.0
>
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Michal Koutný]

Hi.

On Fri, May 06, 2022 at 10:09:16PM -0700, Ganesan Rajagopal <rganesan@arista.com> wrote:
> We run a lot of automated tests when building our software and run into
> OOM scenarios when the tests run unbounded. v1 memcg exports
> memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> metric to heuristically limit the number of tests that can run in
> parallel based on per test historical data.
> 
> This metric is currently not exported for v2 memcg and there is no
> other easy way of getting this information. getrusage() syscall returns
> "ru_maxrss" which can be used as an approximation but that's the max
> RSS of a single child process across all children instead of the
> aggregated max for all child processes. The only work around is to
> periodically poll "memory.current" but that's not practical for
> short-lived one-off cgroups.
> 
> Hence, expose memcg->watermark as "memory.peak" for v2 memcg.

It'll save some future indirections if the commit messages includes the
argument about multiple readers and purposeful irresetability.

> 
> Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>
> ---
>  Documentation/admin-guide/cgroup-v2.rst |  7 +++++++
>  mm/memcontrol.c                         | 13 +++++++++++++
>  2 files changed, 20 insertions(+)

Besides that it looks useful and correct, feel free to add
Reviewed-by: Michal Koutný <mkoutny@suse.com>

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Ganesan Rajagopal]

On Wed, May 11, 2022 at 11:19 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Hi.
>
> On Fri, May 06, 2022 at 10:09:16PM -0700, Ganesan Rajagopal <rganesan@arista.com> wrote:
> > We run a lot of automated tests when building our software and run into
> > OOM scenarios when the tests run unbounded. v1 memcg exports
> > memcg->watermark as "memory.max_usage_in_bytes" in sysfs. We use this
> > metric to heuristically limit the number of tests that can run in
> > parallel based on per test historical data.
> >
> > This metric is currently not exported for v2 memcg and there is no
> > other easy way of getting this information. getrusage() syscall returns
> > "ru_maxrss" which can be used as an approximation but that's the max
> > RSS of a single child process across all children instead of the
> > aggregated max for all child processes. The only work around is to
> > periodically poll "memory.current" but that's not practical for
> > short-lived one-off cgroups.
> >
> > Hence, expose memcg->watermark as "memory.peak" for v2 memcg.
>
> It'll save some future indirections if the commit messages includes the
> argument about multiple readers and purposeful irresetability.

Good point. The patch has already been picked up for mm-unstable. I don't
know what's the process in this situation. Should I post a "[PATCH v3]"
with an updated commit message?

>
> >
> > Signed-off-by: Ganesan Rajagopal <rganesan@arista.com>
> > ---
> >  Documentation/admin-guide/cgroup-v2.rst |  7 +++++++
> >  mm/memcontrol.c                         | 13 +++++++++++++
> >  2 files changed, 20 insertions(+)
>
> Besides that it looks useful and correct, feel free to add
> Reviewed-by: Michal Koutný <mkoutny@suse.com>

Thank you.

Ganesan

Re: [PATCH v2] mm/memcontrol: Export memcg->watermark via sysfs for v2 memcg

[Michal Koutný]

On Thu, May 12, 2022 at 08:18:01AM +0530, Ganesan Rajagopal <rganesan@arista.com> wrote:
> Good point. The patch has already been picked up for mm-unstable.

Oh, I didn't notice that.

> I don't know what's the process in this situation. Should I post a
> "[PATCH v3]" with an updated commit message?

Or you can send a fixup for folding? (I see this is something new,
you better ask Andrew.)

Michal