Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1484261 > unrolled thread

Re: [RFC 0/4] mm, oom: get rid of TIF_MEMDIE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 0/4] mm, oom: get rid of TIF_MEMDIE Johannes Weiner <hannes@cmpxchg.org> - 2016-09-15 16:50 +0200
    Re: [RFC 0/4] mm, oom: get rid of TIF_MEMDIE Michal Hocko <mhocko@kernel.org> - 2016-09-16 09:20 +0200
      Re: [RFC 0/4] mm, oom: get rid of TIF_MEMDIE Johannes Weiner <hannes@cmpxchg.org> - 2016-09-19 18:20 +0200
        Re: [RFC 0/4] mm, oom: get rid of TIF_MEMDIE Michal Hocko <mhocko@kernel.org> - 2016-09-19 21:10 +0200

#1484261 — Re: [RFC 0/4] mm, oom: get rid of TIF_MEMDIE

Hi Michal,

On Thu, Sep 01, 2016 at 11:51:00AM +0200, Michal Hocko wrote:
> Hi,
> this is an early RFC to see whether the approach I've taken is acceptable.
> The series is on top of the current mmotm tree (2016-08-31-16-06). I didn't
> get to test it so it might be completely broken.
> 
> The primary point of this series is to get rid of TIF_MEMDIE finally.
> Recent changes in the oom proper allows for that finally, I believe. Now
> that all the oom victims are reapable we are no longer depending on
> ALLOC_NO_WATERMARKS because the memory held by the victim is reclaimed
> asynchronously. A partial access to memory reserves should be sufficient
> just to guarantee that the oom victim is not starved due to other
> memory consumers. This also means that we do not have to pretend to be
> conservative and give access to memory reserves only to one thread from
> the process at the time. This is patch 1.
>
> Patch 2 is a simple cleanup which turns TIF_MEMDIE users to tsk_is_oom_victim
> which is process rather than thread centric. None of those callers really
> requires to be thread aware AFAICS.
> 
> The tricky part then is exit_oom_victim vs. oom_killer_disable because
> TIF_MEMDIE acted as a token there so we had a way to count threads from
> the process. It didn't work 100% reliably and had it own issues but we
> have to replace it with something which doesn't rely on counting threads
> but rather find a moment when all threads have reached steady state in
> do_exit. This is what patch 3 does and I would really appreciate if Oleg
> could double check my thinking there. I am also CCing Al on that one
> because I am moving exit_io_context up in do_exit right before exit_notify.

You're explaining the mechanical thing you are doing, but I'm having
trouble understanding why you want to get rid of TIF_MEMDIE. For one,
it's more code. And apparently, it's also more complicated than what
we have right now.

Can you please explain in the cover letter what's broken/undesirable?

Thanks

[toc] | [next] | [standalone]

#1484725

On Thu 15-09-16 10:41:18, Johannes Weiner wrote:
> Hi Michal,
> 
> On Thu, Sep 01, 2016 at 11:51:00AM +0200, Michal Hocko wrote:
> > Hi,
> > this is an early RFC to see whether the approach I've taken is acceptable.
> > The series is on top of the current mmotm tree (2016-08-31-16-06). I didn't
> > get to test it so it might be completely broken.
> > 
> > The primary point of this series is to get rid of TIF_MEMDIE finally.
> > Recent changes in the oom proper allows for that finally, I believe. Now
> > that all the oom victims are reapable we are no longer depending on
> > ALLOC_NO_WATERMARKS because the memory held by the victim is reclaimed
> > asynchronously. A partial access to memory reserves should be sufficient
> > just to guarantee that the oom victim is not starved due to other
> > memory consumers. This also means that we do not have to pretend to be
> > conservative and give access to memory reserves only to one thread from
> > the process at the time. This is patch 1.
> >
> > Patch 2 is a simple cleanup which turns TIF_MEMDIE users to tsk_is_oom_victim
> > which is process rather than thread centric. None of those callers really
> > requires to be thread aware AFAICS.
> > 
> > The tricky part then is exit_oom_victim vs. oom_killer_disable because
> > TIF_MEMDIE acted as a token there so we had a way to count threads from
> > the process. It didn't work 100% reliably and had it own issues but we
> > have to replace it with something which doesn't rely on counting threads
> > but rather find a moment when all threads have reached steady state in
> > do_exit. This is what patch 3 does and I would really appreciate if Oleg
> > could double check my thinking there. I am also CCing Al on that one
> > because I am moving exit_io_context up in do_exit right before exit_notify.
> 
> You're explaining the mechanical thing you are doing, but I'm having
> trouble understanding why you want to get rid of TIF_MEMDIE. For one,
> it's more code. And apparently, it's also more complicated than what
> we have right now.
> 
> Can you please explain in the cover letter what's broken/undesirable?

