zram OOM behavior

PATCH In review Minchan Kim 3 participants 16 messages Nov 2, 2012 – Nov 23, 2012

Subsystems (3)

Memory management MGLRU MM reclaim

lore

Share & tools

lore

Minchan Kim

author

2012-11-02 06:33 UTC #0 lore raw

Hi Mel,

Quoted 65 lines · Mel Gorman

Quoted 12 lines · David Rientjes
Quoted 10 lines · Minchan Kim
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;
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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);
Quoted 7 lines
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.
Quoted 7 lines
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.

Quoted 2 lines

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. And since this second path, throttle_direct_reclaim will continue to return false so that it could end up calling OOM kill.

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?

Quoted 97 lines

If this patch passes review it should be considered a -stable candidate for 3.6.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/vmscan.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

mm/vmscan.c +27 ~~-11~~

index 2b7edfa..ca9e37f 100644
@@ -2238,9 +2238,12 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
  * Throttle direct reclaimers if backing storage is backed by the network
  * and the PFMEMALLOC reserve for the preferred node is getting dangerously
  * depleted. kswapd will continue to make progress and wake the processes
- * when the low watermark is reached
+ * when the low watermark is reached.
+ *
+ * Returns true if a fatal signal was delivered during throttling. If this
+ * happens, the page allocator should not consider triggering the OOM killer.
  */
-static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
+static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
 	struct zone *zone;

@@ -2255,13 +2258,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * processes to block on log_wait_commit().
 	 */
 	if (current->flags & PF_KTHREAD)
-		return;
+		goto out;
+
+	/*
+	 * If a fatal signal is pending, this process should not throttle.
+	 * It should return quickly so it can exit and free its memory
+	 */
+	if (fatal_signal_pending(current))
+		goto out;
 
 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
 	pgdat = zone->zone_pgdat;
 	if (pfmemalloc_watermark_ok(pgdat))
-		return;
+		goto out;
 
 	/* Account for the throttling */
 	count_vm_event(PGSCAN_DIRECT_THROTTLE);

@@ -2277,12 +2287,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
 			pfmemalloc_watermark_ok(pgdat), HZ);
-		return;
+
+		goto check_pending;
 	}
 
 	/* Throttle until kswapd wakes the process */
 	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
 		pfmemalloc_watermark_ok(pgdat));
+
+check_pending:
+	if (fatal_signal_pending(current))
+		return true;
+
+out:
+	return false;
 }
 
 unsigned long try_to_free_pages(struct zonelist *zonelist, int order,

@@ -2304,13 +2322,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.gfp_mask = sc.gfp_mask,
 	};
 
-	throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
-
 	/*
-	 * Do not enter reclaim if fatal signal is pending. 1 is returned so
-	 * that the page allocator does not consider triggering OOM
+	 * Do not enter reclaim if fatal signal was delivered while throttled.
+	 * 1 is returned so that the page allocator does not OOM kill at this
+	 * point.
 	 */
-	if (fatal_signal_pending(current))
+	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,
 
--

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 8 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-11-02 08:31 UTC #1 lore raw

Quoted 76 lines · Minchan Kim

Hi Mel,
Quoted 65 lines · Mel Gorman
Quoted 12 lines · David Rientjes
Quoted 10 lines · Minchan Kim
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;
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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);
Quoted 7 lines
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.
Quoted 7 lines
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.
Quoted 2 lines
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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Signature · 8 lines

Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

author

2012-11-02 22:36 UTC #2 lore raw

Quoted 97 lines · Mel Gorman

Quoted 76 lines · Minchan Kim
Hi Mel,
Quoted 65 lines · Mel Gorman
Quoted 12 lines · David Rientjes
Quoted 10 lines · Minchan Kim
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;
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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);
Quoted 7 lines
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.
Quoted 7 lines
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.
Quoted 2 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.

How about this?

mm/vmscan.c +0 -7

index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);

In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal. If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
Is it a problem for your case?

Quoted 4 lines

Signature · 3 lines
-- 
Mel Gorman
SUSE Labs

Signature · 8 lines

Kind Regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-11-05 14:46 UTC #3 lore raw

Quoted 11 lines · Minchan Kim

Quoted 4 lines
<SNIP>
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.
Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.
How about this?

Both will work as expected but....

Quoted 19 lines

mm/vmscan.c +0 -7

index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);

Quoted 2 lines

In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal.

It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.

Quoted 3 lines

If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.

While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?

Signature · 8 lines

Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

author

2012-11-06 00:25 UTC #4 lore raw

Quoted 41 lines · Mel Gorman

Quoted 11 lines · Minchan Kim
Quoted 4 lines
<SNIP>
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.
Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.
How about this?
Both will work as expected but....
Quoted 19 lines
mm/vmscan.c +0 -7
index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);
Quoted 2 lines
In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal.
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.

Quoted 6 lines

Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?

Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.

Quoted 4 lines

Signature · 3 lines
-- 
Mel Gorman
SUSE Labs

Signature · 8 lines

Kind Regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-11-06 08:58 UTC #5 lore raw

Quoted 46 lines · Minchan Kim

Quoted 45 lines · Mel Gorman
Quoted 11 lines · Minchan Kim
Quoted 4 lines
<SNIP>
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.
Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.
How about this?
Both will work as expected but....
Quoted 19 lines
mm/vmscan.c +0 -7
index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);
Quoted 2 lines
In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal.
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.
Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.

Quoted 8 lines

Quoted 2 lines
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?
Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.

Part of that reclaim can be the process reclaiming its own pages and putting them in swap just so it can exit shortly afterwards. If it was throttled in this path, it implies that swap-over-NFS is enabled where such reclaim in fact might require the pfmemalloc reserves to be used to allocate network buffers. It's potentially unnecessary work because the same reserves could have been used to just exit the process.

I'll go your way if you insist because it's not like getting throttled and killed before exit is a common situation and it should work either way.

Signature · 8 lines

Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

author

2012-11-06 10:17 UTC #6 lore raw

Quoted 56 lines · Mel Gorman

Quoted 46 lines · Minchan Kim
Quoted 45 lines · Mel Gorman
Quoted 11 lines · Minchan Kim
Quoted 4 lines
<SNIP>
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.
Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.
How about this?
Both will work as expected but....
Quoted 19 lines
mm/vmscan.c +0 -7
index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);
Quoted 2 lines
In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal.
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.
Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
Quoted 8 lines
Quoted 2 lines
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?
Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.

Quoted 3 lines

Part of that reclaim can be the process reclaiming its own pages and putting them in swap just so it can exit shortly afterwards. If it was throttled in this path, it implies that swap-over-NFS is enabled where

Could we make sure it's only the case for swap-over-NFS? I think it can happen if the system has very slow thumb card.

Quoted 2 lines

such reclaim in fact might require the pfmemalloc reserves to be used to allocate network buffers. It's potentially unnecessary work because the

