PATCH: freezer: add fake signal clearing back when thaw task

PATCH: freezer: add fake signal clearing back when thaw task

[Lianwei Wang]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

Hi Tejun Heo and all,

The commit of "34b087e freezer: kill unused
set_freezable_with_signal()" remove recalc_sigpending*() calls in
freezer, so the user tasks get TIF_SIGPENDING fake signal that is set
when freezing userspace process. It left the fake signal to userspcae
which cause the userspace task that wait_event_freezable and friends
return a wrong ERESTARTSYS. This is not good because it waste cpu time
to handle the fake signal.

Can we just call the recalc_sigpending to clear the fake signal for
userspace tasks? as below patch do:

>From 176fccee178bc0185d92853dd2f521c9166b0853 Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Mon, 21 Jan 2013 18:21:26 +0800
Subject: [PATCH] freezer: add fake signal clearing back when thaw task

The fake TIF_SIGPENDING is set during freeze userspace process, but it
is not cleared when thaw tasks after below commit:
  34b087e freezer: kill unused set_freezable_with_signal()

This will cause the userspace task that wait_event_freezable and friends
return a wrong ERESTARTSYS. This is not good because it waste cpu time to
handle the fake signal.

Try to clear the TIF_SIGPENDING flag for userspace apps when wakeup the
frozen task to fix this issue.

Change-Id: I91c90ad2ee9a46c42e3b39a7384ec81e97bc0394
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 kernel/freezer.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index c38893b..09557f6 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -46,6 +46,16 @@ bool freezing_slow_path(struct task_struct *p)
 }
 EXPORT_SYMBOL(freezing_slow_path);