Sure, I will extend the cover when submitting the series again. This RFC
was mostly aimed at correctness so I focused more on technical details.
Patch 1 should contain some reasoning. Do you find it sufficient or I
should extend on top of that?

Thanks!

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1486644

On Fri, Sep 16, 2016 at 09:15:17AM +0200, Michal Hocko wrote:
> On Thu 15-09-16 10:41:18, Johannes Weiner wrote:
> > Hi Michal,
> > 
> > On Thu, Sep 01, 2016 at 11:51:00AM +0200, Michal Hocko wrote:
> > > Hi,
> > > this is an early RFC to see whether the approach I've taken is acceptable.
> > > The series is on top of the current mmotm tree (2016-08-31-16-06). I didn't
> > > get to test it so it might be completely broken.
> > > 
> > > The primary point of this series is to get rid of TIF_MEMDIE finally.
> > > Recent changes in the oom proper allows for that finally, I believe. Now
> > > that all the oom victims are reapable we are no longer depending on
> > > ALLOC_NO_WATERMARKS because the memory held by the victim is reclaimed
> > > asynchronously. A partial access to memory reserves should be sufficient
> > > just to guarantee that the oom victim is not starved due to other
> > > memory consumers. This also means that we do not have to pretend to be
> > > conservative and give access to memory reserves only to one thread from
> > > the process at the time. This is patch 1.
> > >
> > > Patch 2 is a simple cleanup which turns TIF_MEMDIE users to tsk_is_oom_victim
> > > which is process rather than thread centric. None of those callers really
> > > requires to be thread aware AFAICS.
> > > 
> > > The tricky part then is exit_oom_victim vs. oom_killer_disable because
> > > TIF_MEMDIE acted as a token there so we had a way to count threads from
> > > the process. It didn't work 100% reliably and had it own issues but we
> > > have to replace it with something which doesn't rely on counting threads
> > > but rather find a moment when all threads have reached steady state in
> > > do_exit. This is what patch 3 does and I would really appreciate if Oleg
> > > could double check my thinking there. I am also CCing Al on that one
> > > because I am moving exit_io_context up in do_exit right before exit_notify.
> > 
> > You're explaining the mechanical thing you are doing, but I'm having
> > trouble understanding why you want to get rid of TIF_MEMDIE. For one,
> > it's more code. And apparently, it's also more complicated than what
> > we have right now.
> > 
> > Can you please explain in the cover letter what's broken/undesirable?
> 
> Sure, I will extend the cover when submitting the series again. This RFC
> was mostly aimed at correctness so I focused more on technical details.
> Patch 1 should contain some reasoning. Do you find it sufficient or I
> should extend on top of that?

: For ages we have been relying on TIF_MEMDIE thread flag to mark OOM
: victims and then, among other things, to give these threads full
: access to memory reserves. There are few shortcomings of this
: implementation, though.
: 
: First of all and the most serious one is that the full access to memory
: reserves is quite dangerous because we leave no safety room for the
: system to operate and potentially do last emergency steps to move on.

Do we encounter this in practice? I think one of the clues is that you
introduce the patch with "for ages we have been doing X", so I'd like
to see a more practical explanation of how we did it was flawed.

: Secondly this flag is per task_struct while the OOM killer operates
: on mm_struct granularity so all processes sharing the given mm are
: killed. Giving the full access to all these task_structs could leave to
: a quick memory reserves depletion. We have tried to reduce this risk by
: giving TIF_MEMDIE only to the main thread and the currently allocating
: task but that doesn't really solve this problem while it surely opens up
: a room for corner cases - e.g. GFP_NO{FS,IO} requests might loop inside
: the allocator without access to memory reserves because a particular
: thread was not the group leader.

Same here I guess.

It *sounds* to me like there are two different things going on
here. One being the access to emergency reserves, the other being the
synchronization token to count OOM victims. Maybe it would be easier
to separate those issues out in the line of argument?

