Re: [PATCH v2] mm: vmscan: abort proactive reclaim early when freezing for suspend

[Michal Hocko <mhocko@suse.com>]

[Add Oleg]

On Wed 15-07-26 15:56:49, Richard Chang wrote:
> Hi Michal,
> Thanks for the patch.
> Yes, inserting try_to_freeze() at that point carries low risk.
> However, there are still a few issues:
> 
> 1. As I understand it, fatal_signal_pending() only checks for SIGKILL
> (via sigismember(..., SIGKILL)).
> It does not return true for SIGINT (Ctrl+C), even if the signal is
> unhandled and will eventually terminate the task.
> If we switch to fatal_signal_pending, a large proactive reclaim task
> will continue reclaiming until the loop finishes.

OK, I see you concern now. The code is a bit confusing in this respect.
You are right that fatal_signal_pending returns true only if SIGKILL is
flagged. You need to have a look at the signal delivery path though. If
there is a terminating signal sent and there is a default handler
(sig_fatal) then complete_signal will add SIGKILL to kill the whole
process group.

 
> 2. try_to_freeze() does not clear the TIF_SIGPENDING flag. When the
> task resumes and continues the user_proactive_reclaim() loop, the next
> iteration of the inner loop will immediately abort.

You are right. I forgot that try_to_freeze is not really aimed to be
used for user processes. Rather than trying to enforce explicit
try_to_freeze and dealing with pending signals I believe the right
approach would be to instead simply do the following. 

Oleg you haven't been CCed from the top of the thread but TL;DR is that
we need to freeze a user task performing pro-active memory reclaim that
might take long.  Currently we are calling signal_pending to bail out
but that leaks EINTR which is bad as freezing task has a side effect of
terminating it.

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 35c3bb15ae96..336af76d2a28 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -7909,8 +7909,13 @@ int user_proactive_reclaim(char *buf,
 		unsigned long batch_size = (nr_to_reclaim - nr_reclaimed) / 4;
 		unsigned long reclaimed;
 
+		/*
+		 * Make sure that we enter freezer without delaying much and
+		 * leaking EINTR when that happens but also take care of 
+		 * fatal signals to terminate.
+		 */
 		if (signal_pending(current))
-			return -EINTR;
+			return -ERESTARTSYS;
 
 		/*
 		 * This is the final attempt, drain percpu lru caches in the
-- 
Michal Hocko
SUSE Labs