On Fri, Nov 02, 2012 at 03:39:58PM +0900, Minchan Kim wrote:
> Hi Mel,
>
> On Thu, Nov 01, 2012 at 08:28:14AM +0000, Mel Gorman wrote:
> > On Wed, Oct 31, 2012 at 09:48:57PM -0700, David Rientjes wrote:
> > > On Thu, 1 Nov 2012, Minchan Kim wrote:
> > >
> > > > It's not true any more.
> > > > 3.6 includes following code in try_to_free_pages
> > > >
> > > > /*
> > > > * Do not enter reclaim if fatal signal is pending. 1 is returned so
> > > > * that the page allocator does not consider triggering OOM
> > > > */
> > > > if (fatal_signal_pending(current))
> > > > return 1;
> > > >
> > > > So the hunged task never go to the OOM path and could be looping forever.
> > > >
> > >
> > > Ah, interesting. This is from commit 5515061d22f0 ("mm: throttle direct
> > > reclaimers if PF_MEMALLOC reserves are low and swap is backed by network
> > > storage"). Thanks for adding Mel to the cc.
> > >
> >
> > Indeed, thanks.
> >
> > > The oom killer specifically has logic for this condition: when calling
> > > out_of_memory() the first thing it does is
> > >
> > > if (fatal_signal_pending(current))
> > > set_thread_flag(TIF_MEMDIE);
> > >
> > > to allow it access to memory reserves so that it may exit if it's having
> > > trouble. But that ends up never happening because of the above code that
> > > Minchan has identified.
> > >
> > > So we either need to do set_thread_flag(TIF_MEMDIE) in try_to_free_pages()
> > > as well or revert that early return entirely; there's no justification
> > > given for it in the comment nor in the commit log.
> >
> > The check for fatal signal is in the wrong place. The reason it was added
> > is because a throttled process sleeps in an interruptible sleep. If a user
> > user forcibly kills a throttled process, it should not result in an OOM kill.
> >
> > > I'd rather remove it
> > > and allow the oom killer to trigger and grant access to memory reserves
> > > itself if necessary.
> > >
> > > Mel, how does commit 5515061d22f0 deal with threads looping forever if
> > > they need memory in the exit path since the oom killer never gets called?
> > >
> >
> > It doesn't. How about this?
> >
> > ---8<---
> > mm: vmscan: Check for fatal signals iff the process was throttled
> >
> > commit 5515061d22f0 ("mm: throttle direct reclaimers if PF_MEMALLOC reserves
> > are low and swap is backed by network storage") introduced a check for
> > fatal signals after a process gets throttled for network storage. The
> > intention was that if a process was throttled and got killed that it
> > should not trigger the OOM killer. As pointed out by Minchan Kim and
> > David Rientjes, this check is in the wrong place and too broad. If a
> > system is in am OOM situation and a process is exiting, it can loop in
> > __alloc_pages_slowpath() and calling direct reclaim in a loop. As the
> > fatal signal is pending it returns 1 as if it is making forward progress
> > and can effectively deadlock.
> >
> > This patch moves the fatal_signal_pending() check after throttling to
> > throttle_direct_reclaim() where it belongs.
>
> I'm not sure how below patch achieve your goal which is to prevent
> unnecessary OOM kill if throttled process is killed by user during
> throttling. If I misunderstood your goal, please correct me and
> write down it in description for making it more clear.
>
> If user kills throttled process, throttle_direct_reclaim returns true by
> this patch so try_to_free_pages returns 1. It means it doesn't call OOM
> in first path of reclaim but shortly it will try to reclaim again
> by should_alloc_retry.
Yes and it returned without calling direct reclaim.
> And since this second path, throttle_direct_reclaim
> will continue to return false so that it could end up calling OOM kill.
>
Yes except the second time it has not been throttled and it entered direct
reclaim. If it fails to make any progress it will return 0 but if this
happens, it potentially really is an OOM situation. If it manages to
reclaim, it'll be returning a positive number, is making forward
progress and should successfully exit without triggering OOM.
Note that throttle_direct_reclaim also now checks fatal_signal_pending
before deciding to throttle at all.
> Is it a your intention? If so, what's different with old version?
> This patch just delay OOM kill so what's benefit does it has?
>
In the first version it would never try to enter direct reclaim if a
fatal signal was pending but always claim that forward progress was
being made.
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>