[PATCH][V2] Make get_user_pages interruptible

[PATCH][V2] Make get_user_pages interruptible

[Ying Han]

make get_user_pages interruptible
The initial implementation of checking TIF_MEMDIE covers the cases of OOM
killing. If the process has been OOM killed, the TIF_MEMDIE is set and it
return immediately. This patch includes:

1. add the case that the SIGKILL is sent by user processes. The process can
try to get_user_pages() unlimited memory even if a user process has sent a
SIGKILL to it(maybe a monitor find the process exceed its memory limit and
try to kill it). In the old implementation, the SIGKILL won't be handled
until the get_user_pages() returns.

2. change the return value to be ERESTARTSYS. It makes no sense to return
ENOMEM if the get_user_pages returned by getting a SIGKILL signal.
Considering the general convention for a system call interrupted by a
signal is ERESTARTNOSYS, so the current return value is consistant to that.

Signed-off-by:	Paul Menage <menage@google.com>
		      Ying Han <yinghan@google.com>

include/linux/sched.h         |    1 +
kernel/signal.c                   |    2 +-
mm/memory.c                   |    9 +-

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b483f39..f9c6a8a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1790,6 +1790,7 @@ extern void sched_dead(struct task_struct *p);
 extern int in_group_p(gid_t);
 extern int in_egroup_p(gid_t);

+extern int sigkill_pending(struct task_struct *tsk);
 extern void proc_caches_init(void);
 extern void flush_signals(struct task_struct *);
 extern void ignore_signals(struct task_struct *);
diff --git a/kernel/signal.c b/kernel/signal.c
index 105217d..f3f154e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1497,7 +1497,7 @@ static inline int may_ptrace_stop(void)
  * Return nonzero if there is a SIGKILL that should be waking us up.
  * Called with the siglock held.
  */