For emergency reserves, you make the point that we now have the reaper
and don't rely on the reserves as much anymore. However, we need to
consider that the reaper relies on the mmap_sem and is thus not as
robust as the concept of an emergency pool to guarantee fwd progress.

For synchronization, I don't quite get the argument. The patch 4
changelog uses term like "not optimal" and that we "should" be doing
certain things, but doesn't explain the consequences of the "wrong"
behavior, the impact is of frozen threads getting left behind etc.

That stuff would be useful to know, both in the cover letter (higher
level user impact) and the changelogs (more detailed user impact).

I.e. sell the problem before selling the solution :-)

Thanks!

[toc] | [prev] | [next] | [standalone]

#1486774

On Mon 19-09-16 12:18:37, Johannes Weiner wrote:
> On Fri, Sep 16, 2016 at 09:15:17AM +0200, Michal Hocko wrote:
[...]
> : For ages we have been relying on TIF_MEMDIE thread flag to mark OOM
> : victims and then, among other things, to give these threads full
> : access to memory reserves. There are few shortcomings of this
> : implementation, though.
> : 
> : First of all and the most serious one is that the full access to memory
> : reserves is quite dangerous because we leave no safety room for the
> : system to operate and potentially do last emergency steps to move on.
> 
> Do we encounter this in practice?

We rarely experience anything from this in practice. Even OOM is an
outstanding situation. Most of the lockups I am trying to solve are
close to non-existent. But this doesn't mean they are non-existent so as
far as the resulting code makes sense and doesn't make the situation
overly more complicated then I would go with enhancements.

> I think one of the clues is that you
> introduce the patch with "for ages we have been doing X", so I'd like
> to see a more practical explanation of how we did it was flawed.

OK, I will try harder to explain that. The core idea is that we couldn't
do anything better without the async oom killing because we were
strictly synchronous and only the victim could make some progress.

> : Secondly this flag is per task_struct while the OOM killer operates
> : on mm_struct granularity so all processes sharing the given mm are
> : killed. Giving the full access to all these task_structs could leave to
> : a quick memory reserves depletion. We have tried to reduce this risk by
> : giving TIF_MEMDIE only to the main thread and the currently allocating
> : task but that doesn't really solve this problem while it surely opens up
> : a room for corner cases - e.g. GFP_NO{FS,IO} requests might loop inside
> : the allocator without access to memory reserves because a particular
> : thread was not the group leader.
> 
> Same here I guess.
> 
> It *sounds* to me like there are two different things going on
> here. One being the access to emergency reserves, the other being the
> synchronization token to count OOM victims. Maybe it would be easier
> to separate those issues out in the line of argument?

Yes this is precisely the problem. The meaning of the flag is overloaded
for multiple purposes and it is not as easy to describe all of this and
get tangled in all the details. Over time we have reduced the locking
side of the flag but we still use it for oom_disable synchronization
and memory reserves access.

> For emergency reserves, you make the point that we now have the reaper
> and don't rely on the reserves as much anymore. However, we need to
> consider that the reaper relies on the mmap_sem and is thus not as
> robust as the concept of an emergency pool to guarantee fwd progress.

Yes it is not 100% but if we get stuck there then we will have a way to
go on to another victim so we will not lockup. On the other hand lockup
due to mmap_sem for write should be really rare because the lock is
mostly killable and taken outside of the oom victim context even more
rarely.

> For synchronization, I don't quite get the argument. The patch 4
> changelog uses term like "not optimal" and that we "should" be doing
> certain things, but doesn't explain the consequences of the "wrong"
> behavior, the impact is of frozen threads getting left behind etc.

Yes I will try harder. The primary point is a mismatch between oom
killer being per-mm operation and the flag being per-thread thing. For
the freezer context it means that only one thread is woken up while
other are still in the fridge. This wouldn't be a big deal for the pm
freezer but the cgroup freezer allows normal user to hide processes in
the fridge so users might hide processes there intentionally. The other
part of the puzzle is that not all resources are reclaimable by the
async oom killer. Say pipe/socket buffers can consume a lot of memory
while they are pinned by process not threads.

> That stuff would be useful to know, both in the cover letter (higher
> level user impact) and the changelogs (more detailed user impact).
> 
> I.e. sell the problem before selling the solution :-)

Sure, I see what you are saying. I just feel there are so many subtle
details that it is hard to squeeze all of them into the changelog and
still make some sense ;)

Anyway, I will definitely try to be more specific in the next post.
Thanks for the feedback!

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web