Hi,
Following the "Freezing of kernel threads" discussion

(http://lkml.org/lkml/2007/5/12/162) I have created a patch that changes the

freezer's behavior with respect to kernel threads.  Namely, with the patch

applied all kernel threads are nonfreezable by default (have PF_NOFREEZE set)

and the ones that want to be frozen need to call set_freezable() (which clears

PF_NOFREEZE for them) and try_to_freeze().  The other (nonfreezable) kernel

threads don't need to call try_to_freeze() any more.
I have removed try_to_freeze() from a handful of kernel threads that I think

need not be freezable, but in many cases I wasn't quite sure whether or not

it was a good idea to change the current behavior.  For this reason I've added

set_freezable() to the majority of (currently freezable) kernel threads that

belong to device drivers and filesystems.
Of course, I have removed the setting of PF_NOFREEZE from the kernel threads

that are currently nonfreezable, since with the other changes in the patch it

isn't necessary any more.
This patch is on top of the "Freezer: Avoid freezing kernel threads prematurely"

patch that I posted yestarday, available at http://lkml.org/lkml/2007/5/25/199

(updated version that applies cleanly on top of 2.6.22-rc3, is available at

http://www.sisk.pl/kernel/hibernation_and_suspend/2.6.22-rc3/patches/07-...).

It has been tested on a couple of machines and doesn't seem to break anything.
[As you can see there are quite a lot of files affected, so I didn't add all

maintainers of them to the CC list.  In fact, I'm not sure how to handle

notifying them of the change, so please advise.]
Greetings,

Rafael
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

---

 Documentation/power/kernel_threads.txt    |   54 ++++++++++++------------------

 Documentation/power/swsusp.txt            |   16 --------

 arch/i386/kernel/apm.c                    |    1

 arch/i386/kernel/io_apic.c                |    1

 drivers/bl...
[ message continues ]

Well... and unless thread that does disk writes or DMA _wants_ to, you

have nice disk corruption... It should be pointed out that it is not

voluntary for those types of threads.

									Pavel

--

(english) http://www.livejournal.com/~pavelmachek

(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-

Well, that really depends.
I think the only thing that must not be written to disk is filesystem stuff.

The other things (eg. writing directly to block devices etc.) doesn't hurt us.
Also, I think that DMA-ing while we're creating the image should be impossible

due to devices being frozen at that point.
That said, I think we need to write some better freezer documentation ASAP.
Greetings,

Rafael

-

Well.. it can write anywhere it wants (filesystem or not) as long as

the system is not going to be confused after resume by its caches not

matching on-disk state. I'd prefer it not to write anywhere at all.
								Pavel

--

(english) http://www.livejournal.com/~pavelmachek

(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-

Hi,
OK
Please have a look at the current version of the patch (appended).
I have followed the Nigel's suggestion not to change the current behavior

in this patch (I'll add a couple of patches removing the freezability from

some kernel threads), with one exception: I couldn't figure out any reason

to have try_to_freeze() called in net/sunrpc/svcsock.c:svc_recv() .
I've also added a piece of documentation, freezing-of-tasks.txt .  Please

see if it's not missing anything (I'd like it to be quite complete).
Greetings,

Rafael
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

---

 Documentation/power/freezing-of-tasks.txt |  160 ++++++++++++++++++++++++++++++

 Documentation/power/kernel_threads.txt    |   40 -------

 Documentation/power/swsusp.txt            |   18 ---

 arch/i386/kernel/apm.c                    |    1

 arch/i386/kernel/io_apic.c                |    1

 drivers/block/loop.c                      |    7 -

 drivers/block/pktcdvd.c                   |    1

 drivers/char/apm-emulation.c              |   11 --

 drivers/char/hvc_console.c                |    1

 drivers/edac/edac_mc.c                    |    1

 drivers/ieee1394/ieee1394_core.c          |    2

 drivers/ieee1394/nodemgr.c                |    1

 drivers/input/gameport/gameport.c         |    1

 drivers/input/serio/serio.c               |    1

 drivers/input/touchscreen/ucb1400_ts.c    |    1

 drivers/macintosh/therm_adt746x.c         |    1

 drivers/macintosh/windfarm_core.c         |    1

 drivers/md/md.c                           |    1

 drivers/media/dvb/dvb-core/dvb_frontend.c |    1

 drivers/media/video/cx88/cx88-tvaudio.c   |    1

 drivers/media/video/msp3400-kthreads.c    |    5

 drivers/media/video/tvaudio.c             |    2

 drivers/media/video/video-buf-dvb.c       |    1

 drivers/media/video/vivi.c                |    1

 drivers/mfd/ucb1x00-ts.c                  |    1

 drivers/mmc/card/queue.c                  |    6 -

 drivers/mtd/mtd_blkdevs.c             ...
[ message continues ]

Heh. Your previous patch removed more lines than it added, this one adds

more lines than it removes. Snif..
I realize that it's all from that Documentation update:
 Documentation/power/freezing-of-tasks.txt |  160 ++++++++++++++++++++++++++++++

 Documentation/power/kernel_threads.txt    |   40 -------

 Documentation/power/swsusp.txt            |   18 ---
so it's all good. Anyway, I'll happily do this (along with the patch to

not do freezer at all for STR) after 2.6.22 is out, but until then I'll

obviously be ignoring the patches flying around..
		Linus

-

It probably broke suspend at some point... leave it there. Processes

can stay in D period, waiting for NFS server to come back.
and yes, we want nfs threads frozen, too (and anything that talks to

network). Speaking to nfs servers while we are suspending the machine

is not nice, and if that continues after snapshot, we'll act as a very

confused machine to the outside...
								Pavel

--

(english) http://www.livejournal.com/~pavelmachek

(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-

OK, I've added set_freezable() to the NFS-related threads.
[Updated patch is in the reply to Nigel.]
Greetings,

Rafael

-

Hi.
[...]
Mostly just grammar and the odd typo. On the whole, it's really well

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
s/Namely, for now/At the moment/
efore
t might
ds may
s/them is/these are/
es may
I'd shift the "For example" to after "may", giving "...may, for example,
I found these first two lines confusing - I though the "Why we

freeze..." was Linus, rather than a quotation he was responding to. I'd

suggest starting the quote at what follows this point... but then as I

read further, I can see the quote is necessary to make sense of the

second paragraph below. Perhaps the best way would to put a line before
Oh, and double quotes should surround the whole quote, with single

quotes replacing the double quotes in the quotation. Hope all those

occured
ce we
erage.
s/stuffs up/distorts/ ('Stuffs up' is accurate as a colloquialism, but

I'm suggesting the change because the language in the remainder of the

tml).
ough
y.]
I understand the logic and agree with that you're trying to say in this

last example, but think the example is faulty. If the firmware is on a

filesystem accessible only through the device that needs the firmware,

then you wouldn't be able to bring it up in the first place.
Regards,
Nigel

Yup.
Removed.
Fixed.
Very true.  I've changed that in the updated patch (appended).
Thanks again,

Rafael
Hi,
OK
Please have a look at the current version of the patch (appended).
I have followed the Nigel's suggestion not to change the current behavior

in this patch (I'll add a couple of patches removing the freezability from

some kernel threads), with one exception: I couldn't figure out any reason

to have try_to_freeze() called in net/sunrpc/svcsock.c:svc_recv() .
I've also added a piece of documentation, freezing-of-tasks.txt .  Please

see if it's not missing anything (I'd like it to be quite complete).
Greetings,

Rafael
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 Documentation/power/freezing-of-tasks.txt |  160 ++++++++++++++++++++++++++++++

 Documentation/power/kernel_threads.txt    |   40 -------

 Documentation/power/swsusp.txt            |   18 ---

 arch/i386/kernel/apm.c                    |    1

 arch/i386/kernel/io_apic.c                |    1

 drivers/block/loop.c                      |    7 -

 drivers/block/pktcdvd.c                   |    1

 drivers/char/apm-emulation.c              |   11 --

 drivers/char/hvc_console.c                |    1

 drivers/edac/edac_mc.c                    |    1

 drivers/ieee1394/ieee1394_core.c          |    2

 drivers/ieee1394/nodemgr.c                |    1

 drivers/input/gameport/gameport.c         |    1

 drivers/input/serio/serio.c               |    1

 drivers/input/touchscreen/ucb1400_ts.c    |    1

 drivers/macintosh/therm_adt746x.c         |    1

 drivers/macintosh/windfarm_core.c         |    1

 drivers/md/md.c                           |    1

 drivers/media/dvb/dvb-core/dvb_frontend.c |    1

 drivers/media/video/cx88/cx88-tvaudio.c   |    1

 drivers/media/video/msp3400-kthreads.c    |    5

 drivers/media/video/tvaudio.c             |    2

 drivers/media/video/video-buf-dvb.c       |    1

 drivers/media/video/vivi.c                |    1

 drivers/mfd...
[ message continues ]

Hi.
Thank you for doing all this work in the first place!
Acked-by: Nigel Cunningham <nigel@nigel.suspend2.net>
Regards,
Nigel

Hello!
In reply to your more recent message, I had looked but not tried, so

didn't feel in a position to reply yet.
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
s/transision/transition

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
Given the clearing of the flag above, should we just have a

set_unfreezeable here that's used above (and potentially elsewhere)...

(reads more)... or more generic set_[un]freezeable(task_struct *p)

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
I'm the one who is confused, aren't I? If I'm reading this right,

io_apic used to be frozen. After this patch, it will not be frozen. If

that's the intended behaviour, shouldn't this be two patches - one to

make kernel threads unfreezeable by default, and one to make threads

that were formerly freezeable unfreezeable?
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
Perhaps it would be good to keep a variant of this question, along the

lines of:
Q: I have a kernel thread that needs to be frozen during hibernation.

How do I make that happen?
Regards,
Nigel

Yes, I can introduce set_unfreezeable(), although that would be used in

a couple of places only.
I don't think it's a good idea to have set_[un]freezeable(task_struct *p),
Yes, I think you're right.  I tend to try to make too many changes in one
Good idea.
Thanks for the comments.
Greetings,

Rafael

-

Hi.
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
The copy_flags routine changes another process's flags - that's why I

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
Glad to be of service!
Nigel

Hi,
Below is the latest version of the $subject patch, with a changelog.
This is the version I'd like to submit for merging, so if you have any comments

or objections, please let me know.
Greetings,

Rafael
---

From: Rafael J. Wysocki <rjw@sisk.pl>
Currently, the freezer treats all tasks as freezable, except for the kernel

threads that explicitly set the PF_NOFREEZE flag for themselves.  This approach

is problematic, since it requires every kernel thread to either set PF_NOFREEZE

explicitly, or call try_to_freeze(), even if it doesn't care for the freezing of

tasks at all.
It seems better to only require the kernel threads that want to or need to be

frozen to use some freezer-related code and to remove any freezer-related code

from the other (nonfreezable) kernel threads, which is done in this patch.
The patch causes all kernel threads to be nonfreezable by default (ie. to have

PF_NOFREEZE set by default) and introduces the set_freezable() function that

should be called by the freezable kernel threads in order to unset PF_NOFREEZE.

It also makes all of the currently freezable kernel threads call

set_freezable(), so it shouldn't cause any (intentional) change of behaviour to

appear.  Additionally, it updates documentation to describe the freezing of

tasks more accurately.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Nigel Cunningham <nigel@nigel.suspend2.net>

---

 Documentation/power/freezing-of-tasks.txt |  160 ++++++++++++++++++++++++++++++

 Documentation/power/kernel_threads.txt    |   40 -------

 Documentation/power/swsusp.txt            |   18 ---

 arch/i386/kernel/apm.c                    |    1

 arch/i386/kernel/io_apic.c                |    1

 drivers/block/loop.c                      |    7 -

 drivers/block/pktcdvd.c                   |    1

 drivers/char/apm-emulation.c              |   11 --

 drivers/char/hvc_console.c                |    1

 drivers/edac/edac_mc.c                    |    1

 drivers/ieee1394/ieee1394_co...
[ message continues ]

Yes, it does, but I'm dropping the clearing of PF_NOFREEZE from there,

not adding anything new. :-)
Greetings,

Rafael

-

Hi.
Doh! Sorry :)
Nigel

Does the lack of comments mean that everyone on the CC list agrees with this

approach? ;-)
In the meantime, it turns out that this patch fixes the hibernation/suspend

problem with cryptd discussed in the thread at

http://lkml.org/lkml/2007/5/26/24 .
The problem is that cryptd doesn't call try_to_freeze() and doesn't set

PF_NOFREEZE for itself, so the freezer cannot handle it properly.  In principle

we can add either try_to_freeze() or the setting of PF_NOFREEZE to it, but if

the approach in the $subject patch is acceptable, we'll need to remove that

soon.  So, what should we do?
Rafael
-

I'd add PF_NONFREEZE. Changing the defaults in freezer is obviously

not 2.6.22 material.

								Pavel

--

(english) http://www.livejournal.com/~pavelmachek

(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-

OK
Rafael

-