-static int sigkill_pending(struct task_struct *tsk)
+int sigkill_pending(struct task_struct *tsk)
 {
 	return	sigismember(&tsk->pending.signal, SIGKILL) ||
 		sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
diff --git a/mm/memory.c b/mm/memory.c
index 164951c..5d3db5e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1218,12 +1218,11 @@ int __get_user_pages(struct task_struct *tsk, struct m
 			struct page *page;

 			/*
-			 * If tsk is ooming, cut off its access to large memory
-			 * allocations. It has a pending SIGKILL, but it can't
-			 * be processed until returning to user space.
+			 * If we have a pending SIGKILL, don't keep
+			 * allocating memory.
 			 */
-			if (unlikely(test_tsk_thread_flag(tsk, TIF_MEMDIE)))
-				return i ? i : -ENOMEM;
+			if (sigkill_pending(current))
+				return i ? i : -ERESTARTSYS;

 			if (write)
 				foll_flags |= FOLL_WRITE;

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

Re: [PATCH][V2] Make get_user_pages interruptible

[David Rientjes]

On Fri, 21 Nov 2008, Ying Han wrote:

> Signed-off-by:	Paul Menage <menage@google.com>
> 		      Ying Han <yinghan@google.com>
> 

That should be:

Signed-off-by: Paul Menage <menage@google.com>
Signed-off-by: Ying Han <yinghan@google.com>

and the first signed-off line is usually indicative of who wrote the 
original change.  If Paul wrote this code, please add:

From: Paul Menage <menage@google.com>

as the first line of the email so that the proper authorship gets 
attributed in the commit.

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b483f39..f9c6a8a 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1790,6 +1790,7 @@ extern void sched_dead(struct task_struct *p);
>  extern int in_group_p(gid_t);
>  extern int in_egroup_p(gid_t);
> 
> +extern int sigkill_pending(struct task_struct *tsk);
>  extern void proc_caches_init(void);
>  extern void flush_signals(struct task_struct *);
>  extern void ignore_signals(struct task_struct *);

Interesting way around your email client's line truncation.

> diff --git a/mm/memory.c b/mm/memory.c
> index 164951c..5d3db5e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1218,12 +1218,11 @@ int __get_user_pages(struct task_struct *tsk, struct m
>  			struct page *page;
> 
>  			/*
> -			 * If tsk is ooming, cut off its access to large memory
> -			 * allocations. It has a pending SIGKILL, but it can't
> -			 * be processed until returning to user space.
> +			 * If we have a pending SIGKILL, don't keep
> +			 * allocating memory.
>  			 */
> -			if (unlikely(test_tsk_thread_flag(tsk, TIF_MEMDIE)))
> -				return i ? i : -ENOMEM;
> +			if (sigkill_pending(current))
> +				return i ? i : -ERESTARTSYS;
> 
>  			if (write)
>  				foll_flags |= FOLL_WRITE;
> 

We previously tested tsk for TIF_MEMDIE and not current (in fact, nothing 
in __get_user_pages() operates on current).  So why are we introducing 
this check on current and not tsk?

Do we want to avoid branch prediction now because there's data suggesting 
tsk will be SIGKILL'd more frequently in this path other than by the oom 
killer?

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Ying Han]

On Fri, Nov 21, 2008 at 4:21 PM, David Rientjes <rientjes@google.com> wrote:
> On Fri, 21 Nov 2008, Ying Han wrote:
>
>> Signed-off-by:        Paul Menage <menage@google.com>
>>                     Ying Han <yinghan@google.com>
>>
>
> That should be:
>
> Signed-off-by: Paul Menage <menage@google.com>
> Signed-off-by: Ying Han <yinghan@google.com>
>
> and the first signed-off line is usually indicative of who wrote the
> original change.  If Paul wrote this code, please add:
>
> From: Paul Menage <menage@google.com>
>
> as the first line of the email so that the proper authorship gets
> attributed in the commit.
thanks for your info. fixed.
>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index b483f39..f9c6a8a 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1790,6 +1790,7 @@ extern void sched_dead(struct task_struct *p);
>>  extern int in_group_p(gid_t);
>>  extern int in_egroup_p(gid_t);
>>
>> +extern int sigkill_pending(struct task_struct *tsk);
>>  extern void proc_caches_init(void);
>>  extern void flush_signals(struct task_struct *);
>>  extern void ignore_signals(struct task_struct *);
>
> Interesting way around your email client's line truncation.
>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 164951c..5d3db5e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1218,12 +1218,11 @@ int __get_user_pages(struct task_struct *tsk, struct m
>>                       struct page *page;
>>
>>                       /*
>> -                      * If tsk is ooming, cut off its access to large memory
>> -                      * allocations. It has a pending SIGKILL, but it can't
>> -                      * be processed until returning to user space.
>> +                      * If we have a pending SIGKILL, don't keep
>> +                      * allocating memory.
>>                        */
>> -                     if (unlikely(test_tsk_thread_flag(tsk, TIF_MEMDIE)))
>> -                             return i ? i : -ENOMEM;
>> +                     if (sigkill_pending(current))
>> +                             return i ? i : -ERESTARTSYS;
>>
>>                       if (write)
>>                               foll_flags |= FOLL_WRITE;
>>
>
> We previously tested tsk for TIF_MEMDIE and not current (in fact, nothing
> in __get_user_pages() operates on current).  So why are we introducing
> this check on current and not tsk?
   Initially, the patch is merely to cause a process stuck in mlock to
honour a pending sigkill. And in mlock case, tsk==current.

>
> Do we want to avoid branch prediction now because there's data suggesting
> tsk will be SIGKILL'd more frequently in this path other than by the oom
> killer?
>
any specific example?

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

Re: [PATCH][V2] Make get_user_pages interruptible

[David Rientjes]

On Fri, 21 Nov 2008, Ying Han wrote:

> >> index 164951c..5d3db5e 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -1218,12 +1218,11 @@ int __get_user_pages(struct task_struct *tsk, struct m
> >>                       struct page *page;
> >>
> >>                       /*
> >> -                      * If tsk is ooming, cut off its access to large memory
> >> -                      * allocations. It has a pending SIGKILL, but it can't
> >> -                      * be processed until returning to user space.
> >> +                      * If we have a pending SIGKILL, don't keep
> >> +                      * allocating memory.
> >>                        */
> >> -                     if (unlikely(test_tsk_thread_flag(tsk, TIF_MEMDIE)))
> >> -                             return i ? i : -ENOMEM;
> >> +                     if (sigkill_pending(current))
> >> +                             return i ? i : -ERESTARTSYS;
> >>
> >>                       if (write)
> >>                               foll_flags |= FOLL_WRITE;
> >>
> >
> > We previously tested tsk for TIF_MEMDIE and not current (in fact, nothing
> > in __get_user_pages() operates on current).  So why are we introducing
> > this check on current and not tsk?
>    Initially, the patch is merely to cause a process stuck in mlock to
> honour a pending sigkill. And in mlock case, tsk==current.
> 

It doesn't matter, __get_user_pages() acts on tsk.

> > Do we want to avoid branch prediction now because there's data suggesting
> > tsk will be SIGKILL'd more frequently in this path other than by the oom
> > killer?
> >
> any specific example?
> 

I'm asking why you removed the unlikely() wrapper.

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Paul Menage]

On Fri, Nov 21, 2008 at 4:21 PM, David Rientjes <rientjes@google.com> wrote:
> Signed-off-by: Paul Menage <menage@google.com>
> Signed-off-by: Ying Han <yinghan@google.com>
>
> and the first signed-off line is usually indicative of who wrote the
> original change.  If Paul wrote this code, please add:
>
> From: Paul Menage <menage@google.com>

No, I didn't exactly write it originally - the only thing I added in
our kernel was the use of sigkill_pending() rather than checking for
TIF_MEMDIE.

Paul

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

Re: [PATCH][V2] Make get_user_pages interruptible

[David Rientjes]

On Fri, 21 Nov 2008, Paul Menage wrote:

> No, I didn't exactly write it originally - the only thing I added in
> our kernel was the use of sigkill_pending() rather than checking for
> TIF_MEMDIE.
> 

That's what this patch does, its title just appears to be wrong since it 
was already interruptible.

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Ying Han]

--Ying

On Sat, Nov 22, 2008 at 12:07 PM, David Rientjes <rientjes@google.com> wrote:
> On Fri, 21 Nov 2008, Paul Menage wrote:
>
>> No, I didn't exactly write it originally - the only thing I added in
>> our kernel was the use of sigkill_pending() rather than checking for
>> TIF_MEMDIE.
>>
>
> That's what this patch does, its title just appears to be wrong since it
> was already interruptible.
>

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Hugh Dickins]

On Mon, 24 Nov 2008, Ying Han wrote:
> --Ying
> 
> On Sat, Nov 22, 2008 at 12:07 PM, David Rientjes <rientjes@google.com> wrote:
> > On Fri, 21 Nov 2008, Paul Menage wrote:
> >
> >> No, I didn't exactly write it originally - the only thing I added in
> >> our kernel was the use of sigkill_pending() rather than checking for
> >> TIF_MEMDIE.
> >>
> >
> > That's what this patch does, its title just appears to be wrong since it
> > was already interruptible.
> >
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

The linux-mm list has a tiresome habit of removing one line at the top.

For a year or so I used to wonder why Christoph Lameter sent so many
empty messages in response to patches: at last I realized he was
sending a single-line Acked-by: which linux-mm kindly removed.

I grow tired of it, but forget who to report it to: Rik is sure to know.

Ah, looking at the raw mailbox, I see

...
X-Loop:	owner-majordomo@kvack.org
David:	I made the two fixes and posted another thread as [PATCH][V3]
X-OriginalArrivalTime: 24 Nov 2008 17:39:37.0205 (UTC) FILETIME=[9EEC5A50:01C94E5B]
...

so it looks as if a first line with a colon gets treated as header.

Of course, in your case, it serves you right for top-posting ;)

