freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

Commit 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too" made freezer wake up tasks in TASK_KILLABLE sleep too citing non-interruptible but killable sleeps in cifs and nfs.

I don't think we can do this. We should not send spurious unsolicited non-interruptible wakeups. Most synchornization constructs are built to cope with spurious wakeups and any INTERRUPTIBLE sleep must be able to handle spurious wakeups but that's not true for KILLABLE sleeps - KILLABLE condition cannot be cancelled.

This is probably okay for most cases but circumventing fundamental wakeup condition like this is asking for trouble. Furthermore, I'm not sure the behavior change brought on by this change - breaking nfs/cifs uninterruptible operation guarantee - is correct. If such behavior is desirable, the right thing to do is using intr mount option, not circumventing it from PM layer.

Signed-off-by Tejun Heo <[email protected]>

Cc Jeff Layton <[email protected]>

---
Neil, Steve, do the network filesystems need a way to indicate "I can either be killed or enter freezer"?

Commit 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too" made freezer wake up tasks in TASK_KILLABLE sleep too citing non-interruptible but killable sleeps in cifs and nfs.

I don't think we can do this. We should not send spurious unsolicited non-interruptible wakeups. Most synchornization constructs are built to cope with spurious wakeups and any INTERRUPTIBLE sleep must be able to handle spurious wakeups but that's not true for KILLABLE sleeps - KILLABLE condition cannot be cancelled.

This is probably okay for most cases but circumventing fundamental wakeup condition like this is asking for trouble. Furthermore, I'm not sure the behavior change brought on by this change - breaking nfs/cifs uninterruptible operation guarantee - is correct. If such behavior is desirable, the right thing to do is using intr mount option, not circumventing it from PM layer.

Signed-off-by Tejun Heo <[email protected]>

Cc Jeff Layton <[email protected]>

---
Neil, Steve, do the network filesystems need a way to indicate "I can either be killed or enter freezer"?

Thanks.

 kernel/freezer.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/freezer.c +0 -0

index 66a594e..7b01de9 100644

Quoted 6 lines

This is probably okay for most cases but circumventing fundamental wakeup condition like this is asking for trouble. Furthermore, I'm not sure the behavior change brought on by this change - breaking nfs/cifs uninterruptible operation guarantee - is correct. If such behavior is desirable, the right thing to do is using intr mount option, not circumventing it from PM layer.

Do you have any specific examples of breakage, or is it just that you _think_ it's not quite right?

I can't remember one off the top of my head but I'm pretty sure there at least are few which expect tight inter-locking between sleeps and wakeups. I'll look for examples and post reply. ISTR them being kernel threads so this might not apply directly but it's still a dangerous game to play.

Hey, again.

Quoted 5 lines · Tejun Heo

I can't remember one off the top of my head but I'm pretty sure there at least are few which expect tight inter-locking between sleeps and wakeups. I'll look for examples and post reply. ISTR them being kernel threads so this might not apply directly but it's still a dangerous game to play.

Hmm... I couldn't find KILLABLE used like that but here are two UNINTERRUPTIBLE sleep examples.

* kthread_start() depends on the fact that a kthread won't be woken up from UNINTERRUPTIBLE sleep spuriously.

* jfs_flush_journal() doesn't check whether the wakeup was spurious after waiting if !tblkGC_COMMITTED.

Maybe we can re-define KILLABLE as killable && freezable but IMHO that requires pretty strong rationales. If at all possible, let's not diddle with that if it can be worked around some other way.

Thank you.

Quoted 15 lines · On Mon, 31 Oct 2011 17:55:05 -0700 Tejun Heo…

Hey, again.

Quoted 5 lines · Tejun Heo

I can't remember one off the top of my head but I'm pretty sure there at least are few which expect tight inter-locking between sleeps and wakeups. I'll look for examples and post reply. ISTR them being kernel threads so this might not apply directly but it's still a dangerous game to play.

Hmm... I couldn't find KILLABLE used like that but here are two UNINTERRUPTIBLE sleep examples.

* kthread_start() depends on the fact that a kthread won't be woken up from UNINTERRUPTIBLE sleep spuriously.

Quoted 9 lines

* jfs_flush_journal() doesn't check whether the wakeup was spurious after waiting if !tblkGC_COMMITTED.

Maybe we can re-define KILLABLE as killable && freezable but IMHO that requires pretty strong rationales. If at all possible, let's not diddle with that if it can be worked around some other way.