You mean we need pfmemalloc reserve to swap out anon pages by swap-over-NFS? Yes. In this case, you're right. I would be better to use reserve pool for just exiting instead of swap out over network. But how can you make sure that we have only anonymous page when we try to reclaim?
If there are some file-backed pages, we can avoid swapout at that time. Maybe we need some check.

Quoted 5 lines

same reserves could have been used to just exit the process.
I'll go your way if you insist because it's not like getting throttled and killed before exit is a common situation and it should work either way.

I don't want to insist on. Just want to know what's the problem and find better solution. :)

P.S) I'm at situation which is very hard to sit down in front of computer for a long time due to really really thanksful training course. :( Shortly, I should go to dance.
Please feel free to send patch without expectation I will send patch soon.

Quoted 4 lines

Signature · 3 lines
-- 
Mel Gorman
SUSE Labs

Signature · 8 lines

Kind Regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-11-09 09:50 UTC #7 lore raw

Quoted 57 lines · Minchan Kim

Quoted 56 lines · Mel Gorman
Quoted 54 lines · Minchan Kim
Quoted 41 lines · Mel Gorman
Quoted 11 lines · Minchan Kim
Quoted 4 lines
<SNIP>
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.
Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.
How about this?
Both will work as expected but....
Quoted 19 lines
mm/vmscan.c +0 -7
index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);
Quoted 2 lines
In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal.
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.
Quoted 6 lines
Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?
Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.

Quoted 5 lines

Quoted 3 lines
Part of that reclaim can be the process reclaiming its own pages and putting them in swap just so it can exit shortly afterwards. If it was throttled in this path, it implies that swap-over-NFS is enabled where
Could we make sure it's only the case for swap-over-NFS?

The PFMEMALLOC reserves being consumed to the point of throttline is only expected in the case of swap-over-network -- check the pgscan_direct_throttle counter to be sure. So it's already the case that this throttling logic and its signal handling is mostly a swap-over-NFS thing. It is possible that a badly behaving driver using GFP_ATOMIC to allocate long-lived buffers could force a situation where a process gets throttled but I'm not aware of a case where this happens todays.

Quoted 2 lines

I think it can happen if the system has very slow thumb card.

How? They shouldn't be stuck in throttling in this case. They should be blocked on IO, congestion wait, dirty throttling etc.

Quoted 4 lines

Quoted 2 lines
such reclaim in fact might require the pfmemalloc reserves to be used to allocate network buffers. It's potentially unnecessary work because the
You mean we need pfmemalloc reserve to swap out anon pages by swap-over-NFS?

In very low-memory situations - yes. We can be at the min watermark but still need to allocate a page for a network buffer to swap out the anon page.

Quoted 6 lines

Yes. In this case, you're right. I would be better to use reserve pool for just exiting instead of swap out over network. But how can you make sure that we have only anonymous page when we try to reclaim?
If there are some file-backed pages, we can avoid swapout at that time. Maybe we need some check.

That would be a fairly invasive set of checks for a corner case. if swap-over-nfs + critically low + about to OOM + file pages available then only reclaim files.

It's getting off track as to why we're having this discussion in the first place -- looping due to improper handling of fatal signal pending.

Quoted 9 lines

Quoted 5 lines
same reserves could have been used to just exit the process.
I'll go your way if you insist because it's not like getting throttled and killed before exit is a common situation and it should work either way.
I don't want to insist on. Just want to know what's the problem and find better solution. :)

In that case, I'm going to send the patch to Andrew on Monday and avoid direct reclaim when a fatal signal is pending in the swap-over-network case. Are you ok with that?

Signature · 8 lines

Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

author

2012-11-12 13:32 UTC #8 lore raw

Sorry for the late reply.
I'm still going on training course until this week so my response would be delayed, too.

Quoted 64 lines · Mel Gorman

Quoted 57 lines · Minchan Kim
Quoted 56 lines · Mel Gorman
Quoted 54 lines · Minchan Kim
Quoted 41 lines · Mel Gorman
Quoted 11 lines · Minchan Kim
Quoted 4 lines
<SNIP>
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.
Surely we need fix for preventing deadlock with OOM kill and that's why I have Cced you and this patch fixes it but my question is why we need such fatal signal checking trick.
How about this?
Both will work as expected but....
Quoted 19 lines
mm/vmscan.c +0 -7
index 10090c8..881619e 100644
@@ -2306,13 +2306,6 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 
        throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
 