Hugh

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Andrew Morton]

On Mon, 24 Nov 2008 19:15:16 +0000 (GMT)
Hugh Dickins <hugh@veritas.com> wrote:

> The linux-mm list has a tiresome habit of removing one line at the top.
> 
> For a year or so I used to wonder why Christoph Lameter sent so many
> empty messages in response to patches: at last I realized he was
> sending a single-line Acked-by: which linux-mm kindly removed.
> 
> I grow tired of it, but forget who to report it to: Rik is sure to know.

Benjamin LaHaise <bcrl@kvack.org>

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Hugh Dickins]

Hi Ben,

On Mon, 24 Nov 2008, Hugh Dickins wrote:
> On Mon, 24 Nov 2008, Ying Han wrote:
> > --Ying
> > 
> > On Sat, Nov 22, 2008 at 12:07 PM, David Rientjes <rientjes@google.com> wrote:
> > > On Fri, 21 Nov 2008, Paul Menage wrote:
> > >
> > >> No, I didn't exactly write it originally - the only thing I added in
> > >> our kernel was the use of sigkill_pending() rather than checking for
> > >> TIF_MEMDIE.
> > >>
> > >
> > > That's what this patch does, its title just appears to be wrong since it
> > > was already interruptible.
> > >
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 
> The linux-mm list has a tiresome habit of removing one line at the top.
> 
> For a year or so I used to wonder why Christoph Lameter sent so many
> empty messages in response to patches: at last I realized he was
> sending a single-line Acked-by: which linux-mm kindly removed.
> 
> I grow tired of it, but forget who to report it to: Rik is sure to know.
> 
> Ah, looking at the raw mailbox, I see
> 
> ...
> X-Loop:	owner-majordomo@kvack.org
> David:	I made the two fixes and posted another thread as [PATCH][V3]
> X-OriginalArrivalTime: 24 Nov 2008 17:39:37.0205 (UTC) FILETIME=[9EEC5A50:01C94E5B]
> ...
> 
> so it looks as if a first line with a colon gets treated as header.
> 
> Of course, in your case, it serves you right for top-posting ;)