Thank you.

(cc'ing Trond and the linux-nfs mailing list -- fwiw, he maintains the NFS client code -- Bruce is the NFS server maintainer and probably has little interest in this thread).

The main reason for this change is primarily that we have people with laptops and nfs and cifs mounts that sometimes fail to suspend.

IIUC, the TASK_KILLABLE was mostly added to ensure that file-store writes would be uninterruptible, but still allow those tasks to be killed if the process is going down anyway.

The intr/nointr mount options in NFS have been deprecated since TASK_KILLABLE was added. The scheme now is basically that those sleeps ignore any signals except for fatal ones. So, that knob is meaningless and has been for a long time now.

cifs never had a working intr/nointr knob, but signal handling while waiting for replies was always a difficult thing to handle correctly. I don't think the right answer is to go back to using such a knob in cifs or nfs.

I suppose we could look at going back to the world of complicated signal handling and TASK_INTERRUPTIBLE, but that's not a trivial change either. The TASK_WAKE_FREEZABLE flag you mention might make more sense than doing that.

Quoted 4 lines

I suppose we could look at going back to the world of complicated signal handling and TASK_INTERRUPTIBLE, but that's not a trivial change either. The TASK_WAKE_FREEZABLE flag you mention might make more sense than doing that.

Also, since task state and signal handling clearly isn't my forte...can you clarify what the main difference is in using a TASK_KILLABLE sleep vs. TASK_INTERRUPTIBLE with all signals but SIGKILL blocked?

I know that the process state would end up different (S vs. D state). Is there anything else we'd need to be concerned with if we converted all these call sites to use such a scheme in lieu of a new TASK_WAKE_FREEZABLE flag or similar?

Hello, Jeff.

Quoted 4 lines · Jeff Layton

Quoted 4 lines

I suppose we could look at going back to the world of complicated signal handling and TASK_INTERRUPTIBLE, but that's not a trivial change either. The TASK_WAKE_FREEZABLE flag you mention might make more sense than doing that.

I see.

Quoted 8 lines

Also, since task state and signal handling clearly isn't my forte...can you clarify what the main difference is in using a TASK_KILLABLE sleep vs. TASK_INTERRUPTIBLE with all signals but SIGKILL blocked?

I know that the process state would end up different (S vs. D state). Is there anything else we'd need to be concerned with if we converted all these call sites to use such a scheme in lieu of a new TASK_WAKE_FREEZABLE flag or similar?

Manipulating sigmask would work in most cases but there are corner cases (e.g. debug signals will force the mask off) and diddling with sigmask in kernel generally isn't a good idea.

If TASK_KILLABLE is only used for killable && freezable, that probably would be the cleanest solution but given the name and the fact that people are in general much more aware of SIGKILL's then freezer, I think adding such assumption is likely to cause very subtle bugs. For example, mem_cgroup_handle_oom() seems to assume that if it's waken from TASK_KILLABLE either the condition it was waiting for is true or it is dying.

If there are only a handful sites which need this type of behavior, wouldn't something like the following work?

#define wait_event_freezekillable(wq, condition) \

do {									\
	DEFINE_WAIT(__wait);						\
	for (;;) {							\
		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
		if (condition || fatal_signal_pending(current))		\
			break;						\
		schedule();						\
		try_to_freeze();					\
	}								\
	finish_wait(&wq, &__wait);					\
} while (0)

Quoted 12 lines

#define wait_event_freezekillable(wq, condition) \

do {									\
	DEFINE_WAIT(__wait);						\
	for (;;) {							\
		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
		if (condition || fatal_signal_pending(current))		\
			break;						\
		schedule();						\
		try_to_freeze();					\
	}								\
	finish_wait(&wq, &__wait);					\
} while (0)

Err... Won't this end up busy-waiting if a non-fatal signal is received?

diddling with
sigmask in kernel generally isn't a good idea.

If there are only a handful sites which need this type of behavior, wouldn't something like the following work?

#define wait_event_freezekillable(wq, condition) \

do {									\
	DEFINE_WAIT(__wait);						\
	for (;;) {							\
		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
		if (condition || fatal_signal_pending(current))		\
			break;						\
		schedule();						\

Quoted 8 lines

#define wait_event_freezekillable(wq, condition) \

do {									\
	DEFINE_WAIT(__wait);						\
	for (;;) {							\
		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
		if (condition || fatal_signal_pending(current))		\
			break;						\
		schedule();						\

No, this can't work, afaics.

Once the caller recieves a non-fatal signal (gets TIF_SIGPENDING), schedule() won't block in TASK_INTERRUPTIBLE state.

IOW, wait_event_freezekillable() becomes a busy-wait loop.

Yeah yeah, Trond already pointed it out. I forgot about the sigpending special case in schedule(), which I think is rather odd,

Any better ideas?

Quoted 3 lines · Tejun Heo

Yeah yeah, Trond already pointed it out. I forgot about the sigpending special case in schedule(), which I think is rather odd,

I disagree with "rather odd" ;)

We have a lot of examples of

	current->state = TASK_INTERRUPTIBLE;
	...
	if (signal_pending())
		break;
	schedule();

Without that special case in schedule() the code above becomes racy. Just consider __wait_event_interruptible().

Quoted 1 line

Any better ideas?

Well. As a simple (probably temporary) fix, I'd suggest

	#define wait_event_freezekillable(wq, condition)
	{
		freezer_do_not_count();
		__retval = wait_event_killable(condition);
		freezer_count();
		__retval;
	}

Do you think it can work?

Hello,

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · Tejun Heo

Yeah yeah, Trond already pointed it out. I forgot about the sigpending special case in schedule(), which I think is rather odd,

I disagree with "rather odd" ;)

We have a lot of examples of

Quoted 8 lines

	current->state = TASK_INTERRUPTIBLE;
	...
	if (signal_pending())
		break;
	schedule();

Without that special case in schedule() the code above becomes racy. Just consider __wait_event_interruptible().

But __wait_event_interruptible() does proper set-TASK_*, check sigpending and schedule() sequence. As long as the waker performs seg-sigpending, wakeup sequence in the correct order, nothing is broken (as w/ any other wakeup conditions). The special case deals with callers which don't check sigpending between set-TASK_* and schedule() and that's the part I think is a bit odd.

Whether I feel
odd or not is irrelevant tho - it's already there.

Quoted 12 lines

Quoted 1 line

Any better ideas?

Well. As a simple (probably temporary) fix, I'd suggest

	#define wait_event_freezekillable(wq, condition)
	{
		freezer_do_not_count();
		__retval = wait_event_killable(condition);
		freezer_count();
		__retval;
	}

Quoted 1 line

Do you think it can work?

Yeah, probably. I was hoping to remove count/do_not_count tho.

Hmmm... maybe we can just flip PF_NOFREEZE instead with a bit of modification, I think.

Or we can add TASK_FREEZABLE (like TASK_WAKEKILL), iirc we already discussed this some time ago. And probably it makes sense to add the generic wait_event_state().

Quoted 4 lines · Oleg Nesterov

Or we can add TASK_FREEZABLE (like TASK_WAKEKILL), iirc we already discussed this some time ago. And probably it makes sense to add the generic wait_event_state().

Forgot to mention. I think that before anything else we need signal_wake_up_state(). For example, note that none of the callers of signal_wake_up(resume => true) in ptrace code wants to wake up the killable task.

Hey, Oleg.

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

Or we can add TASK_FREEZABLE (like TASK_WAKEKILL), iirc we already discussed this some time ago. And probably it makes sense to add the generic wait_event_state().

Quoted 4 lines

Forgot to mention. I think that before anything else we need signal_wake_up_state(). For example, note that none of the callers of signal_wake_up(resume => true) in ptrace code wants to wake up the killable task.

Yeah, agreed for both wait_event_state() and signal_wake_up_state(). For now, let's go with the count/dont_count. Can you please write up a patch for that? Jeff, does this seem okay to you?

For TASK_FREEZABLE, I'm not entirely sure. Combined with wait_event_state(), it can definitely reduce the number of different variants of wait_event_*(). Let's see.

Quoted 3 lines · On Tue, 1 Nov 2011 14:57:10 -0700 Tejun Heo…

For now, let's go with the count/dont_count. Can you please write up a patch for that? Jeff, does this seem okay to you?

Let me make sure I understand since I don't have a great grasp of the freezer internals...

This will set the PF_FREEZER_SKIP flag on the task, which prevents try_to_freeze_tasks from incrementing the "todo" var for this process and should let the suspend proceed.

So this really makes try_to_freeze_tasks set PF_FREEZING on the task, but not get upset that it doesn't actually call try_to_freeze().

Is that sufficient for a process that's just sleeping here?

For now, let's go with the count/dont_count. Can you please write up a patch for that? Jeff, does this seem okay to you?

Hi,

Quoted 3 lines · Tejun Heo

For now, let's go with the count/dont_count. Can you please write up a patch for that? Jeff, does this seem okay to you?

OK, will do in a minute. On top of
"[PATCH pm] freezer: fix wait_event_freezable/__thaw_task races" you sent. (btw, thanks, I forgout about it ;)

Rafael, could you remind why freezer_do_not_count/freezer_count check ->mm != NULL ?

The comment says "However, we don't want kernel threads to be frozen", but it is not clear anyway. A kernel thread simply shouldn't use this interface if it doesn't want to freeze.

And in any case, PF_KTHREAD looks better if we really need to filter out the kernel threads.

Quoted 7 lines · Oleg Nesterov

Hi,

Quoted 3 lines · Tejun Heo

For now, let's go with the count/dont_count. Can you please write up a patch for that? Jeff, does this seem okay to you?

Quoted 6 lines

OK, will do in a minute. On top of
"[PATCH pm] freezer: fix wait_event_freezable/__thaw_task races" you sent. (btw, thanks, I forgout about it ;)

Rafael, could you remind why freezer_do_not_count/freezer_count check ->mm != NULL ?

You're asking difficult questions. ;-)

The intention was to prevent PF_FREEZER_SKIP from having any effect on kernel threads, IIRC. Anyway, there are only two legitimate users of it (vfork and apm_ioctl) and in both cases the task in question is user space.

Quoted 4 lines

The comment says "However, we don't want kernel threads to be frozen", but it is not clear anyway. A kernel thread simply shouldn't use this interface if it doesn't want to freeze.

Quoted 2 lines

And in any case, PF_KTHREAD looks better if we really need to filter out the kernel threads.

PF_FREEZER_SKIP was introduced specifically with vfork in mind and I'm not sure if it's a good idea to re-use it for something else (at least not for something entirely obvious).

Quoted 3 lines · Oleg Nesterov

Rafael, could you remind why freezer_do_not_count/freezer_count check ->mm != NULL ?

You're asking difficult questions. ;-)

The intention was to prevent PF_FREEZER_SKIP from having any effect on kernel threads, IIRC. Anyway, there are only two legitimate users of it (vfork and apm_ioctl) and in both cases the task in question is user space.

Quoted 6 lines

The comment says "However, we don't want kernel threads to be frozen", but it is not clear anyway. A kernel thread simply shouldn't use this interface if it doesn't want to freeze.

And in any case, PF_KTHREAD looks better if we really need to filter out the kernel threads.

PF_FREEZER_SKIP was introduced specifically with vfork in mind and I'm not sure if it's a good idea to re-use it for something else (at least not for something entirely obvious).

fake_signal_wake_up() should not wake up the TASK_KILLABLE tasks, we are going to fix this. This means that wait_event_freezekillable() can't rely on wakeup from freezer. Reimplement it using freezer_do_not_count/freezer_count. This is not really nice, just a simple fix for now.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/freezer.h |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

include/linux/freezer.h +0 -3

@@ -143,13 +143,9 @@ static inline void set_freezable_with_si
 #define wait_event_freezekillable(wq, condition)			\
 ({									\
 	int __retval;							\
-	for (;;) {							\
-		__retval = wait_event_killable(wq,			\
-				(condition) || freezing(current));	\

I'm not sure we really need this macro anymore since this is much simpler. I could just move cifs back to using wait_event_killable and simply wrap it in freezer_do_not_count/freezer_count (with some comments to explain why we're doing that).

In any event, I plan to test this scheme out today and will let you know whether it works...

Hello,

Quoted 5 lines · Jeff Layton

I'm not sure we really need this macro anymore since this is much simpler. I could just move cifs back to using wait_event_killable and simply wrap it in freezer_do_not_count/freezer_count (with some comments to explain why we're doing that).

Quoted 2 lines

In any event, I plan to test this scheme out today and will let you know whether it works...

I think it would be better to put this in a macro as the implementation is likely to change in future and I really don't want to see FREEZER_SKIP scattered around the tree.

Ok, note though that I also need to do a similar set of changes to the nfs and sunrpc code [1].

Those call sites do not use wait_event_* macros though. So I'll either need to add a separate set of functions/macros to handle those, or sprinkle freezer_do_not_count/freezer_count around the code there...

Quoted 11 lines · Oleg Nesterov

Quoted 8 lines

#define wait_event_freezekillable(wq, condition) \

do {									\
	DEFINE_WAIT(__wait);						\
	for (;;) {							\
		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
		if (condition || fatal_signal_pending(current))		\
			break;						\
		schedule();						\

No, this can't work, afaics.

Quoted 4 lines

Once the caller recieves a non-fatal signal (gets TIF_SIGPENDING), schedule() won't block in TASK_INTERRUPTIBLE state.

IOW, wait_event_freezekillable() becomes a busy-wait loop.

Yeah yeah, Trond already pointed it out. I forgot about the sigpending special case in schedule(), which I think is rather odd, oh well. Any better ideas?

Quoted 11 lines · Tejun Heo

Commit 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too" made freezer wake up tasks in TASK_KILLABLE sleep too citing non-interruptible but killable sleeps in cifs and nfs.

I don't think we can do this.  We should not send spurious unsolicited non-interruptible wakeups.  Most synchornization constructs are built to cope with spurious wakeups and any INTERRUPTIBLE sleep must be able to handle spurious wakeups but that's not true for KILLABLE sleeps - KILLABLE condition cannot be cancelled.

Quoted 6 lines

This is probably okay for most cases but circumventing fundamental wakeup condition like this is asking for trouble.  Furthermore, I'm not sure the behavior change brought on by this change - breaking nfs/cifs uninterruptible operation guarantee - is correct.  If such behavior is desirable, the right thing to do is using intr mount option, not circumventing it from PM layer.

Do you have any specific examples of breakage, or is it just that you _think_ it's not quite right?

One patch depending on that change has been merged already and I have two more in the queue, so I'd like to clarify this ASAP.  Jeff, Steve?

Quoted 5 lines

Signed-off-by Tejun Heo <[email protected]>

Cc Jeff Layton <[email protected]>

---
Neil, Steve, do the network filesystems need a way to indicate "I can either be killed or enter freezer"?

Quoted 5 lines

Quoted 5 lines

Signed-off-by Tejun Heo <[email protected]>

Cc Jeff Layton <[email protected]>

---
Neil, Steve, do the network filesystems need a way to indicate "I can either be killed or enter freezer"?

Probably, yes, but I will defer to Jeff as he has looked more recently at these issues.

I can explain cifs state, and disconnect/reconnection of sessions (and smb2 is a little more feature rich in this regard), but will let Jeff explain the more subtle points you are getting at.

Hello,

Quoted 6 lines · Steve French

Quoted 5 lines

Quoted 5 lines

Signed-off-by Tejun Heo <[email protected]>

Cc Jeff Layton <[email protected]>

---
Neil, Steve, do the network filesystems need a way to indicate "I can either be killed or enter freezer"?

Quoted 6 lines

Probably, yes, but I will defer to Jeff as he has looked more recently at these issues.

I can explain cifs state, and disconnect/reconnection of sessions (and smb2 is a little more feature rich in this regard), but will let Jeff explain the more subtle points you are getting at.

Hmmm... I'm getting confused.

For nfs, this really is a non-issue.  Either the user wants nointr or intr behavior.  NFS nointr is rather crazy - it's basically "nothing can do anything to tasks which is doing NFS IO until it's complete" and really meant to be used for servers sharing filesystems for /usr, /home and stuff.  It doesn't make whole lot of sense on systems which may go suspend and that's why there's intr option.

I suppose the problem is that cifs doesn't know how to do 'intr' yet, right?  If that really is the problem, the correct long term solution would be implementing proper intr behavior and it doesn't make any sense to push this type of change to PM core to for short term workaround.  Just use prepare_to_wait() / schedule() / finish_wait() directly w/ INTERRUPTIBLE sleep and don't break out of wait loop on signal_pending().  If this should be used in multiple places, write up a wait_event_XXX() wrapper.  There is absolutely no reason to change wakeup condition.

Commit 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too" made freezer wake up tasks in TASK_KILLABLE sleep too citing non-interruptible but killable sleeps in cifs and nfs.

I don't think we can do this. We should not send spurious unsolicited non-interruptible wakeups. Most synchornization constructs are built to cope with spurious wakeups and any INTERRUPTIBLE sleep must be able to handle spurious wakeups but that's not true for KILLABLE sleeps - KILLABLE condition cannot be cancelled.