-       /*
-        * 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;
-
        trace_mm_vmscan_direct_reclaim_begin(order,
                                sc.may_writepage,
                                gfp_mask);
Quoted 2 lines
In this case, after throttling, current will try to do direct reclaim and if he makes forward progress, he will get a memory and exit if he receive KILL signal.
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.
Quoted 6 lines
Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?
Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.
Quoted 5 lines
Quoted 3 lines
Part of that reclaim can be the process reclaiming its own pages and putting them in swap just so it can exit shortly afterwards. If it was throttled in this path, it implies that swap-over-NFS is enabled where
Could we make sure it's only the case for swap-over-NFS?

Quoted 7 lines

The PFMEMALLOC reserves being consumed to the point of throttline is only expected in the case of swap-over-network -- check the pgscan_direct_throttle counter to be sure. So it's already the case that this throttling logic and its signal handling is mostly a swap-over-NFS thing. It is possible that a badly behaving driver using GFP_ATOMIC to allocate long-lived buffers could force a situation where a process gets throttled but I'm not aware of a case where this happens todays.

I saw some custom drviers in embedded side have used GFP_ATOMIC easily to protect avoiding deadlock. Of course, it's not a good behavior but it lives with us. Even, we can't fix it because we don't have any source. :(

Quoted 6 lines

Quoted 3 lines
I think it can happen if the system has very slow thumb card.
How? They shouldn't be stuck in throttling in this case. They should be blocked on IO, congestion wait, dirty throttling etc.

Some block driver(ex, mmc) uses a thread model with PF_MEMALLOC so I think they can be stucked by the throttling logic.

Quoted 16 lines

Quoted 5 lines
Quoted 3 lines
such reclaim in fact might require the pfmemalloc reserves to be used to allocate network buffers. It's potentially unnecessary work because the
You mean we need pfmemalloc reserve to swap out anon pages by swap-over-NFS?
In very low-memory situations - yes. We can be at the min watermark but still need to allocate a page for a network buffer to swap out the anon page.
Quoted 6 lines
Yes. In this case, you're right. I would be better to use reserve pool for just exiting instead of swap out over network. But how can you make sure that we have only anonymous page when we try to reclaim?
If there are some file-backed pages, we can avoid swapout at that time. Maybe we need some check.

Quoted 6 lines

That would be a fairly invasive set of checks for a corner case. if swap-over-nfs + critically low + about to OOM + file pages available then only reclaim files.
It's getting off track as to why we're having this discussion in the first place -- looping due to improper handling of fatal signal pending.

If some user tune /proc/sys/vm/swappiness, we could have many page cache pages when swap-over-NFS happens.
My point is that why do we should use emergency memory pool although we have reclaimalble memory?

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
same reserves could have been used to just exit the process.
I'll go your way if you insist because it's not like getting throttled and killed before exit is a common situation and it should work either way.

Quoted 7 lines

Quoted 3 lines
I don't want to insist on. Just want to know what's the problem and find better solution. :)
In that case, I'm going to send the patch to Andrew on Monday and avoid direct reclaim when a fatal signal is pending in the swap-over-network case. Are you ok with that?

Sorry but I don't think your patch is best approach.

Quoted 4 lines

Signature · 3 lines
-- 
Mel Gorman
SUSE Labs

Signature · 8 lines

Kind Regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-11-12 14:06 UTC #9 lore raw

Quoted 35 lines · Minchan Kim

Sorry for the late reply.
I'm still going on training course until this week so my response would be delayed, too.
Quoted 24 lines
Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Quoted 11 lines
<SNIP>
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.
Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?
Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.
Quoted 5 lines
Quoted 3 lines
Part of that reclaim can be the process reclaiming its own pages and putting them in swap just so it can exit shortly afterwards. If it was throttled in this path, it implies that swap-over-NFS is enabled where
Could we make sure it's only the case for swap-over-NFS?
Quoted 7 lines
The PFMEMALLOC reserves being consumed to the point of throttline is only expected in the case of swap-over-network -- check the pgscan_direct_throttle counter to be sure. So it's already the case that this throttling logic and its signal handling is mostly a swap-over-NFS thing. It is possible that a badly behaving driver using GFP_ATOMIC to allocate long-lived buffers could force a situation where a process gets throttled but I'm not aware of a case where this happens todays.

Quoted 2 lines

I saw some custom drviers in embedded side have used GFP_ATOMIC easily to protect avoiding deadlock.

They must be getting a lot of allocation failures in that case.

Quoted 4 lines

Of course, it's not a good behavior but it lives with us. Even, we can't fix it because we don't have any source. :(
Quoted 1 line

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
I think it can happen if the system has very slow thumb card.
How? They shouldn't be stuck in throttling in this case. They should be blocked on IO, congestion wait, dirty throttling etc.
Some block driver(ex, mmc) uses a thread model with PF_MEMALLOC so I think they can be stucked by the throttling logic.

If they are using PF_MEMALLOC + GFP_ATOMIC, there is a strong chance that they'll actually deadlock their system if there are a storm of allocations. The drivers is fundamentally broken in a dangerous way. None of that is fixed by forcing an exiting process to enter direct reclaim.

Quoted 20 lines

Quoted 20 lines
Quoted 5 lines
Quoted 3 lines
such reclaim in fact might require the pfmemalloc reserves to be used to allocate network buffers. It's potentially unnecessary work because the
You mean we need pfmemalloc reserve to swap out anon pages by swap-over-NFS?
In very low-memory situations - yes. We can be at the min watermark but still need to allocate a page for a network buffer to swap out the anon page.
Quoted 6 lines
Yes. In this case, you're right. I would be better to use reserve pool for just exiting instead of swap out over network. But how can you make sure that we have only anonymous page when we try to reclaim?
If there are some file-backed pages, we can avoid swapout at that time. Maybe we need some check.
That would be a fairly invasive set of checks for a corner case. if swap-over-nfs + critically low + about to OOM + file pages available then only reclaim files.

Quoted 5 lines

Quoted 2 lines
It's getting off track as to why we're having this discussion in the first place -- looping due to improper handling of fatal signal pending.
If some user tune /proc/sys/vm/swappiness, we could have many page cache pages when swap-over-NFS happens.

That's a BIG if. swappiness could be anything and it'll depend on the workload anyway.

Quoted 3 lines

My point is that why do we should use emergency memory pool although we have reclaimalble memory?

Because as I have already pointed out, the use of swap-over-nfs itself creates more allocation pressure if it is used in the reclaim path. The emergency memory pool is used *anyway* unless there are clean file pages that can be discarded. But that's a big "if". The safer path is to try and exit and if *that* fails *then* enter direct reclaim.

Signature · 8 lines

Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

author

2012-11-13 13:31 UTC #10 lore raw

Quoted 36 lines · Mel Gorman

Quoted 35 lines · Minchan Kim
Sorry for the late reply.
I'm still going on training course until this week so my response would be delayed, too.
Quoted 31 lines
Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
Quoted 11 lines
<SNIP>
It may be completely unnecessary to reclaim memory if the process that was throttled and killed just exits quickly. As the fatal signal is pending it will be able to use the pfmemalloc reserves.
Quoted 3 lines
If he can't make forward progress with direct reclaim, he can ends up OOM path but out_of_memory checks signal check of current and allow to access reserved memory pool for quick exit and return without killing other victim selection.
While this is true, what advantage is there to having a killed process potentially reclaiming memory it does not need to?
Killed process needs a memory for him to be terminated. I think it's not a good idea for him to use reserved memory pool unconditionally although he is throtlled and killed. Because reserved memory pool is very stricted resource for emergency so using reserved memory pool should be last resort after he fail to reclaim.
Quoted 5 lines
Quoted 3 lines
Part of that reclaim can be the process reclaiming its own pages and putting them in swap just so it can exit shortly afterwards. If it was throttled in this path, it implies that swap-over-NFS is enabled where
Could we make sure it's only the case for swap-over-NFS?
The PFMEMALLOC reserves being consumed to the point of throttline is only expected in the case of swap-over-network -- check the pgscan_direct_throttle counter to be sure. So it's already the case that this throttling logic and its signal handling is mostly a swap-over-NFS thing. It is possible that a badly behaving driver using GFP_ATOMIC to allocate long-lived buffers could force a situation where a process gets throttled but I'm not aware of a case where this happens todays.

Quoted 4 lines

Quoted 2 lines
I saw some custom drviers in embedded side have used GFP_ATOMIC easily to protect avoiding deadlock.
They must be getting a lot of allocation failures in that case.

It depends on workload and I didn't received any report from them.

Quoted 11 lines

Quoted 11 lines
Of course, it's not a good behavior but it lives with us. Even, we can't fix it because we don't have any source. :(
Quoted 6 lines
Quoted 3 lines
I think it can happen if the system has very slow thumb card.
How? They shouldn't be stuck in throttling in this case. They should be blocked on IO, congestion wait, dirty throttling etc.

Quoted 8 lines

Quoted 3 lines
Some block driver(ex, mmc) uses a thread model with PF_MEMALLOC so I think they can be stucked by the throttling logic.
If they are using PF_MEMALLOC + GFP_ATOMIC, there is a strong chance that they'll actually deadlock their system if there are a storm of allocations. The drivers is fundamentally broken in a dangerous way. None of that is fixed by forcing an exiting process to enter direct reclaim.

Agreed.

Quoted 21 lines

Quoted 21 lines
Quoted 21 lines
Quoted 6 lines
Quoted 4 lines
such reclaim in fact might require the pfmemalloc reserves to be used to allocate network buffers. It's potentially unnecessary work because the
You mean we need pfmemalloc reserve to swap out anon pages by swap-over-NFS?
In very low-memory situations - yes. We can be at the min watermark but still need to allocate a page for a network buffer to swap out the anon page.
Quoted 6 lines
Yes. In this case, you're right. I would be better to use reserve pool for just exiting instead of swap out over network. But how can you make sure that we have only anonymous page when we try to reclaim?
If there are some file-backed pages, we can avoid swapout at that time. Maybe we need some check.
That would be a fairly invasive set of checks for a corner case. if swap-over-nfs + critically low + about to OOM + file pages available then only reclaim files.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
It's getting off track as to why we're having this discussion in the first place -- looping due to improper handling of fatal signal pending.
If some user tune /proc/sys/vm/swappiness, we could have many page cache pages when swap-over-NFS happens.
That's a BIG if. swappiness could be anything and it'll depend on the workload anyway.

Yes but we don't have to ignore such case.

Quoted 10 lines

Quoted 4 lines
My point is that why do we should use emergency memory pool although we have reclaimalble memory?
Because as I have already pointed out, the use of swap-over-nfs itself creates more allocation pressure if it is used in the reclaim path. The emergency memory pool is used *anyway* unless there are clean file pages that can be discarded. But that's a big "if". The safer path is to try and exit and if *that* fails *then* enter direct reclaim.

Okay. Let's see your code again POV side effect other than OOM deadlock problem.

1. pfmemalloc_watermark_ok == false but the process is received SIGKILL before calling throttle_direct_reclaim.

In this case, it enters direct reclaim path and would swap out anon pages. It's a thing you are concerning now(ie, creates more allocation pressure) Is it okay?

2. pfmemalloc_watermark_ok == false but the process is received SIGKILL while throttling.

In this case, it skips direct reclaim in first path and retry to allocate page. If another procces free some memory or is killed, it can get a free page and return. Yes. it would be good rather than unnecessary swap out and OOM kill. Otherwise, it calls direct compaction again and then enter direct reclaim path. It ends up consuming emergency memory pool to swap out anonymous pages or OOM killed. Again, it's a thing you are concerning now.

So, your patch's effect depends on timing that other process release memory. Is it right?
If it is your intention, I don't oppose it any more because apprantely it has a benefit than I suggested. But please write description more clearly. Below previous description focused only OOM deadlock problem and didn't explain patch's side effect which I mentioned above.

[
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.

If this patch passes review it should be considered a -stable candidate for 3.6.
]

Quoted 4 lines

Signature · 3 lines
-- 
Mel Gorman
SUSE Labs

Signature · 8 lines

Kind Regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-11-21 15:38 UTC #11 lore raw

PATCH 1/1 mm: vmscan: Check for fatal signals iff the process was throttled

This patch moves the fatal_signal_pending() check after throttling to throttle_direct_reclaim() where it belongs. If the process is killed while throttled, it will return immediately without direct reclaim except now it will have TIF_MEMDIE set and will use the PFMEMALLOC reserves.

Minchan pointed out that it may be better to direct reclaim before returning to avoid using the reserves because there may be pages that can easily reclaim that would avoid using the reserves. However, we do no such targetted reclaim and there is no guarantee that suitable pages are available. As it is expected that this throttling happens when swap-over-NFS is used there is a possibility that the process will instead swap which may allocate network buffers from the PFMEMALLOC reserves. Hence, in the swap-over-nfs case where a process can be throtted and be killed it can use the reserves to exit or it can potentially use reserves to swap a few pages and then exit. This patch takes the option of using the reserves if necessary to allow the process exit quickly.

If this patch passes review it should be considered a -stable candidate for 3.6.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/vmscan.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

mm/vmscan.c +27 ~~-10~~

index 48550c6..cbf84e1 100644
@@ -2207,9 +2207,12 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
  * Throttle direct reclaimers if backing storage is backed by the network
  * and the PFMEMALLOC reserve for the preferred node is getting dangerously
  * depleted. kswapd will continue to make progress and wake the processes
- * when the low watermark is reached
+ * when the low watermark is reached.
+ *
+ * Returns true if a fatal signal was delivered during throttling. If this
+ * happens, the page allocator should not consider triggering the OOM killer.
  */
