[PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Dasgupta, Romit]

Kicks out a frozen thread from the refrigerator when an active thread has
invoked kthread_stop on the frozen thread.

Signed-off-by: Romit Dasgupta <romit@ti.com>
---

diff --git a/kernel/freezer.c b/kernel/freezer.c
index bd1d42b..c28dbe8 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/syscalls.h>
 #include <linux/freezer.h>
+#include <linux/kthread.h>
 
 /*
  * freezing is complete, mark current process as frozen
@@ -49,7 +50,7 @@ void refrigerator(void)
 
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (!frozen(current))
+		if (!frozen(current) || (!current->mm && kthread_should_stop()))
 			break;
 		schedule();
 	}

Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Pavel Machek]

Hi!

> Kicks out a frozen thread from the refrigerator when an active thread has
> invoked kthread_stop on the frozen thread.
> 
> Signed-off-by: Romit Dasgupta <romit@ti.com>
> ---
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index bd1d42b..c28dbe8 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/syscalls.h>
>  #include <linux/freezer.h>
> +#include <linux/kthread.h>
>  
>  /*
>   * freezing is complete, mark current process as frozen
> @@ -49,7 +50,7 @@ void refrigerator(void)
>  
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
> -		if (!frozen(current))
> +		if (!frozen(current) || (!current->mm && kthread_should_stop()))
>  			break;
>  		schedule();

Well, what if the thread does some processing before stopping? That
would break refrigerator assumptions...
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Dasgupta, Romit]

> Subject: Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel
> threads
> 
> Hi!
> 
> > Kicks out a frozen thread from the refrigerator when an active thread has
> > invoked kthread_stop on the frozen thread.
> >
> > Signed-off-by: Romit Dasgupta <romit@ti.com>
> > ---
> >
> > diff --git a/kernel/freezer.c b/kernel/freezer.c
> > index bd1d42b..c28dbe8 100644
> > --- a/kernel/freezer.c
> > +++ b/kernel/freezer.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/module.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/freezer.h>
> > +#include <linux/kthread.h>
> >
> >  /*
> >   * freezing is complete, mark current process as frozen
> > @@ -49,7 +50,7 @@ void refrigerator(void)
> >
> >  	for (;;) {
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > -		if (!frozen(current))
> > +		if (!frozen(current) || (!current->mm && kthread_should_stop()))
> >  			break;
> >  		schedule();
> 
> Well, what if the thread does some processing before stopping? That
> would break refrigerator assumptions...

The suspend thread will block until the 'to be stopped' thread clears up. That is what any call to kthread_stop would boil down to. The target thread would anyway be out of the refrigerator so I am not sure what assumption you mean here. Eventually, the target thread would clear up and wake up the suspend thread and then things would go on as usual.

Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Pavel Machek]

On Sun 2009-11-08 09:52:52, Dasgupta, Romit wrote:
> > Subject: Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel
> > threads
> > 
> > Hi!
> > 
> > > Kicks out a frozen thread from the refrigerator when an active thread has
> > > invoked kthread_stop on the frozen thread.
...
> > > @@ -49,7 +50,7 @@ void refrigerator(void)
> > >
> > >  	for (;;) {
> > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > > -		if (!frozen(current))
> > > +		if (!frozen(current) || (!current->mm && kthread_should_stop()))
> > >  			break;
> > >  		schedule();
> > 
> > Well, what if the thread does some processing before stopping? That
> > would break refrigerator assumptions...
> 
> The suspend thread will block until the 'to be stopped' thread clears up. That is what any call to kthread_stop would boil down to. The target thread would anyway be out of the refrigerator so I am not sure what assumption you mean here. Eventually, the target thread would clear up and wake up the suspend thread and then things would go on as usual.

(Please format to 80 columns).

No, I do not get it.

Lets say we have

evil_data_writer thread that needs to be stopped becuase it writes to
filesystem

nofreeze random_stopper thread

now we create the suspend image, and start writing it out. But that's
okay, evil_data_writer is stopped so it can't do no harm. But now
random_stopper decides to thread_stop() the evil_data_writer, and this
new code allows it to exit the refrigerator, *do some writing*, and
then stop.

That's bad, right?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Dasgupta, Romit]

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Sunday, November 08, 2009 1:57 PM
> To: Dasgupta, Romit
> Cc: Rafael J. Wysocki; linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-pm@lists.linux-foundation.org
> Subject: Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
> 
> On Sun 2009-11-08 09:52:52, Dasgupta, Romit wrote:
> > > Subject: Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel
> > > threads
> > >
> > > Hi!
> > >
> > > > Kicks out a frozen thread from the refrigerator when an active thread has
> > > > invoked kthread_stop on the frozen thread.
> ...
> > > > @@ -49,7 +50,7 @@ void refrigerator(void)
> > > >
> > > >  	for (;;) {
> > > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > > > -		if (!frozen(current))
> > > > +		if (!frozen(current) || (!current->mm &&
> kthread_should_stop()))
> > > >  			break;
> > > >  		schedule();
> > >
> > > Well, what if the thread does some processing before stopping? That
> > > would break refrigerator assumptions...
> >
> > The suspend thread will block until the 'to be stopped' thread clears up. That is
> what any call to kthread_stop would boil down to. The target thread would
> anyway be out of the refrigerator so I am not sure what assumption you mean
> here. Eventually, the target thread would clear up and wake up the suspend
> thread and then things would go on as usual.
> 
> (Please format to 80 columns).
> 
> No, I do not get it.
> 
> Lets say we have
> 
> evil_data_writer thread that needs to be stopped becuase it writes to
> filesystem
> 
> nofreeze random_stopper thread
> 
> now we create the suspend image, and start writing it out. But that's
> okay, evil_data_writer is stopped so it can't do no harm. But now
> random_stopper decides to thread_stop() the evil_data_writer, and this
> new code allows it to exit the refrigerator, *do some writing*, and
> then stop.
> 
> That's bad, right?

evil_data_writer will enter refrigerator after invoking 'try_to_freeze'. This should be followed by a call to kthread_should_stop. There it decides if it needs to exit the thread (after cleanups if necessary) or not. I have seen that the bdi_writeback_task function is like that. It does not care if there is pending data to be written if it detects that someone have invoked a 'kthread_stop' on it. It simply exits. I have seen some other kernel threads that do not follow this and I think that probably is not right. 

RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Dasgupta, Romit]

(Resending with 80 column restriction) 

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz] 
> Sent: Sunday, November 08, 2009 1:57 PM
> To: Dasgupta, Romit
> Cc: Rafael J. Wysocki; linux-omap@vger.kernel.org; 
> linux-kernel@vger.kernel.org; linux-pm@lists.linux-foundation.org
> Subject: Re: [PATCH 1/1] PM: Thaws refrigerated and to be 
> exited kernel threads
> 
> On Sun 2009-11-08 09:52:52, Dasgupta, Romit wrote:
> > > Subject: Re: [PATCH 1/1] PM: Thaws refrigerated and to be 
> exited kernel
> > > threads
> > > 
> > > Hi!
> > > 
> > > > Kicks out a frozen thread from the refrigerator when an 
> active thread has
> > > > invoked kthread_stop on the frozen thread.
> ...
> > > > @@ -49,7 +50,7 @@ void refrigerator(void)
> > > >
> > > >  	for (;;) {
> > > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > > > -		if (!frozen(current))
> > > > +		if (!frozen(current) || (!current->mm 
> && kthread_should_stop()))
> > > >  			break;
> > > >  		schedule();
> > > 
> > > Well, what if the thread does some processing before 
> stopping? That
> > > would break refrigerator assumptions...
> > 
> > The suspend thread will block until the 'to be stopped' 
> thread clears up. That is what any call to kthread_stop would 
> boil down to. The target thread would anyway be out of the 
> refrigerator so I am not sure what assumption you mean here. 
> Eventually, the target thread would clear up and wake up the 
> suspend thread and then things would go on as usual.
> 
> (Please format to 80 columns).
> 
> No, I do not get it.
> 
> Lets say we have
> 
> evil_data_writer thread that needs to be stopped becuase it writes to
> filesystem
> 
> nofreeze random_stopper thread
> 
> now we create the suspend image, and start writing it out. But that's
> okay, evil_data_writer is stopped so it can't do no harm. But now
> random_stopper decides to thread_stop() the evil_data_writer, and this
> new code allows it to exit the refrigerator, *do some writing*, and
> then stop.
> 
> That's bad, right?
evil_data_writer will enter refrigerator after invoking 'try_to_freeze'. This 
should be followed by a call to kthread_should_stop. There it decides if it 
needs to exit the thread (after cleanups if necessary) or not. I have seen that
the bdi_writeback_task function is like that. It does not care if there is 
pending data to be written if it detects that someone have invoked a 
'kthread_stop' on it. It simply exits. I have seen some other kernel threads 
that do not follow this and I think that probably is not right. 

Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Pavel Machek]

On Sun 2009-11-08 16:40:07, Dasgupta, Romit wrote:
> (Resending with 80 column restriction) 

Thanks.

> > > > > @@ -49,7 +50,7 @@ void refrigerator(void)
> > > > >
> > > > >  	for (;;) {
> > > > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > > > > -		if (!frozen(current))
> > > > > +		if (!frozen(current) || (!current->mm 
> > && kthread_should_stop()))
> > > > >  			break;
> > > > >  		schedule();
> > > > 
> > > > Well, what if the thread does some processing before 
> > stopping? That
> > > > would break refrigerator assumptions...
> > > 
> > > The suspend thread will block until the 'to be stopped' 
> > thread clears up. That is what any call to kthread_stop would 
> > boil down to. The target thread would anyway be out of the 
> > refrigerator so I am not sure what assumption you mean here. 
> > Eventually, the target thread would clear up and wake up the 
> > suspend thread and then things would go on as usual.
> > 
> > (Please format to 80 columns).
> > 
> > No, I do not get it.
> > 
> > Lets say we have
> > 
> > evil_data_writer thread that needs to be stopped becuase it writes to
> > filesystem
> > 
> > nofreeze random_stopper thread
> > 
> > now we create the suspend image, and start writing it out. But that's
> > okay, evil_data_writer is stopped so it can't do no harm. But now
> > random_stopper decides to thread_stop() the evil_data_writer, and this
> > new code allows it to exit the refrigerator, *do some writing*, and
> > then stop.
> > 
> > That's bad, right?
> evil_data_writer will enter refrigerator after invoking 'try_to_freeze'. This 
> should be followed by a call to kthread_should_stop. There it decides if it 

Really? I believe the "ktrhead_should_stop" is new rule, and code does
not seem to follow it. Actually, for example audit does not seem to
use kthread_should_stop() at all...

./kernel/rtmutex-tester.c-
./kernel/rtmutex-tester.c-		/* Wait for the next command to be executed */
./kernel/rtmutex-tester.c-		schedule();
./kernel/rtmutex-tester.c:		try_to_freeze();
./kernel/rtmutex-tester.c-
./kernel/rtmutex-tester.c-		if (signal_pending(current))
./kernel/rtmutex-tester.c-			flush_signals(current);
--
./kernel/slow-work.c-			schedule();
./kernel/slow-work.c-		finish_wait(&slow_work_thread_wq, &wait);
./kernel/slow-work.c-
./kernel/slow-work.c:		try_to_freeze();
./kernel/slow-work.c-
./kernel/slow-work.c-		vsmax = vslow_work_proportion;
./kernel/slow-work.c-		vsmax *= atomic_read(&slow_work_thread_count);
--
./kernel/audit.c-			add_wait_queue(&kauditd_wait, &wait);
./kernel/audit.c-
./kernel/audit.c-			if (!skb_queue_len(&audit_skb_queue)) {
./kernel/audit.c:				try_to_freeze();
./kernel/audit.c-				schedule();
./kernel/audit.c-			}
./kernel/audit.c-
--
./kernel/signal.c-	/* Now we don't run again until woken by SIGCONT or SIGKILL */
./kernel/signal.c-	do {
./kernel/signal.c-		schedule();
./kernel/signal.c:	} while (try_to_freeze());
./kernel/signal.c-
./kernel/signal.c-	tracehook_finish_jctl();
./kernel/signal.c-	current->exit_code = 0;
--
./kernel/signal.c-	 * Now that we woke up, it's crucial if we're supposed to be
./kernel/signal.c-	 * frozen that we freeze now before running anything substantial.
./kernel/signal.c-	 */
./kernel/signal.c:	try_to_freeze();
./kernel/signal.c-
./kernel/signal.c-	spin_lock_irq(&sighand->siglock);
./kernel/signal.c-	/*
--
./net/core/pktgen.c-			t->control &= ~(T_REMDEV);
./net/core/pktgen.c-		}
./net/core/pktgen.c-
./net/core/pktgen.c:		try_to_freeze();
./net/core/pktgen.c-
./net/core/pktgen.c-		set_current_state(TASK_INTERRUPTIBLE);
./net/core/pktgen.c-	}
--
./net/sunrpc/svc_xprt.c-
./net/sunrpc/svc_xprt.c-		time_left = schedule_timeout(timeout);
./net/sunrpc/svc_xprt.c-
./net/sunrpc/svc_xprt.c:		try_to_freeze();
./net/sunrpc/svc_xprt.c-
./net/sunrpc/svc_xprt.c-		spin_lock_bh(&pool->sp_lock);
./net/sunrpc/svc_xprt.c-		remove_wait_queue(&rqstp->rq_wait, &wait);
--
./arch/m32r/kernel/signal.c-	if (!user_mode(regs))
./arch/m32r/kernel/signal.c-		return 1;
./arch/m32r/kernel/signal.c-
./arch/m32r/kernel/signal.c:	if (try_to_freeze()) 
./arch/m32r/kernel/signal.c-		goto no_signal;
./arch/m32r/kernel/signal.c-
./arch/m32r/kernel/signal.c-	if (!oldset)
--
./arch/h8300/kernel/signal.c-	if ((regs->ccr & 0x10))
./arch/h8300/kernel/signal.c-		return 1;
./arch/h8300/kernel/signal.c-
./arch/h8300/kernel/signal.c:	if (try_to_freeze())
./arch/h8300/kernel/signal.c-		goto no_signal;
./arch/h8300/kernel/signal.c-
./arch/h8300/kernel/signal.c-	current->thread.esp0 = (unsigned long) regs;
--
./arch/powerpc/platforms/ps3/device-init.c-
./arch/powerpc/platforms/ps3/device-init.c-	/* Loop here processing the requested notification events. */
./arch/powerpc/platforms/ps3/device-init.c-	do {
./arch/powerpc/platforms/ps3/device-init.c:		try_to_freeze();
./arch/powerpc/platforms/ps3/device-init.c-
./arch/powerpc/platforms/ps3/device-init.c-		memset(notify_event, 0, sizeof(*notify_event));
./arch/powerpc/platforms/ps3/device-init.c-
--
./arch/powerpc/platforms/83xx/suspend.c-{
./arch/powerpc/platforms/83xx/suspend.c-	while (1) {
./arch/powerpc/platforms/83xx/suspend.c-		wait_event_interruptible(agent_wq, pci_pm_state >= 2);
./arch/powerpc/platforms/83xx/suspend.c:		try_to_freeze();
./arch/powerpc/platforms/83xx/suspend.c-
./arch/powerpc/platforms/83xx/suspend.c-		if (signal_pending(current) || pci_pm_state < 2)
./arch/powerpc/platforms/83xx/suspend.c-			continue;
--
./arch/blackfin/kernel/signal.c-
./arch/blackfin/kernel/signal.c-	current->thread.esp0 = (unsigned long)regs;
./arch/blackfin/kernel/signal.c-
./arch/blackfin/kernel/signal.c:	if (try_to_freeze())
./arch/blackfin/kernel/signal.c-		goto no_signal;
./arch/blackfin/kernel/signal.c-
./arch/blackfin/kernel/signal.c-	if (test_thread_flag(TIF_RESTORE_SIGMASK))
--
./arch/frv/kernel/signal.c-	if (!user_mode(__frame))
./arch/frv/kernel/signal.c-		return;
./arch/frv/kernel/signal.c-
./arch/frv/kernel/signal.c:	if (try_to_freeze())
./arch/frv/kernel/signal.c-		goto no_signal;
./arch/frv/kernel/signal.c-
./arch/frv/kernel/signal.c-	if (test_thread_flag(TIF_RESTORE_SIGMASK))
--
./arch/arm/kernel/signal.c-	if (!user_mode(regs))
./arch/arm/kernel/signal.c-		return;
./arch/arm/kernel/signal.c-
./arch/arm/kernel/signal.c:	if (try_to_freeze())
./arch/arm/kernel/signal.c-		goto no_signal;
./arch/arm/kernel/signal.c-
./arch/arm/kernel/signal.c-	single_step_clear(current);
--
./arch/sh/kernel/signal_32.c-	if (!user_mode(regs))
./arch/sh/kernel/signal_32.c-		return;
./arch/sh/kernel/signal_32.c-
./arch/sh/kernel/signal_32.c:	if (try_to_freeze())
./arch/sh/kernel/signal_32.c-		goto no_signal;
./arch/sh/kernel/signal_32.c-
./arch/sh/kernel/signal_32.c-	if (test_thread_flag(TIF_RESTORE_SIGMASK))
...

									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Dasgupta, Romit]

> Really? I believe the "ktrhead_should_stop" is new rule, and code does
> not seem to follow it. Actually, for example audit does not seem to
> use kthread_should_stop() at all...
> 
> ./kernel/rtmutex-tester.c-
> ./kernel/rtmutex-tester.c-		/* Wait for the next 
> command to be executed */
> ./kernel/rtmutex-tester.c-		schedule();
> ./kernel/rtmutex-tester.c:		try_to_freeze();
> ./kernel/rtmutex-tester.c-
> ./kernel/rtmutex-tester.c-		if (signal_pending(current))
> ./kernel/rtmutex-tester.c-			flush_signals(current);
> --
Not a new rule. For these threads you listed no one stops them by sending 
'kthread_stop' so the problem does not arise! But for the threads that are 
stopped by invoking kthread_stop they do check kthread_should_stop. 

Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads

[Rafael J. Wysocki]

On Monday 09 November 2009, Dasgupta, Romit wrote:
> > Really? I believe the "ktrhead_should_stop" is new rule, and code does
> > not seem to follow it. Actually, for example audit does not seem to
> > use kthread_should_stop() at all...
> > 
> > ./kernel/rtmutex-tester.c-
> > ./kernel/rtmutex-tester.c-		/* Wait for the next 
> > command to be executed */
> > ./kernel/rtmutex-tester.c-		schedule();
> > ./kernel/rtmutex-tester.c:		try_to_freeze();
> > ./kernel/rtmutex-tester.c-
> > ./kernel/rtmutex-tester.c-		if (signal_pending(current))
> > ./kernel/rtmutex-tester.c-			flush_signals(current);
> > --
> Not a new rule. For these threads you listed no one stops them by sending 
> 'kthread_stop' so the problem does not arise! But for the threads that are 
> stopped by invoking kthread_stop they do check kthread_should_stop. 

At the moment we don't have the rule that kernel threads should exit
immediately after kthread_stop() is called for them, which your patch implies
for freezable kernel threads.

Now, I think we might require the freezable kernel threads to exit as soon as
ktrhead_should_stop() is true for them, but (a) that needs to be documented
(please note it also applies to freezable workqueues) and (b) the existing
freezable kernel threads need to be audited, so we're sure they follow this
rule.  Also, it might lead to subtle problems if someone overlooks this
rule in the future.

So, I'd rather not introduce such a rule if there's no other way to fix the
problem at hand.

BTW, in future please describe the motivation for a change in the changelog
or people will wonder what the change is for.  In particular, if it fixes a
problem, please describe the problem and why you think your approach is
appropriate for fixing it.

Thanks,
Rafael