+static void fake_signal_clear(struct task_struct *p)
+{
+ unsigned long flags;
+
+ if (lock_task_sighand(p, &flags)) {
+     recalc_sigpending();
+     unlock_task_sighand(p, &flags);
+ }
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 bool __refrigerator(bool check_kthr_stop)
 {
@@ -74,6 +84,10 @@ bool __refrigerator(bool check_kthr_stop)

  pr_debug("%s left refrigerator\n", current->comm);

+ if (!(current->flags & PF_KTHREAD))
+     if (test_tsk_thread_flag(current, TIF_SIGPENDING))
+         fake_signal_clear(current);
+
  /*
  * Restore saved task state before returning.  The mb'd version
  * needs to be used; otherwise, it might silently break
--
1.7.4.1

[-- Attachment #2: 0001-freezer-add-fake-signal-clearing-back-when-thaw-task.patch --]
[-- Type: application/octet-stream, Size: 1805 bytes --]

From 176fccee178bc0185d92853dd2f521c9166b0853 Mon Sep 17 00:00:00 2001
From: Lianwei Wang <lianwei.wang@gmail.com>
Date: Mon, 21 Jan 2013 18:21:26 +0800
Subject: [PATCH] freezer: add fake signal clearing back when thaw task

The fake TIF_SIGPENDING is set during freeze userspace process, but it
is not cleared when thaw tasks after below commit:
  34b087e freezer: kill unused set_freezable_with_signal()

This will cause the userspace task that wait_event_freezable and friends
return a wrong ERESTARTSYS. This is not good because it waste cpu time to
handle the fake signal.

Try to clear the TIF_SIGPENDING flag for userspace apps when wakeup the
frozen task to fix this issue.

Change-Id: I91c90ad2ee9a46c42e3b39a7384ec81e97bc0394
Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
---
 kernel/freezer.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index c38893b..09557f6 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -46,6 +46,16 @@ bool freezing_slow_path(struct task_struct *p)
 }
 EXPORT_SYMBOL(freezing_slow_path);
 
+static void fake_signal_clear(struct task_struct *p)
+{
+	unsigned long flags;
+
+	if (lock_task_sighand(p, &flags)) {
+		recalc_sigpending();
+		unlock_task_sighand(p, &flags);
+	}
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 bool __refrigerator(bool check_kthr_stop)
 {
@@ -74,6 +84,10 @@ bool __refrigerator(bool check_kthr_stop)
 
 	pr_debug("%s left refrigerator\n", current->comm);
 
+	if (!(current->flags & PF_KTHREAD))
+		if (test_tsk_thread_flag(current, TIF_SIGPENDING))
+			fake_signal_clear(current);
+
 	/*
 	 * Restore saved task state before returning.  The mb'd version
 	 * needs to be used; otherwise, it might silently break
-- 
1.7.4.1

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Tejun Heo]

(cc'ing Rafael and Oleg and quoting whole body)

On Thu, Feb 21, 2013 at 02:19:21PM +0800, Lianwei Wang wrote:
> Hi Tejun Heo and all,
> 
> The commit of "34b087e freezer: kill unused
> set_freezable_with_signal()" remove recalc_sigpending*() calls in
> freezer, so the user tasks get TIF_SIGPENDING fake signal that is set
> when freezing userspace process. It left the fake signal to userspcae
> which cause the userspace task that wait_event_freezable and friends
> return a wrong ERESTARTSYS. This is not good because it waste cpu time
> to handle the fake signal.

Is this even measureable?  Freeze / thaw isn't exactly a hot path and
I'm having difficult time believing -ERESTARTSYS would have a
noticeable impact on anything.  Can you please explain why this is a
problem?

> Can we just call the recalc_sigpending to clear the fake signal for
> userspace tasks? as below patch do:
> 
> From 176fccee178bc0185d92853dd2f521c9166b0853 Mon Sep 17 00:00:00 2001
> From: Lianwei Wang <lianwei.wang@gmail.com>
> Date: Mon, 21 Jan 2013 18:21:26 +0800
> Subject: [PATCH] freezer: add fake signal clearing back when thaw task
> 
> The fake TIF_SIGPENDING is set during freeze userspace process, but it
> is not cleared when thaw tasks after below commit:
>   34b087e freezer: kill unused set_freezable_with_signal()
> 
> This will cause the userspace task that wait_event_freezable and friends
> return a wrong ERESTARTSYS. This is not good because it waste cpu time to
> handle the fake signal.
> 
> Try to clear the TIF_SIGPENDING flag for userspace apps when wakeup the
> frozen task to fix this issue.
> 
> Change-Id: I91c90ad2ee9a46c42e3b39a7384ec81e97bc0394
> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
> ---
>  kernel/freezer.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index c38893b..09557f6 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -46,6 +46,16 @@ bool freezing_slow_path(struct task_struct *p)
>  }
>  EXPORT_SYMBOL(freezing_slow_path);
> 
> +static void fake_signal_clear(struct task_struct *p)
> +{
> + unsigned long flags;
> +
> + if (lock_task_sighand(p, &flags)) {
> +     recalc_sigpending();
> +     unlock_task_sighand(p, &flags);
> + }
> +}
> +
>  /* Refrigerator is place where frozen processes are stored :-). */
>  bool __refrigerator(bool check_kthr_stop)
>  {
> @@ -74,6 +84,10 @@ bool __refrigerator(bool check_kthr_stop)
> 
>   pr_debug("%s left refrigerator\n", current->comm);
> 
> + if (!(current->flags & PF_KTHREAD))
> +     if (test_tsk_thread_flag(current, TIF_SIGPENDING))
> +         fake_signal_clear(current);
> +
>   /*
>   * Restore saved task state before returning.  The mb'd version
>   * needs to be used; otherwise, it might silently break
> --
> 1.7.4.1



-- 
tejun

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Oleg Nesterov]

On 02/25, Tejun Heo wrote:
>
> (cc'ing Rafael and Oleg and quoting whole body)
>
> On Thu, Feb 21, 2013 at 02:19:21PM +0800, Lianwei Wang wrote:
> > Hi Tejun Heo and all,
> >
> > The commit of "34b087e freezer: kill unused
> > set_freezable_with_signal()" remove recalc_sigpending*() calls in
> > freezer, so the user tasks get TIF_SIGPENDING fake signal that is set
> > when freezing userspace process. It left the fake signal to userspcae
> > which cause the userspace task that wait_event_freezable and friends
> > return a wrong ERESTARTSYS. This is not good because it waste cpu time
> > to handle the fake signal.
>
> Is this even measureable?  Freeze / thaw isn't exactly a hot path and
> I'm having difficult time believing -ERESTARTSYS would have a
> noticeable impact on anything.  Can you please explain why this is a
> problem?

For example, wait_for_dump_helpers() can fail because it checks
signal_pending(). And we can sleep in TASK_KILLABLE because pipe_release()
does wake_up_interruptible(). But this is minor, wait_for_dump_helpers()
could be fixed without this change.

The real problem is dump_write-like code. Say, pipe_write() can fail if
signal_pending() == T. I am not saying this is unsolvable, in fact I was
going to add the freeze + recalc_sigpending + retry logic initially, but
this looks soooo ugly.

Also. Rightly or not, but I came to conclusion that this change is right
even if we forget about killable/freezable problems in coredump. The
coredumping thread is no longer a "real" user-space process. It can never
handle the signals, it doesn't return to user-mode, but it does a lot of
work in kernel space. So I think it should look as PF_KTHREAD to freezer.

> > Can we just call the recalc_sigpending to clear the fake signal for
> > userspace tasks? as below patch do:
> >
> > +static void fake_signal_clear(struct task_struct *p)
> > +{
> > + unsigned long flags;
> > +
> > + if (lock_task_sighand(p, &flags)) {
> > +     recalc_sigpending();
> > +     unlock_task_sighand(p, &flags);
> > + }

You know, _perhaps_ we have another reason for this change. Otherwise
wait_event_freezable() doesn't look right. Or we should clarify that
it is only for PF_KTHREAD but than we can simplify wait_event_freezable().
And in this case I do not think we should reintroduce recalc_sigpending()
removed by 34b087e48 "freezer: kill unused set_freezable_with_signal()".

I'll write another email about this, nobody actually need
wait_event_freezable().

But. The change above can't help the coredumping thread. It still
needs to do

	spin_lock_irq(current->siglock);
	if (!__fatal_signal_pending(current))
		clear_thread_flag(TIF_SIGPENDING);
	spin_unlock_irq(current->siglock);

or we should change recalc_sigpending() to check PF_KTHREAD.

Oleg.

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Oleg Nesterov]

Ah, I just noticed that the changelog mentions wait_event_freezable() too,

On 02/25, Tejun Heo wrote:
>
> > Subject: [PATCH] freezer: add fake signal clearing back when thaw task
> >
> > The fake TIF_SIGPENDING is set during freeze userspace process, but it
> > is not cleared when thaw tasks after below commit:
> >   34b087e freezer: kill unused set_freezable_with_signal()
> >
> > This will cause the userspace task that wait_event_freezable

But there is no userspace users?

I guess wait_event_freezable() is only used to avoid the contribution
to load_avg. So we can simplify it, __wait_event_freezable() can simply
do freezable_schedule() or wait_event_freezable() can do
freezer_do_not_count + wait_event_interruptible. Or we are going to add
more users?

As for current users, I think they do not need this interface. Something
like below. I was going to try to convert khugepaged/ksm_scan_thread
yesterday but I was distracted, will try to do tomorrow.

Oleg.

--- x/mm/huge_memory.c
+++ x/mm/huge_memory.c
@@ -2616,7 +2616,7 @@ static int khugepaged_has_work(void)
 static int khugepaged_wait_event(void)
 {
 	return !list_empty(&khugepaged_scan.mm_head) ||
-		kthread_should_stop();
+		kthread_should_stop_or_freeze();
 }
 
 static void khugepaged_do_scan(void)
@@ -2655,20 +2655,18 @@ static void khugepaged_do_scan(void)
 
 static void khugepaged_wait_work(void)
 {
-	try_to_freeze();
-
 	if (khugepaged_has_work()) {
 		if (!khugepaged_scan_sleep_millisecs)
 			return;
 
-		wait_event_freezable_timeout(khugepaged_wait,
-					     kthread_should_stop(),
+		wait_event_interruptible_timeout(khugepaged_wait,
+					     kthread_should_stop_or_freeze(),
 			msecs_to_jiffies(khugepaged_scan_sleep_millisecs));
 		return;
 	}
 
 	if (khugepaged_enabled())
-		wait_event_freezable(khugepaged_wait, khugepaged_wait_event());
+		wait_event_interruptible(khugepaged_wait, khugepaged_wait_event());
 }
 
 static int khugepaged(void *none)
@@ -2678,7 +2676,7 @@ static int khugepaged(void *none)
 	set_freezable();
 	set_user_nice(current, 19);
 
-	while (!kthread_should_stop()) {
+	while (!kthread_freezable_should_stop(NULL)) {
 		khugepaged_do_scan();
 		khugepaged_wait_work();
 	}

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Oleg Nesterov]

Damn.

Tejun, et all, sorry for confusion.

Somehow I misunderstood your email completely! as if you argue with
my "freezer: do not send a fake signal to a PF_DUMPCORE thread"
change http://marc.info/?l=linux-kernel&m=136173112604220

As for "add fake signal clearing back when thaw task", I do not
understand why do we need this change too, so I agree with you.

On 02/26, Oleg Nesterov wrote:
>
> On 02/25, Tejun Heo wrote:
> >
> > (cc'ing Rafael and Oleg and quoting whole body)
> >
> > On Thu, Feb 21, 2013 at 02:19:21PM +0800, Lianwei Wang wrote:
> > > Hi Tejun Heo and all,
> > >
> > > The commit of "34b087e freezer: kill unused
> > > set_freezable_with_signal()" remove recalc_sigpending*() calls in
> > > freezer, so the user tasks get TIF_SIGPENDING fake signal that is set
> > > when freezing userspace process. It left the fake signal to userspcae
> > > which cause the userspace task that wait_event_freezable and friends
> > > return a wrong ERESTARTSYS. This is not good because it waste cpu time
> > > to handle the fake signal.
> >
> > Is this even measureable?  Freeze / thaw isn't exactly a hot path and
> > I'm having difficult time believing -ERESTARTSYS would have a
> > noticeable impact on anything.  Can you please explain why this is a
> > problem?
>
> For example, wait_for_dump_helpers() can fail because it checks
> signal_pending(). And we can sleep in TASK_KILLABLE because pipe_release()
> does wake_up_interruptible(). But this is minor, wait_for_dump_helpers()
> could be fixed without this change.
>
> The real problem is dump_write-like code. Say, pipe_write() can fail if
> signal_pending() == T. I am not saying this is unsolvable, in fact I was
> going to add the freeze + recalc_sigpending + retry logic initially, but
> this looks soooo ugly.
>
> Also. Rightly or not, but I came to conclusion that this change is right
> even if we forget about killable/freezable problems in coredump. The
> coredumping thread is no longer a "real" user-space process. It can never
> handle the signals, it doesn't return to user-mode, but it does a lot of
> work in kernel space. So I think it should look as PF_KTHREAD to freezer.
> 
> > > Can we just call the recalc_sigpending to clear the fake signal for
> > > userspace tasks? as below patch do:
> > >
> > > +static void fake_signal_clear(struct task_struct *p)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + if (lock_task_sighand(p, &flags)) {
> > > +     recalc_sigpending();
> > > +     unlock_task_sighand(p, &flags);
> > > + }
> 
> You know, _perhaps_ we have another reason for this change. Otherwise
> wait_event_freezable() doesn't look right. Or we should clarify that
> it is only for PF_KTHREAD but than we can simplify wait_event_freezable().
> And in this case I do not think we should reintroduce recalc_sigpending()
> removed by 34b087e48 "freezer: kill unused set_freezable_with_signal()".
> 
> I'll write another email about this, nobody actually need
> wait_event_freezable().
> 
> But. The change above can't help the coredumping thread. It still
> needs to do
> 
> 	spin_lock_irq(current->siglock);
> 	if (!__fatal_signal_pending(current))
> 		clear_thread_flag(TIF_SIGPENDING);
> 	spin_unlock_irq(current->siglock);
> 
> or we should change recalc_sigpending() to check PF_KTHREAD.
> 
> Oleg.

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Lianwei Wang]

Sorry for the late response.

Freeze/Thaw is a hot path for the Linux based mobile OS, e.g. Android.
If we don't remove the pending fake signal, then the user space apps
or the related kernel driver has to handle such error. And yes, the
user can handle such case by checking the return value, but it mislead
the user and confuse to them that why wait_event_freezable and friends
return a error on resume path every time? Can we avoid such useless
error return?

(Resend with plain text)

2013/2/26 Tejun Heo <tj@kernel.org>:
> (cc'ing Rafael and Oleg and quoting whole body)
>
> On Thu, Feb 21, 2013 at 02:19:21PM +0800, Lianwei Wang wrote:
>> Hi Tejun Heo and all,
>>
>> The commit of "34b087e freezer: kill unused
>> set_freezable_with_signal()" remove recalc_sigpending*() calls in
>> freezer, so the user tasks get TIF_SIGPENDING fake signal that is set
>> when freezing userspace process. It left the fake signal to userspcae
>> which cause the userspace task that wait_event_freezable and friends
>> return a wrong ERESTARTSYS. This is not good because it waste cpu time
>> to handle the fake signal.
>
> Is this even measureable?  Freeze / thaw isn't exactly a hot path and
> I'm having difficult time believing -ERESTARTSYS would have a
> noticeable impact on anything.  Can you please explain why this is a
> problem?
>
>> Can we just call the recalc_sigpending to clear the fake signal for
>> userspace tasks? as below patch do:
>>
>> From 176fccee178bc0185d92853dd2f521c9166b0853 Mon Sep 17 00:00:00 2001
>> From: Lianwei Wang <lianwei.wang@gmail.com>
>> Date: Mon, 21 Jan 2013 18:21:26 +0800
>> Subject: [PATCH] freezer: add fake signal clearing back when thaw task
>>
>> The fake TIF_SIGPENDING is set during freeze userspace process, but it
>> is not cleared when thaw tasks after below commit:
>>   34b087e freezer: kill unused set_freezable_with_signal()
>>
>> This will cause the userspace task that wait_event_freezable and friends
>> return a wrong ERESTARTSYS. This is not good because it waste cpu time to
>> handle the fake signal.
>>
>> Try to clear the TIF_SIGPENDING flag for userspace apps when wakeup the
>> frozen task to fix this issue.
>>
>> Change-Id: I91c90ad2ee9a46c42e3b39a7384ec81e97bc0394
>> Signed-off-by: Lianwei Wang <lianwei.wang@gmail.com>
>> ---
>>  kernel/freezer.c |   14 ++++++++++++++
>>  1 files changed, 14 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/freezer.c b/kernel/freezer.c
>> index c38893b..09557f6 100644
>> --- a/kernel/freezer.c
>> +++ b/kernel/freezer.c
>> @@ -46,6 +46,16 @@ bool freezing_slow_path(struct task_struct *p)
>>  }
>>  EXPORT_SYMBOL(freezing_slow_path);
>>
>> +static void fake_signal_clear(struct task_struct *p)
>> +{
>> + unsigned long flags;
>> +
>> + if (lock_task_sighand(p, &flags)) {
>> +     recalc_sigpending();
>> +     unlock_task_sighand(p, &flags);
>> + }
>> +}
>> +
>>  /* Refrigerator is place where frozen processes are stored :-). */
>>  bool __refrigerator(bool check_kthr_stop)
>>  {
>> @@ -74,6 +84,10 @@ bool __refrigerator(bool check_kthr_stop)
>>
>>   pr_debug("%s left refrigerator\n", current->comm);
>>
>> + if (!(current->flags & PF_KTHREAD))
>> +     if (test_tsk_thread_flag(current, TIF_SIGPENDING))
>> +         fake_signal_clear(current);
>> +
>>   /*
>>   * Restore saved task state before returning.  The mb'd version
>>   * needs to be used; otherwise, it might silently break
>> --
>> 1.7.4.1
>
>
>
> --
> tejun

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Tejun Heo]

Hello,

On Mon, Mar 04, 2013 at 11:24:06AM +0800, Lianwei Wang wrote:
> Freeze/Thaw is a hot path for the Linux based mobile OS, e.g. Android.
> If we don't remove the pending fake signal, then the user space apps

Hotness is relative.  How often are we talking about?  Do you have any
numbers backing up that this actually makes any difference?  The
reason why I'm not convinced is that by the time the control reaches
fake_signal_clear() the system already has done most of the heavy
lifting and the code path which would be excluded by the extra
checking seems pretty short.  If you can show otherwise, please do so.

> or the related kernel driver has to handle such error. And yes, the
> user can handle such case by checking the return value, but it mislead
> the user and confuse to them that why wait_event_freezable and friends
> return a error on resume path every time? Can we avoid such useless
> error return?

-ERESTARTSYS may happen without freezing and the syscall path has been
capable of handling it for a very long time.  Again, do you have
anything which shows otherwise?

Thanks.

-- 
tejun

Re: PATCH: freezer: add fake signal clearing back when thaw task

[Oleg Nesterov]

On 03/04, Lianwei Wang wrote:
>
> Freeze/Thaw is a hot path for the Linux based mobile OS, e.g. Android.
> If we don't remove the pending fake signal, then the user space apps
> or the related kernel driver has to handle such error.

But if we add recalc_sigpending() we penalize the common case which
doesn't need this.

> And yes, the
> user can handle such case by checking the return value,

Yes.

> but it mislead
> the user and confuse to them that why wait_event_freezable and friends
> return a error on resume path every time? Can we avoid such useless
> error return?

Agreed, it looks strange. It is only for kthreads, I think. And we can
simplify wait_event_freezable() or even kill it.

But if we add new user-space users, I do not know... Fortunately I am
not maintainer ;)

> >> +static void fake_signal_clear(struct task_struct *p)
> >> +{
> >> + unsigned long flags;
> >> +
> >> + if (lock_task_sighand(p, &flags)) {
> >> +     recalc_sigpending();

This looks strange. p is always current, otherwise recalc_sigpending()
can't help.

And since it is current you do not need lock_task_sighand().

Oleg.