-static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
+static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
 	struct zone *zone;

@@ -2224,13 +2227,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * processes to block on log_wait_commit().
 	 */
 	if (current->flags & PF_KTHREAD)
-		return;
+		goto out;
+
+	/*
+	 * If a fatal signal is pending, this process should not throttle.
+	 * It should return quickly so it can exit and free its memory
+	 */
+	if (fatal_signal_pending(current))
+		goto out;
 
 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
 	pgdat = zone->zone_pgdat;
 	if (pfmemalloc_watermark_ok(pgdat))
-		return;
+		goto out;
 
 	/* Account for the throttling */
 	count_vm_event(PGSCAN_DIRECT_THROTTLE);

@@ -2246,12 +2256,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
 			pfmemalloc_watermark_ok(pgdat), HZ);
-		return;
+
+		goto check_pending;
 	}
 
 	/* Throttle until kswapd wakes the process */
 	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
 		pfmemalloc_watermark_ok(pgdat));
+
+check_pending:
+	if (fatal_signal_pending(current))
+		return true;
+
+out:
+	return false;
 }
 
 unsigned long try_to_free_pages(struct zonelist *zonelist, int order,

@@ -2273,13 +2291,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.gfp_mask = sc.gfp_mask,
 	};
 
-	throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
-
 	/*
-	 * Do not enter reclaim if fatal signal is pending. 1 is returned so
-	 * that the page allocator does not consider triggering OOM
+	 * Do not enter reclaim if fatal signal was delivered while throttled.
+	 * 1 is returned so that the page allocator does not OOM kill at this
+	 * point.
 	 */
-	if (fatal_signal_pending(current))
+	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2012-11-21 20:16 UTC #12 lore raw

Re: [PATCH] mm: vmscan: Check for fatal signals iff the process was throttled

Quoted 37 lines · On Wed, 21 Nov 2012 15:38:24 +0000 Mel Gorman <[email protected]> wrote:

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. If the process is killed while throttled, it will return immediately without direct reclaim except now it will have TIF_MEMDIE set and will use the PFMEMALLOC reserves.
Minchan pointed out that it may be better to direct reclaim before returning to avoid using the reserves because there may be pages that can easily reclaim that would avoid using the reserves. However, we do no such targetted reclaim and there is no guarantee that suitable pages are available. As it is expected that this throttling happens when swap-over-NFS is used there is a possibility that the process will instead swap which may allocate network buffers from the PFMEMALLOC reserves. Hence, in the swap-over-nfs case where a process can be throtted and be killed it can use the reserves to exit or it can potentially use reserves to swap a few pages and then exit. This patch takes the option of using the reserves if necessary to allow the process exit quickly.
If this patch passes review it should be considered a -stable candidate for 3.6.
...

mm/vmscan.c static bool pfmemalloc_watermark_ok(pg_data_t *pgdat) · line 2212

Quoted 7 lines

mm/vmscan.c +3 -1