Thanks to Andrew for reminding me that you're the man for linux-mm:
see from the above, I have a gripe - please, something you could fix?

Thanks a lot,
Hugh

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Benjamin LaHaise]

On Mon, Nov 24, 2008 at 07:38:34PM +0000, Hugh Dickins wrote:
> Hi Ben,
> 
> On Mon, 24 Nov 2008, Hugh Dickins wrote:
> > The linux-mm list has a tiresome habit of removing one line at the top.
> > 
> > For a year or so I used to wonder why Christoph Lameter sent so many
> > empty messages in response to patches: at last I realized he was
> > sending a single-line Acked-by: which linux-mm kindly removed.
> > 
> > I grow tired of it, but forget who to report it to: Rik is sure to know.
> > 
> > Ah, looking at the raw mailbox, I see
> > 
> > ...
> > X-Loop:	owner-majordomo@kvack.org
> > David:	I made the two fixes and posted another thread as [PATCH][V3]
> > X-OriginalArrivalTime: 24 Nov 2008 17:39:37.0205 (UTC) FILETIME=[9EEC5A50:01C94E5B]
> > ...
> > 
> > so it looks as if a first line with a colon gets treated as header.
> > 
> > Of course, in your case, it serves you right for top-posting ;)
> 
> Thanks to Andrew for reminding me that you're the man for linux-mm:
> see from the above, I have a gripe - please, something you could fix?

At least in my own testing, I can't reproduce this behaviour, and I tried 
sending out single test: foo messages with both mutt and mail.  Provide a 
reliable test case and I'll fix it, but at this point I'm inclined to believe 
that people are sending out mangled messages.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Benjamin LaHaise]

I spoke too soon.  At least the message that shows up in the archives of 
what gets sent to majordomo doesn't seem to be missing the carriage return, 
but resending the same message to the test list isn't causing a problem.  
Hrm...

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Randy Dunlap]

Benjamin LaHaise wrote:
> On Mon, Nov 24, 2008 at 07:38:34PM +0000, Hugh Dickins wrote:
>> Hi Ben,
>>
>> On Mon, 24 Nov 2008, Hugh Dickins wrote:
>>> The linux-mm list has a tiresome habit of removing one line at the top.
>>>
>>> For a year or so I used to wonder why Christoph Lameter sent so many
>>> empty messages in response to patches: at last I realized he was
>>> sending a single-line Acked-by: which linux-mm kindly removed.
>>>
>>> I grow tired of it, but forget who to report it to: Rik is sure to know.
>>>
>>> Ah, looking at the raw mailbox, I see
>>>
>>> ...
>>> X-Loop:	owner-majordomo@kvack.org
>>> David:	I made the two fixes and posted another thread as [PATCH][V3]
>>> X-OriginalArrivalTime: 24 Nov 2008 17:39:37.0205 (UTC) FILETIME=[9EEC5A50:01C94E5B]
>>> ...
>>>
>>> so it looks as if a first line with a colon gets treated as header.
>>>
>>> Of course, in your case, it serves you right for top-posting ;)
>> Thanks to Andrew for reminding me that you're the man for linux-mm:
>> see from the above, I have a gripe - please, something you could fix?
> 
> At least in my own testing, I can't reproduce this behaviour, and I tried 
> sending out single test: foo messages with both mutt and mail.  Provide a 
> reliable test case and I'll fix it, but at this point I'm inclined to believe 
> that people are sending out mangled messages.

Subject: [PATCH][V3]Make get_user_pages interruptible

when sent to lkml begins with:

From: Paul Menage <menage@google.com>

but linux-mm shows it beginning with:

make get_user_pages interruptible


[according to my mail files]

~Randy

-- 
~Randy

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Benjamin LaHaise]

It should be fixed now.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

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

Re: [PATCH][V2] Make get_user_pages interruptible

[Hugh Dickins]

Great: thanks a lot, Ben.  (And excuse my top-posting in this case.)

On Mon, 24 Nov 2008, Benjamin LaHaise wrote:
> It should be fixed now.

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