Re: [patch v2] mm, memcg: do not retry precharge charges
On Sat 14-01-17 21:42:48, David Rientjes wrote: > On Sat, 14 Jan 2017, Johannes Weiner wrote: > > > The OOM killer livelock was the motivation for this patch. With that > > ruled out, what's the point of this patch? Try a bit less hard to move > > charges during task migration? > > > > Most important part is to fail ->can_attach() instead of oom killing > processes when attaching a process to a memcg hierarchy. But we are not invoking the oom killer from this path even without __GFP_NORETRY. Or am I missing your point? -- Michal Hocko SUSE Labs
Re: [patch v2] mm, memcg: do not retry precharge charges
On Sat 14-01-17 21:42:48, David Rientjes wrote: > On Sat, 14 Jan 2017, Johannes Weiner wrote: > > > The OOM killer livelock was the motivation for this patch. With that > > ruled out, what's the point of this patch? Try a bit less hard to move > > charges during task migration? > > > > Most important part is to fail ->can_attach() instead of oom killing > processes when attaching a process to a memcg hierarchy. But we are not invoking the oom killer from this path even without __GFP_NORETRY. Or am I missing your point? -- Michal Hocko SUSE Labs
Re: [patch v2] mm, memcg: do not retry precharge charges
On Sat, Jan 14, 2017 at 09:42:48PM -0800, David Rientjes wrote: > On Sat, 14 Jan 2017, Johannes Weiner wrote: > > > The OOM killer livelock was the motivation for this patch. With that > > ruled out, what's the point of this patch? Try a bit less hard to move > > charges during task migration? > > > > Most important part is to fail ->can_attach() instead of oom killing > processes when attaching a process to a memcg hierarchy. Ah, that makes sense. Could you please update the changelog to reflect this? Thanks!
Re: [patch v2] mm, memcg: do not retry precharge charges
On Sat, Jan 14, 2017 at 09:42:48PM -0800, David Rientjes wrote: > On Sat, 14 Jan 2017, Johannes Weiner wrote: > > > The OOM killer livelock was the motivation for this patch. With that > > ruled out, what's the point of this patch? Try a bit less hard to move > > charges during task migration? > > > > Most important part is to fail ->can_attach() instead of oom killing > processes when attaching a process to a memcg hierarchy. Ah, that makes sense. Could you please update the changelog to reflect this? Thanks!
Re: [patch v2] mm, memcg: do not retry precharge charges
On Sat, 14 Jan 2017, Johannes Weiner wrote: > The OOM killer livelock was the motivation for this patch. With that > ruled out, what's the point of this patch? Try a bit less hard to move > charges during task migration? > Most important part is to fail ->can_attach() instead of oom killing processes when attaching a process to a memcg hierarchy.
Re: [patch v2] mm, memcg: do not retry precharge charges
On Sat, 14 Jan 2017, Johannes Weiner wrote: > The OOM killer livelock was the motivation for this patch. With that > ruled out, what's the point of this patch? Try a bit less hard to move > charges during task migration? > Most important part is to fail ->can_attach() instead of oom killing processes when attaching a process to a memcg hierarchy.
Re: [patch v2] mm, memcg: do not retry precharge charges
On Fri, Jan 13, 2017 at 02:09:53AM -0800, David Rientjes wrote: > When memory.move_charge_at_immigrate is enabled and precharges are > depleted during move, mem_cgroup_move_charge_pte_range() will attempt to > increase the size of the precharge. > > Prevent precharges from ever looping by setting __GFP_NORETRY. This was > probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is > pointless as written. The OOM killer livelock was the motivation for this patch. With that ruled out, what's the point of this patch? Try a bit less hard to move charges during task migration?
Re: [patch v2] mm, memcg: do not retry precharge charges
On Fri, Jan 13, 2017 at 02:09:53AM -0800, David Rientjes wrote: > When memory.move_charge_at_immigrate is enabled and precharges are > depleted during move, mem_cgroup_move_charge_pte_range() will attempt to > increase the size of the precharge. > > Prevent precharges from ever looping by setting __GFP_NORETRY. This was > probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is > pointless as written. The OOM killer livelock was the motivation for this patch. With that ruled out, what's the point of this patch? Try a bit less hard to move charges during task migration?
Re: [patch v2] mm, memcg: do not retry precharge charges
When memory.move_charge_at_immigrate is enabled and precharges are
depleted during move, mem_cgroup_move_charge_pte_range() will attempt to
increase the size of the precharge.
Prevent precharges from ever looping by setting __GFP_NORETRY. This was
probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is
pointless as written.
Fixes: 0029e19ebf84 ("mm: memcontrol: remove explicit OOM parameter in charge
path")
Acked-by: Michal Hocko
Signed-off-by: David Rientjes
---
mm/memcontrol.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4353,9 +4353,9 @@ static int mem_cgroup_do_precharge(unsigned long count)
return ret;
}
- /* Try charges one by one with reclaim */
+ /* Try charges one by one with reclaim, but do not retry */
while (count--) {
- ret = try_charge(mc.to, GFP_KERNEL & ~__GFP_NORETRY, 1);
+ ret = try_charge(mc.to, GFP_KERNEL | __GFP_NORETRY, 1);
if (ret)
return ret;
mc.precharge++;
Re: [patch v2] mm, memcg: do not retry precharge charges
When memory.move_charge_at_immigrate is enabled and precharges are
depleted during move, mem_cgroup_move_charge_pte_range() will attempt to
increase the size of the precharge.
Prevent precharges from ever looping by setting __GFP_NORETRY. This was
probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is
pointless as written.
Fixes: 0029e19ebf84 ("mm: memcontrol: remove explicit OOM parameter in charge
path")
Acked-by: Michal Hocko
Signed-off-by: David Rientjes
---
mm/memcontrol.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4353,9 +4353,9 @@ static int mem_cgroup_do_precharge(unsigned long count)
return ret;
}
- /* Try charges one by one with reclaim */
+ /* Try charges one by one with reclaim, but do not retry */
while (count--) {
- ret = try_charge(mc.to, GFP_KERNEL & ~__GFP_NORETRY, 1);
+ ret = try_charge(mc.to, GFP_KERNEL | __GFP_NORETRY, 1);
if (ret)
return ret;
mc.precharge++;
Re: [patch v2] mm, memcg: do not retry precharge charges
On Thu 12-01-17 14:46:34, David Rientjes wrote:
> When memory.move_charge_at_immigrate is enabled and precharges are
> depleted during move, mem_cgroup_move_charge_pte_range() will attempt to
> increase the size of the precharge.
>
> This can be allowed to do reclaim, but should not call the oom killer to
> oom kill a process. It's better to fail the attach rather than oom kill
> a process attached to the memcg hierarchy.
This is not the case though since 3812c8c8f395 ("mm: memcg: do not trap
chargers with full callstack on OOM") - 3.12. Only the page fault path
is allowed to trigger the oom killer.
> Prevent precharges from ever looping by setting __GFP_NORETRY. This was
> probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is
> pointless as written.
>
> Fixes: 0029e19ebf84 ("mm: memcontrol: remove explicit OOM parameter in charge
> path")
> Signed-off-by: David Rientjes
Without the note about the oom killer you can add
Acked-by: Michal Hocko
Thanks!
> ---
> mm/memcontrol.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4353,9 +4353,12 @@ static int mem_cgroup_do_precharge(unsigned long count)
> return ret;
> }
>
> - /* Try charges one by one with reclaim */
> + /*
> + * Try charges one by one with reclaim, but do not retry. This avoids
> + * calling the oom killer when the precharge should just fail.
> + */
> while (count--) {
> - ret = try_charge(mc.to, GFP_KERNEL & ~__GFP_NORETRY, 1);
> + ret = try_charge(mc.to, GFP_KERNEL | __GFP_NORETRY, 1);
> if (ret)
> return ret;
> mc.precharge++;
--
Michal Hocko
SUSE Labs
Re: [patch v2] mm, memcg: do not retry precharge charges
On Thu 12-01-17 14:46:34, David Rientjes wrote:
> When memory.move_charge_at_immigrate is enabled and precharges are
> depleted during move, mem_cgroup_move_charge_pte_range() will attempt to
> increase the size of the precharge.
>
> This can be allowed to do reclaim, but should not call the oom killer to
> oom kill a process. It's better to fail the attach rather than oom kill
> a process attached to the memcg hierarchy.
This is not the case though since 3812c8c8f395 ("mm: memcg: do not trap
chargers with full callstack on OOM") - 3.12. Only the page fault path
is allowed to trigger the oom killer.
> Prevent precharges from ever looping by setting __GFP_NORETRY. This was
> probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is
> pointless as written.
>
> Fixes: 0029e19ebf84 ("mm: memcontrol: remove explicit OOM parameter in charge
> path")
> Signed-off-by: David Rientjes
Without the note about the oom killer you can add
Acked-by: Michal Hocko
Thanks!
> ---
> mm/memcontrol.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4353,9 +4353,12 @@ static int mem_cgroup_do_precharge(unsigned long count)
> return ret;
> }
>
> - /* Try charges one by one with reclaim */
> + /*
> + * Try charges one by one with reclaim, but do not retry. This avoids
> + * calling the oom killer when the precharge should just fail.
> + */
> while (count--) {
> - ret = try_charge(mc.to, GFP_KERNEL & ~__GFP_NORETRY, 1);
> + ret = try_charge(mc.to, GFP_KERNEL | __GFP_NORETRY, 1);
> if (ret)
> return ret;
> mc.precharge++;
--
Michal Hocko
SUSE Labs
[patch v2] mm, memcg: do not retry precharge charges
When memory.move_charge_at_immigrate is enabled and precharges are
depleted during move, mem_cgroup_move_charge_pte_range() will attempt to
increase the size of the precharge.
This can be allowed to do reclaim, but should not call the oom killer to
oom kill a process. It's better to fail the attach rather than oom kill
a process attached to the memcg hierarchy.
Prevent precharges from ever looping by setting __GFP_NORETRY. This was
probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is
pointless as written.
Fixes: 0029e19ebf84 ("mm: memcontrol: remove explicit OOM parameter in charge
path")
Signed-off-by: David Rientjes
---
mm/memcontrol.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4353,9 +4353,12 @@ static int mem_cgroup_do_precharge(unsigned long count)
return ret;
}
- /* Try charges one by one with reclaim */
+ /*
+* Try charges one by one with reclaim, but do not retry. This avoids
+* calling the oom killer when the precharge should just fail.
+*/
while (count--) {
- ret = try_charge(mc.to, GFP_KERNEL & ~__GFP_NORETRY, 1);
+ ret = try_charge(mc.to, GFP_KERNEL | __GFP_NORETRY, 1);
if (ret)
return ret;
mc.precharge++;
[patch v2] mm, memcg: do not retry precharge charges
When memory.move_charge_at_immigrate is enabled and precharges are
depleted during move, mem_cgroup_move_charge_pte_range() will attempt to
increase the size of the precharge.
This can be allowed to do reclaim, but should not call the oom killer to
oom kill a process. It's better to fail the attach rather than oom kill
a process attached to the memcg hierarchy.
Prevent precharges from ever looping by setting __GFP_NORETRY. This was
probably the intention of the GFP_KERNEL & ~__GFP_NORETRY, which is
pointless as written.
Fixes: 0029e19ebf84 ("mm: memcontrol: remove explicit OOM parameter in charge
path")
Signed-off-by: David Rientjes
---
mm/memcontrol.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4353,9 +4353,12 @@ static int mem_cgroup_do_precharge(unsigned long count)
return ret;
}
- /* Try charges one by one with reclaim */
+ /*
+* Try charges one by one with reclaim, but do not retry. This avoids
+* calling the oom killer when the precharge should just fail.
+*/
while (count--) {
- ret = try_charge(mc.to, GFP_KERNEL & ~__GFP_NORETRY, 1);
+ ret = try_charge(mc.to, GFP_KERNEL | __GFP_NORETRY, 1);
if (ret)
return ret;
mc.precharge++;