@@ -2207,9 +2207,12 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
  * Throttle direct reclaimers if backing storage is backed by the network
  * and the PFMEMALLOC reserve for the preferred node is getting dangerously
  * depleted. kswapd will continue to make progress and wake the processes
- * when the low watermark is reached
+ * when the low watermark is reached.
+ *
+ * Returns true if a fatal signal was delivered during throttling. If this

s/delivered/received/imo

mm/vmscan.c static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2230

Quoted 13 lines

mm/vmscan.c +3 -2

+ * happens, the page allocator should not consider triggering the OOM killer.
  */
-static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
+static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
 	struct zone *zone;

@@ -2224,13 +2227,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * processes to block on log_wait_commit().
 	 */
 	if (current->flags & PF_KTHREAD)
-		return;
+		goto out;

hm, well, back in the old days some kernel threads were killable via signals. They had to opt-in to it by diddling their signal masks and a few other things. Too lazy to check if there are still any such sites.

mm/vmscan.c static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2236

Quoted 6 lines

mm/vmscan.c +6 -0

+	/*
+	 * If a fatal signal is pending, this process should not throttle.
+	 * It should return quickly so it can exit and free its memory
+	 */
+	if (fatal_signal_pending(current))
+		goto out;

theresabug. It should return "true" here.

Quoted 11 lines

mm/vmscan.c +1 -1

 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
 	pgdat = zone->zone_pgdat;
 	if (pfmemalloc_watermark_ok(pgdat))
-		return;
+		goto out;
 
 	/* Account for the throttling */
 	count_vm_event(PGSCAN_DIRECT_THROTTLE);

mm/vmscan.c static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2260

Quoted 6 lines

mm/vmscan.c +2 -1

@@ -2246,12 +2256,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
 			pfmemalloc_watermark_ok(pgdat), HZ);
-		return;
+
+		goto check_pending;

And this can be just an "else".

Quoted 16 lines

mm/vmscan.c +7 -0

 	}
 
 	/* Throttle until kswapd wakes the process */
 	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
 		pfmemalloc_watermark_ok(pgdat));
+
+check_pending:
+	if (fatal_signal_pending(current))
+		return true;
+
+out:
+	return false;
 }
 
 unsigned long try_to_free_pages(struct zonelist *zonelist, int order,

mm/vmscan.c unsigned long try_to_free_pages(struct zonelist *zonelist, int order, · line 2295

Quoted 9 lines

mm/vmscan.c +1 -4

@@ -2273,13 +2291,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.gfp_mask = sc.gfp_mask,
 	};
 
-	throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
-
 	/*
-	 * Do not enter reclaim if fatal signal is pending. 1 is returned so
-	 * that the page allocator does not consider triggering OOM
+	 * Do not enter reclaim if fatal signal was delivered while throttled.

Again, "received" is clearer.

mm/vmscan.c unsigned long try_to_free_pages(struct zonelist *zonelist, int order, · line 2302

Quoted 8 lines

mm/vmscan.c +3 -1

+	 * 1 is returned so that the page allocator does not OOM kill at this
+	 * point.
 	 */
-	if (fatal_signal_pending(current))
+	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,

So I end up with the below patch, which yields

static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
nodemask_t *nodemask)

{
	struct zone *zone;
	int high_zoneidx = gfp_zone(gfp_mask);
	pg_data_t *pgdat;

	/*
	 * Kernel threads should not be throttled as they may be indirectly
	 * responsible for cleaning pages necessary for reclaim to make forward
	 * progress. kjournald for example may enter direct reclaim while
	 * committing a transaction where throttling it could force other
	 * processes to block on log_wait_commit().
	 */
	if (current->flags & PF_KTHREAD)
		goto out;

	/*
	 * If a fatal signal is pending, this process should not throttle.
	 * It should return quickly so it can exit and free its memory
	 */
	if (fatal_signal_pending(current))
		goto killed;

	/* Check if the pfmemalloc reserves are ok */
	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
	pgdat = zone->zone_pgdat;
	if (pfmemalloc_watermark_ok(pgdat))
		goto out;

	/* Account for the throttling */
	count_vm_event(PGSCAN_DIRECT_THROTTLE);

	/*
	 * If the caller cannot enter the filesystem, it's possible that it
	 * is due to the caller holding an FS lock or performing a journal
	 * transaction in the case of a filesystem like ext[3|4]. In this case,
	 * it is not safe to block on pfmemalloc_wait as kswapd could be
	 * blocked waiting on the same lock. Instead, throttle for up to a
	 * second before continuing.
	 */
	if (!(gfp_mask & __GFP_FS)) {
		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
					pfmemalloc_watermark_ok(pgdat), HZ);
	} else {
		/* Throttle until kswapd wakes the process */
		wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
				    pfmemalloc_watermark_ok(pgdat));
	}

	if (fatal_signal_pending(current)) {
killed:
		return true;
	}

out:

	return false;
}

(I hate that "goto killed" thing, but can't think of a better way)

mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-check-for-fatal-signals-iff-the-process-was-throttled-fix +12 ~~-13~~

@@ -2209,7 +2209,7 @@ static bool pfmemalloc_watermark_ok(pg_d
  * depleted. kswapd will continue to make progress and wake the processes
  * when the low watermark is reached.
  *
- * Returns true if a fatal signal was delivered during throttling. If this
+ * Returns true if a fatal signal was received during throttling.  If this
  * happens, the page allocator should not consider triggering the OOM killer.
  */
 static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,

@@ -2223,7 +2223,7 @@ static bool throttle_direct_reclaim(gfp_
 	 * Kernel threads should not be throttled as they may be indirectly
 	 * responsible for cleaning pages necessary for reclaim to make forward
 	 * progress. kjournald for example may enter direct reclaim while
-	 * committing a transaction where throttling it could forcing other
+	 * committing a transaction where throttling it could force other
 	 * processes to block on log_wait_commit().
 	 */
 	if (current->flags & PF_KTHREAD)

@@ -2234,7 +2234,7 @@ static bool throttle_direct_reclaim(gfp_
 	 * It should return quickly so it can exit and free its memory
 	 */
 	if (fatal_signal_pending(current))
-		goto out;
+		goto killed;
 
 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);

@@ -2255,18 +2255,17 @@ static bool throttle_direct_reclaim(gfp_
 	 */
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
-			pfmemalloc_watermark_ok(pgdat), HZ);
-
-		goto check_pending;
+					pfmemalloc_watermark_ok(pgdat), HZ);
+	} else {
+		/* Throttle until kswapd wakes the process */
+		wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
+				    pfmemalloc_watermark_ok(pgdat));
 	}
 
-	/* Throttle until kswapd wakes the process */
-	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
-		pfmemalloc_watermark_ok(pgdat));
-
-check_pending:
-	if (fatal_signal_pending(current))
+	if (fatal_signal_pending(current)) {
+killed:
 		return true;
+	}
 
 out:
 	return false;

@@ -2292,7 +2291,7 @@ unsigned long try_to_free_pages(struct z
 	};
 
 	/*
-	 * Do not enter reclaim if fatal signal was delivered while throttled.
+	 * Do not enter reclaim if a fatal signal was received while throttled.
 	 * 1 is returned so that the page allocator does not OOM kill at this
 	 * point.
 	 */

