Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
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-
RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
(Resending with 80 column restriction)
> -Original Message-
> From: Pavel Machek [mailto:pa...@ucw.cz]
> Sent: Sunday, November 08, 2009 1:57 PM
> To: Dasgupta, Romit
> Cc: Rafael J. Wysocki; linux-omap@vger.kernel.org;
> linux-ker...@vger.kernel.org; linux...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
> -Original Message-
> From: Pavel Machek [mailto:pa...@ucw.cz]
> Sent: Sunday, November 08, 2009 1:57 PM
> To: Dasgupta, Romit
> Cc: Rafael J. Wysocki; linux-omap@vger.kernel.org;
> linux-ker...@vger.kernel.org;
> linux...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
> 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
> > ---
> >
> > 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
> > #include
> > #include
> > +#include
> >
> > /*
> > * 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
> ---
>
> 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
> #include
> #include
> +#include
>
> /*
> * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] PM: Thaws refrigerated and to be exited kernel threads
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
---
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
#include
#include
+#include
/*
* 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();
}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
