Re: [PATCH v2 2/3] mm, memcg: propagate memory effective protection on setting memory.min/low
On Tue, Jun 12, 2018 at 11:52:42AM -0400, Johannes Weiner wrote:
> On Mon, Jun 11, 2018 at 10:54:17AM -0700, Roman Gushchin wrote:
> > Explicitly propagate effective memory min/low values down by the tree.
> >
> > If there is the global memory pressure, it's not really necessary.
> > Effective memory guarantees will be propagated automatically as we
> > traverse memory cgroup tree in the reclaim path.
> >
> > But if there is no global memory pressure, effective memory protection
> > still matters for local (memcg-scoped) memory pressure. So, we have to
> > update effective limits in the subtree, if a user changes memory.min and
> > memory.low values.
> >
> > Link: http://lkml.kernel.org/r/20180522132528.23769-1-g...@fb.com
> > Signed-off-by: Roman Gushchin
> > Cc: Johannes Weiner
> > Cc: Michal Hocko
> > Cc: Vladimir Davydov
> > Cc: Greg Thelen
> > Cc: Tejun Heo
> > Cc: Shuah Khan
> > Signed-off-by: Andrew Morton
> > ---
> > mm/memcontrol.c | 14 --
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 5a3873e9d657..485df6f63d26 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5084,7 +5084,7 @@ static int memory_min_show(struct seq_file *m, void
> > *v)
> > static ssize_t memory_min_write(struct kernfs_open_file *of,
> > char *buf, size_t nbytes, loff_t off)
> > {
> > - struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> > + struct mem_cgroup *iter, *memcg = mem_cgroup_from_css(of_css(of));
> > unsigned long min;
> > int err;
> >
> > @@ -5095,6 +5095,11 @@ static ssize_t memory_min_write(struct
> > kernfs_open_file *of,
> >
> > page_counter_set_min(>memory, min);
> >
> > + rcu_read_lock();
> > + for_each_mem_cgroup_tree(iter, memcg)
> > + mem_cgroup_protected(NULL, iter);
> > + rcu_read_unlock();
>
> I'm not quite following. mem_cgroup_protected() is a just-in-time
> query that depends on the groups' usage. How does it make sense to run
> this at the time the limit is set?
mem_cgroup_protected() emulates memory pressure to propagate
effective memory guarantee values.
>
> Also, why is target reclaim different from global reclaim here? We
> have all the information we need, even if we don't start at the
> root_mem_cgroup. If we enter target reclaim against a specific cgroup,
> yes, we don't know the elow it receives from its parents. What we *do*
> know, though, is that it hit its own hard limit. What is happening
> higher up that group doesn't matter for the purpose of protection.
>
> I.e. it seems to me that instead of this patch we should be treating
> the reclaim root and its first-level children the same way we treat
> root_mem_cgroup and top-level cgroups: no protection for the root,
> first children use their low setting as the elow, all descendants get
> the proportional low-usage distribution.
>
Ok, we can keep it this way. We can have some races between the global
and targeted reclaim, but it's fine.
Andrew,
can you, please, drop these patches from the mm tree:
selftests: cgroup: add test for memory.low corner cases
mm, memcg: don't skip memory guarantee calculations
mm, memcg: propagate memory effective protection on setting memory.min/low
The null pointer fix ("b2c21aa3690a mm: fix null pointer dereference in
mem_cgroup_protected")
should be kept and merged asap.
Thank you!
Re: [PATCH v2 2/3] mm, memcg: propagate memory effective protection on setting memory.min/low
On Tue, Jun 12, 2018 at 11:52:42AM -0400, Johannes Weiner wrote:
> On Mon, Jun 11, 2018 at 10:54:17AM -0700, Roman Gushchin wrote:
> > Explicitly propagate effective memory min/low values down by the tree.
> >
> > If there is the global memory pressure, it's not really necessary.
> > Effective memory guarantees will be propagated automatically as we
> > traverse memory cgroup tree in the reclaim path.
> >
> > But if there is no global memory pressure, effective memory protection
> > still matters for local (memcg-scoped) memory pressure. So, we have to
> > update effective limits in the subtree, if a user changes memory.min and
> > memory.low values.
> >
> > Link: http://lkml.kernel.org/r/20180522132528.23769-1-g...@fb.com
> > Signed-off-by: Roman Gushchin
> > Cc: Johannes Weiner
> > Cc: Michal Hocko
> > Cc: Vladimir Davydov
> > Cc: Greg Thelen
> > Cc: Tejun Heo
> > Cc: Shuah Khan
> > Signed-off-by: Andrew Morton
> > ---
> > mm/memcontrol.c | 14 --
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 5a3873e9d657..485df6f63d26 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -5084,7 +5084,7 @@ static int memory_min_show(struct seq_file *m, void
> > *v)
> > static ssize_t memory_min_write(struct kernfs_open_file *of,
> > char *buf, size_t nbytes, loff_t off)
> > {
> > - struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> > + struct mem_cgroup *iter, *memcg = mem_cgroup_from_css(of_css(of));
> > unsigned long min;
> > int err;
> >
> > @@ -5095,6 +5095,11 @@ static ssize_t memory_min_write(struct
> > kernfs_open_file *of,
> >
> > page_counter_set_min(>memory, min);
> >
> > + rcu_read_lock();
> > + for_each_mem_cgroup_tree(iter, memcg)
> > + mem_cgroup_protected(NULL, iter);
> > + rcu_read_unlock();
>
> I'm not quite following. mem_cgroup_protected() is a just-in-time
> query that depends on the groups' usage. How does it make sense to run
> this at the time the limit is set?
mem_cgroup_protected() emulates memory pressure to propagate
effective memory guarantee values.
>
> Also, why is target reclaim different from global reclaim here? We
> have all the information we need, even if we don't start at the
> root_mem_cgroup. If we enter target reclaim against a specific cgroup,
> yes, we don't know the elow it receives from its parents. What we *do*
> know, though, is that it hit its own hard limit. What is happening
> higher up that group doesn't matter for the purpose of protection.
>
> I.e. it seems to me that instead of this patch we should be treating
> the reclaim root and its first-level children the same way we treat
> root_mem_cgroup and top-level cgroups: no protection for the root,
> first children use their low setting as the elow, all descendants get
> the proportional low-usage distribution.
>
Ok, we can keep it this way. We can have some races between the global
and targeted reclaim, but it's fine.
Andrew,
can you, please, drop these patches from the mm tree:
selftests: cgroup: add test for memory.low corner cases
mm, memcg: don't skip memory guarantee calculations
mm, memcg: propagate memory effective protection on setting memory.min/low
The null pointer fix ("b2c21aa3690a mm: fix null pointer dereference in
mem_cgroup_protected")
should be kept and merged asap.
Thank you!
Re: [PATCH v2 2/3] mm, memcg: propagate memory effective protection on setting memory.min/low
On Mon, Jun 11, 2018 at 10:54:17AM -0700, Roman Gushchin wrote:
> Explicitly propagate effective memory min/low values down by the tree.
>
> If there is the global memory pressure, it's not really necessary.
> Effective memory guarantees will be propagated automatically as we
> traverse memory cgroup tree in the reclaim path.
>
> But if there is no global memory pressure, effective memory protection
> still matters for local (memcg-scoped) memory pressure. So, we have to
> update effective limits in the subtree, if a user changes memory.min and
> memory.low values.
>
> Link: http://lkml.kernel.org/r/20180522132528.23769-1-g...@fb.com
> Signed-off-by: Roman Gushchin
> Cc: Johannes Weiner
> Cc: Michal Hocko
> Cc: Vladimir Davydov
> Cc: Greg Thelen
> Cc: Tejun Heo
> Cc: Shuah Khan
> Signed-off-by: Andrew Morton
> ---
> mm/memcontrol.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5a3873e9d657..485df6f63d26 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5084,7 +5084,7 @@ static int memory_min_show(struct seq_file *m, void *v)
> static ssize_t memory_min_write(struct kernfs_open_file *of,
> char *buf, size_t nbytes, loff_t off)
> {
> - struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> + struct mem_cgroup *iter, *memcg = mem_cgroup_from_css(of_css(of));
> unsigned long min;
> int err;
>
> @@ -5095,6 +5095,11 @@ static ssize_t memory_min_write(struct
> kernfs_open_file *of,
>
> page_counter_set_min(>memory, min);
>
> + rcu_read_lock();
> + for_each_mem_cgroup_tree(iter, memcg)
> + mem_cgroup_protected(NULL, iter);
> + rcu_read_unlock();
I'm not quite following. mem_cgroup_protected() is a just-in-time
query that depends on the groups' usage. How does it make sense to run
this at the time the limit is set?
Also, why is target reclaim different from global reclaim here? We
have all the information we need, even if we don't start at the
root_mem_cgroup. If we enter target reclaim against a specific cgroup,
yes, we don't know the elow it receives from its parents. What we *do*
know, though, is that it hit its own hard limit. What is happening
higher up that group doesn't matter for the purpose of protection.
I.e. it seems to me that instead of this patch we should be treating
the reclaim root and its first-level children the same way we treat
root_mem_cgroup and top-level cgroups: no protection for the root,
first children use their low setting as the elow, all descendants get
the proportional low-usage distribution.
Re: [PATCH v2 2/3] mm, memcg: propagate memory effective protection on setting memory.min/low
On Mon, Jun 11, 2018 at 10:54:17AM -0700, Roman Gushchin wrote:
> Explicitly propagate effective memory min/low values down by the tree.
>
> If there is the global memory pressure, it's not really necessary.
> Effective memory guarantees will be propagated automatically as we
> traverse memory cgroup tree in the reclaim path.
>
> But if there is no global memory pressure, effective memory protection
> still matters for local (memcg-scoped) memory pressure. So, we have to
> update effective limits in the subtree, if a user changes memory.min and
> memory.low values.
>
> Link: http://lkml.kernel.org/r/20180522132528.23769-1-g...@fb.com
> Signed-off-by: Roman Gushchin
> Cc: Johannes Weiner
> Cc: Michal Hocko
> Cc: Vladimir Davydov
> Cc: Greg Thelen
> Cc: Tejun Heo
> Cc: Shuah Khan
> Signed-off-by: Andrew Morton
> ---
> mm/memcontrol.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5a3873e9d657..485df6f63d26 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5084,7 +5084,7 @@ static int memory_min_show(struct seq_file *m, void *v)
> static ssize_t memory_min_write(struct kernfs_open_file *of,
> char *buf, size_t nbytes, loff_t off)
> {
> - struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> + struct mem_cgroup *iter, *memcg = mem_cgroup_from_css(of_css(of));
> unsigned long min;
> int err;
>
> @@ -5095,6 +5095,11 @@ static ssize_t memory_min_write(struct
> kernfs_open_file *of,
>
> page_counter_set_min(>memory, min);
>
> + rcu_read_lock();
> + for_each_mem_cgroup_tree(iter, memcg)
> + mem_cgroup_protected(NULL, iter);
> + rcu_read_unlock();
I'm not quite following. mem_cgroup_protected() is a just-in-time
query that depends on the groups' usage. How does it make sense to run
this at the time the limit is set?
Also, why is target reclaim different from global reclaim here? We
have all the information we need, even if we don't start at the
root_mem_cgroup. If we enter target reclaim against a specific cgroup,
yes, we don't know the elow it receives from its parents. What we *do*
know, though, is that it hit its own hard limit. What is happening
higher up that group doesn't matter for the purpose of protection.
I.e. it seems to me that instead of this patch we should be treating
the reclaim root and its first-level children the same way we treat
root_mem_cgroup and top-level cgroups: no protection for the root,
first children use their low setting as the elow, all descendants get
the proportional low-usage distribution.