(Still hating that "goto killed")

(relents)

How about this version?

static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
nodemask_t *nodemask)

{
	struct zone *zone;
	int high_zoneidx = gfp_zone(gfp_mask);
	pg_data_t *pgdat;

	/*
	 * Kernel threads should not be throttled as they may be indirectly
	 * responsible for cleaning pages necessary for reclaim to make forward
	 * progress. kjournald for example may enter direct reclaim while
	 * committing a transaction where throttling it could force other
	 * processes to block on log_wait_commit().
	 */
	if (current->flags & PF_KTHREAD)
		return false;

	/*
	 * If a fatal signal is pending, this process should not throttle.
	 * It should return quickly so it can exit and free its memory
	 */
	if (fatal_signal_pending(current))
		return true;

	/* Check if the pfmemalloc reserves are ok */
	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
	pgdat = zone->zone_pgdat;
	if (pfmemalloc_watermark_ok(pgdat))
		return false;

	/* Account for the throttling */
	count_vm_event(PGSCAN_DIRECT_THROTTLE);

	/*
	 * If the caller cannot enter the filesystem, it's possible that it
	 * is due to the caller holding an FS lock or performing a journal
	 * transaction in the case of a filesystem like ext[3|4]. In this case,
	 * it is not safe to block on pfmemalloc_wait as kswapd could be
	 * blocked waiting on the same lock. Instead, throttle for up to a
	 * second before continuing.
	 */
	if (!(gfp_mask & __GFP_FS)) {
		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
					pfmemalloc_watermark_ok(pgdat), HZ);
	} else {
		/* Throttle until kswapd wakes the process */
		wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
				    pfmemalloc_watermark_ok(pgdat));
	}

	return fatal_signal_pending(current);
}

Mel Gorman

2012-11-21 21:05 UTC #13 lore raw

Re: [PATCH] mm: vmscan: Check for fatal signals iff the process was throttled

Quoted 37 lines · Andrew Morton

Quoted 34 lines · On Wed, 21 Nov 2012 15:38:24 +0000 Mel Gorman <[email protected]> wrote:
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. If the process is killed while throttled, it will return immediately without direct reclaim except now it will have TIF_MEMDIE set and will use the PFMEMALLOC reserves.
Minchan pointed out that it may be better to direct reclaim before returning to avoid using the reserves because there may be pages that can easily reclaim that would avoid using the reserves. However, we do no such targetted reclaim and there is no guarantee that suitable pages are available. As it is expected that this throttling happens when swap-over-NFS is used there is a possibility that the process will instead swap which may allocate network buffers from the PFMEMALLOC reserves. Hence, in the swap-over-nfs case where a process can be throtted and be killed it can use the reserves to exit or it can potentially use reserves to swap a few pages and then exit. This patch takes the option of using the reserves if necessary to allow the process exit quickly.
If this patch passes review it should be considered a -stable candidate for 3.6.
...

Quoted 13 lines

mm/vmscan.c static bool pfmemalloc_watermark_ok(pg_data_t *pgdat) · line 2212

Quoted 10 lines

mm/vmscan.c +3 -1

@@ -2207,9 +2207,12 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
  * Throttle direct reclaimers if backing storage is backed by the network
  * and the PFMEMALLOC reserve for the preferred node is getting dangerously
  * depleted. kswapd will continue to make progress and wake the processes
- * when the low watermark is reached
+ * when the low watermark is reached.
+ *
+ * Returns true if a fatal signal was delivered during throttling. If this

s/delivered/received/imo

Ok.

Quoted 9 lines

mm/vmscan.c +2 -1

+ * happens, the page allocator should not consider triggering the OOM killer.
  */
-static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
+static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
 	struct zone *zone;

@@ -2224,13 +2227,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * processes to block on log_wait_commit().

Quoted 9 lines

mm/vmscan.c static bool throttle_direct_reclaim(gfp_
Quoted 4 lines
mm/vmscan.c +1 -1
 	 */
 	if (current->flags & PF_KTHREAD)
-		return;
+		goto out;
hm, well, back in the old days some kernel threads were killable via signals. They had to opt-in to it by diddling their signal masks and a few other things. Too lazy to check if there are still any such sites.

That check is against throttling rather than signal handling though. It could have been just left as "return".

Quoted 10 lines

mm/vmscan.c static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2236
Quoted 7 lines
mm/vmscan.c +6 -0
+	/*
+	 * If a fatal signal is pending, this process should not throttle.
+	 * It should return quickly so it can exit and free its memory
+	 */
+	if (fatal_signal_pending(current))
+		goto out;
theresabug. It should return "true" here.

The intention here is that a process would

1. allocate, fail, enter direct reclaim
2. no signal pending, gets throttled because of low pfmemalloc reserves
3. a user kills -9 the throttled process. returns true and goes back to the page allocator
4. If that allocation fails again, it re-enters direct reclaim and tries to throttle. This time the fatal signal is pending but we know we must have already failed to make the allocation so this time false is rurned by throttle_direct_reclaim and it tries direct reclaim.
5. direct reclaim frees something -- probably clean file-backed pages if the last allocation attempt had failed.

so the fatal signal check should only prevent entering direct reclaim once. Maybe the comment sucks

/*
* If a fatal signal is pending, this process should not throttle.
* It should return quickly so it can exit and free its memory. Note
* that returning false here allows a process to enter direct reclaim.
* Otherwise there is a risk that the process loops in the page
* allocator, checking signals and never making forward progress */

Quoted 8 lines

mm/vmscan.c +1 -1

 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
 	pgdat = zone->zone_pgdat;
 	if (pfmemalloc_watermark_ok(pgdat))
-		return;
+		goto out;

Quoted 12 lines

mm/vmscan.c static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2260

Quoted 9 lines

mm/vmscan.c +2 -1

 	/* Account for the throttling */
 	count_vm_event(PGSCAN_DIRECT_THROTTLE);

@@ -2246,12 +2256,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
 			pfmemalloc_watermark_ok(pgdat), HZ);
-		return;
+
+		goto check_pending;

And this can be just an "else".

ok.

Quoted 19 lines

mm/vmscan.c +7 -0

 	}
 
 	/* Throttle until kswapd wakes the process */
 	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
 		pfmemalloc_watermark_ok(pgdat));
+
+check_pending:
+	if (fatal_signal_pending(current))
+		return true;
+
+out:
+	return false;
 }
 
 unsigned long try_to_free_pages(struct zonelist *zonelist, int order,

@@ -2273,13 +2291,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.gfp_mask = sc.gfp_mask,
 	};

Quoted 9 lines

mm/vmscan.c unsigned long try_to_free_pages(struct zonelist *zonelist, int order, · line 2295
Quoted 6 lines
mm/vmscan.c +1 -4
-	throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
-
 	/*
-	 * Do not enter reclaim if fatal signal is pending. 1 is returned so
-	 * that the page allocator does not consider triggering OOM
+	 * Do not enter reclaim if fatal signal was delivered while throttled.
Again, "received" is clearer.

It is.

Quoted 26 lines

mm/vmscan.c unsigned long try_to_free_pages(struct zonelist *zonelist, int order, · line 2302

Quoted 8 lines

mm/vmscan.c +3 -1

+	 * 1 is returned so that the page allocator does not OOM kill at this
+	 * point.
 	 */
-	if (fatal_signal_pending(current))
+	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,

So I end up with the below patch, which yields

static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
nodemask_t *nodemask)

{
	struct zone *zone;
	int high_zoneidx = gfp_zone(gfp_mask);
	pg_data_t *pgdat;

	/*
	 * Kernel threads should not be throttled as they may be indirectly
	 * responsible for cleaning pages necessary for reclaim to make forward
	 * progress. kjournald for example may enter direct reclaim while
	 * committing a transaction where throttling it could force other
	 * processes to block on log_wait_commit().
	 */
	if (current->flags & PF_KTHREAD)

Quoted 9 lines

		goto out;

	/*
	 * If a fatal signal is pending, this process should not throttle.
	 * It should return quickly so it can exit and free its memory
	 */
	if (fatal_signal_pending(current))
		goto killed;

I think there is a risk that this allows the page allocator to loop in direct reclaim, always returning true here and never actually making forward progress. Did I miss something?

Quoted 126 lines

	/* Check if the pfmemalloc reserves are ok */
	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
	pgdat = zone->zone_pgdat;
	if (pfmemalloc_watermark_ok(pgdat))
		goto out;

	/* Account for the throttling */
	count_vm_event(PGSCAN_DIRECT_THROTTLE);

	/*
	 * If the caller cannot enter the filesystem, it's possible that it
	 * is due to the caller holding an FS lock or performing a journal
	 * transaction in the case of a filesystem like ext[3|4]. In this case,
	 * it is not safe to block on pfmemalloc_wait as kswapd could be
	 * blocked waiting on the same lock. Instead, throttle for up to a
	 * second before continuing.
	 */
	if (!(gfp_mask & __GFP_FS)) {
		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
					pfmemalloc_watermark_ok(pgdat), HZ);
	} else {
		/* Throttle until kswapd wakes the process */
		wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
				    pfmemalloc_watermark_ok(pgdat));
	}

	if (fatal_signal_pending(current)) {
killed:
		return true;
	}

out:

	return false;
}

(I hate that "goto killed" thing, but can't think of a better way)

mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-check-for-fatal-signals-iff-the-process-was-throttled-fix +12 ~~-13~~

@@ -2209,7 +2209,7 @@ static bool pfmemalloc_watermark_ok(pg_d
  * depleted. kswapd will continue to make progress and wake the processes
  * when the low watermark is reached.
  *
- * Returns true if a fatal signal was delivered during throttling. If this
+ * Returns true if a fatal signal was received during throttling.  If this
  * happens, the page allocator should not consider triggering the OOM killer.
  */
 static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,

@@ -2223,7 +2223,7 @@ static bool throttle_direct_reclaim(gfp_
 	 * Kernel threads should not be throttled as they may be indirectly
 	 * responsible for cleaning pages necessary for reclaim to make forward
 	 * progress. kjournald for example may enter direct reclaim while
-	 * committing a transaction where throttling it could forcing other
+	 * committing a transaction where throttling it could force other
 	 * processes to block on log_wait_commit().
 	 */
 	if (current->flags & PF_KTHREAD)

@@ -2234,7 +2234,7 @@ static bool throttle_direct_reclaim(gfp_
 	 * It should return quickly so it can exit and free its memory
 	 */
 	if (fatal_signal_pending(current))
-		goto out;
+		goto killed;
 
 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);

@@ -2255,18 +2255,17 @@ static bool throttle_direct_reclaim(gfp_
 	 */
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
-			pfmemalloc_watermark_ok(pgdat), HZ);
-
-		goto check_pending;
+					pfmemalloc_watermark_ok(pgdat), HZ);
+	} else {
+		/* Throttle until kswapd wakes the process */
+		wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
+				    pfmemalloc_watermark_ok(pgdat));
 	}
 
-	/* Throttle until kswapd wakes the process */
-	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
-		pfmemalloc_watermark_ok(pgdat));
-
-check_pending:
-	if (fatal_signal_pending(current))
+	if (fatal_signal_pending(current)) {
+killed:
 		return true;
+	}
 
 out:
 	return false;

@@ -2292,7 +2291,7 @@ unsigned long try_to_free_pages(struct z
 	};
 
 	/*
-	 * Do not enter reclaim if fatal signal was delivered while throttled.
+	 * Do not enter reclaim if a fatal signal was received while throttled.
 	 * 1 is returned so that the page allocator does not OOM kill at this
 	 * point.
 	 */

(Still hating that "goto killed")

(relents)

How about this version?

static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
nodemask_t *nodemask)

{
	struct zone *zone;
	int high_zoneidx = gfp_zone(gfp_mask);
	pg_data_t *pgdat;

	/*
	 * Kernel threads should not be throttled as they may be indirectly
	 * responsible for cleaning pages necessary for reclaim to make forward
	 * progress. kjournald for example may enter direct reclaim while
	 * committing a transaction where throttling it could force other
	 * processes to block on log_wait_commit().
	 */
	if (current->flags & PF_KTHREAD)

Quoted 9 lines

		return false;

	/*
	 * If a fatal signal is pending, this process should not throttle.
	 * It should return quickly so it can exit and free its memory
	 */
	if (fatal_signal_pending(current))
		return true;

Same comment about the potential looping. Otherwise I think it's ok.

Quoted 29 lines

	/* Check if the pfmemalloc reserves are ok */
	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
	pgdat = zone->zone_pgdat;
	if (pfmemalloc_watermark_ok(pgdat))
		return false;

	/* Account for the throttling */
	count_vm_event(PGSCAN_DIRECT_THROTTLE);

	/*
	 * If the caller cannot enter the filesystem, it's possible that it
	 * is due to the caller holding an FS lock or performing a journal
	 * transaction in the case of a filesystem like ext[3|4]. In this case,
	 * it is not safe to block on pfmemalloc_wait as kswapd could be
	 * blocked waiting on the same lock. Instead, throttle for up to a
	 * second before continuing.
	 */
	if (!(gfp_mask & __GFP_FS)) {
		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
					pfmemalloc_watermark_ok(pgdat), HZ);
	} else {
		/* Throttle until kswapd wakes the process */
		wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
				    pfmemalloc_watermark_ok(pgdat));
	}

	return fatal_signal_pending(current);
}

Signature · 8 lines

Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2012-11-21 21:30 UTC #14 lore raw

Re: [PATCH] mm: vmscan: Check for fatal signals iff the process was throttled

Quoted 21 lines · On Wed, 21 Nov 2012 21:05:20 +0000 Mel Gorman <[email protected]> wrote:

On Wed, Nov 21, 2012 at 12:15:59PM -0800, Andrew Morton wrote:
...
Quoted 7 lines
Quoted 7 lines
mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-check-for-fatal-signals-iff-the-process-was-throttled-fix +1 -1
-static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
+static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
 	struct zone *zone;
@@ -2224,13 +2227,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * processes to block on log_wait_commit().
Quoted 9 lines
mm/vmscan.c static bool throttle_direct_reclaim(gfp_
Quoted 4 lines
mm/vmscan.c renamed from mm/vmscan.c~mm-vmscan-check-for-fatal-signals-iff-the-process-was-throttled-fix +1 -1
 	 */
 	if (current->flags & PF_KTHREAD)
-		return;
+		goto out;
hm, well, back in the old days some kernel threads were killable via signals. They had to opt-in to it by diddling their signal masks and a few other things. Too lazy to check if there are still any such sites.

Quoted 2 lines

That check is against throttling rather than signal handling though. It could have been just left as "return".

My point is that there might still exist kernel threads which are killable via signals. Those threads match your criteria here: don't throttle - just let them run to exit().

If there are indeed missed opportunities here then they will be small ones. And those threads probably only have signal_pending(), not fatal_signal_pending(). Don't worry about it ;)

Quoted 13 lines

Quoted 10 lines
Review static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist, · line 2236
Quoted 7 lines
(quoted diff) +6 -0
+	/*
+	 * If a fatal signal is pending, this process should not throttle.
+	 * It should return quickly so it can exit and free its memory
+	 */
+	if (fatal_signal_pending(current))
+		goto out;
theresabug. It should return "true" here.
The intention here is that a process would

Quoted 8 lines

1. allocate, fail, enter direct reclaim
2. no signal pending, gets throttled because of low pfmemalloc reserves
3. a user kills -9 the throttled process. returns true and goes back to the page allocator
4. If that allocation fails again, it re-enters direct reclaim and tries to throttle. This time the fatal signal is pending but we know we must have already failed to make the allocation so this time false is rurned by throttle_direct_reclaim and it tries direct reclaim.

My spinning head fell on the floor and is now drilling its way to China.

Quoted 5 lines

5. direct reclaim frees something -- probably clean file-backed pages if the last allocation attempt had failed.
so the fatal signal check should only prevent entering direct reclaim once. Maybe the comment sucks

Well it did say "Returns true if a fatal signal was received during throttling.". That "during" was subtle.

Quoted 9 lines

/*
* If a fatal signal is pending, this process should not throttle.
* It should return quickly so it can exit and free its memory. Note
* that returning false here allows a process to enter direct reclaim.
* Otherwise there is a risk that the process loops in the page
* allocator, checking signals and never making forward progress */
?

It's still unclear why throttle_direct_reclaim() returns false if fatal_signal_pending() *before* throttling, but true *after* throttling. Why not always return true and just scram?

Quoted 4 lines

...
Same comment about the potential looping. Otherwise I think it's ok.

Send me something sometime ;)

Minchan Kim

author Acked-by on v1 1/1

2012-11-23 05:08 UTC #15 lore raw

Re: [PATCH] mm: vmscan: Check for fatal signals iff the process was throttled

In reply to Mel Gorman

Quoted 17 lines · Mel Gorman

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. If the process is killed while throttled, it will return immediately without direct reclaim except now it will have TIF_MEMDIE set and will use the PFMEMALLOC reserves.

Quoted 4 lines

Minchan pointed out that it may be better to direct reclaim before returning to avoid using the reserves because there may be pages that can easily reclaim that would avoid using the reserves. However, we do no such targetted reclaim and there is no guarantee that suitable pages are available. As it

I think we could mimic the target reclaim by checking the number of (NR_FILE_PAGES - NR_SHMEM) and sc.may_swap = false but I am not strong now. If some problem happens by this, we could consider this.
Now, just want to remain history in case of forgetting.

Quoted 8 lines

is expected that this throttling happens when swap-over-NFS is used there is a possibility that the process will instead swap which may allocate network buffers from the PFMEMALLOC reserves. Hence, in the swap-over-nfs case where a process can be throtted and be killed it can use the reserves to exit or it can potentially use reserves to swap a few pages and then exit. This patch takes the option of using the reserves if necessary to allow the process exit quickly.

Quoted 4 lines

If this patch passes review it should be considered a -stable candidate for 3.6.
Signed-off-by Mel Gorman <[email protected]>

Acked-by Minchan Kim <[email protected]>

Thanks, Mel.

Quoted 92 lines

---
 mm/vmscan.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

mm/vmscan.c +27 ~~-11~~

index 48550c6..cbf84e1 100644
@@ -2207,9 +2207,12 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
  * Throttle direct reclaimers if backing storage is backed by the network
  * and the PFMEMALLOC reserve for the preferred node is getting dangerously
  * depleted. kswapd will continue to make progress and wake the processes
- * when the low watermark is reached
+ * when the low watermark is reached.
+ *
+ * Returns true if a fatal signal was delivered during throttling. If this
+ * happens, the page allocator should not consider triggering the OOM killer.
  */
-static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
+static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 					nodemask_t *nodemask)
 {
 	struct zone *zone;

@@ -2224,13 +2227,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	 * processes to block on log_wait_commit().
 	 */
 	if (current->flags & PF_KTHREAD)
-		return;
+		goto out;
+
+	/*
+	 * If a fatal signal is pending, this process should not throttle.
+	 * It should return quickly so it can exit and free its memory
+	 */
+	if (fatal_signal_pending(current))
+		goto out;
 
 	/* Check if the pfmemalloc reserves are ok */
 	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
 	pgdat = zone->zone_pgdat;
 	if (pfmemalloc_watermark_ok(pgdat))
-		return;
+		goto out;
 
 	/* Account for the throttling */
 	count_vm_event(PGSCAN_DIRECT_THROTTLE);

@@ -2246,12 +2256,20 @@ static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
 	if (!(gfp_mask & __GFP_FS)) {
 		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
 			pfmemalloc_watermark_ok(pgdat), HZ);
-		return;
+
+		goto check_pending;
 	}
 
 	/* Throttle until kswapd wakes the process */
 	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
 		pfmemalloc_watermark_ok(pgdat));
+
+check_pending:
+	if (fatal_signal_pending(current))
+		return true;
+
+out:
+	return false;
 }
 
 unsigned long try_to_free_pages(struct zonelist *zonelist, int order,

@@ -2273,13 +2291,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.gfp_mask = sc.gfp_mask,
 	};
 
-	throttle_direct_reclaim(gfp_mask, zonelist, nodemask);
-
 	/*
-	 * Do not enter reclaim if fatal signal is pending. 1 is returned so
-	 * that the page allocator does not consider triggering OOM
+	 * Do not enter reclaim if fatal signal was delivered while throttled.
+	 * 1 is returned so that the page allocator does not OOM kill at this
+	 * point.
 	 */
-	if (fatal_signal_pending(current))
+	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,
 
--

Signature · 8 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>