Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Jeff Layton]

On Mon, 31 Oct 2011 17:55:05 -0700
Tejun Heo <tj@kernel.org> wrote:

> Hey, again.
> 
> On Mon, Oct 31, 2011 at 04:30:59PM -0700, Tejun Heo wrote:
> > 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.
> 

(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.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Jeff Layton]

On Tue, 1 Nov 2011 04:13:37 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Mon, 31 Oct 2011 17:55:05 -0700
> Tejun Heo <tj@kernel.org> wrote:
> 
> > Hey, again.
> > 
> > On Mon, Oct 31, 2011 at 04:30:59PM -0700, Tejun Heo wrote:
> > > 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.
> > 
> 
> (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.
> 

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?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Tejun Heo]

Hello, Jeff.

On Tue, Nov 01, 2011 at 06:59:58AM -0400, Jeff Layton wrote:
> > 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.

> 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)

Thanks.

-- 
tejun

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Trond Myklebust]

On Tue, 2011-11-01 at 09:30 -0700, Tejun Heo wrote: 
> Hello, Jeff.
> 
> On Tue, Nov 01, 2011 at 06:59:58AM -0400, Jeff Layton wrote:
> > > 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.
> 
> > 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)

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

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Tejun Heo]

Hello,

On Tue, Nov 01, 2011 at 12:49:31PM -0400, Trond Myklebust wrote:
> > #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?

Ah... right, forgot about signal_pending_state() special case in
schedule().  Any better ideas, anyone?

-- 
tejun

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

On 11/01, Tejun Heo wrote:
>
> diddling with
> sigmask in kernel generally isn't a good idea.

Agreed.

> 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();						\

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.

Oleg.

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Tejun Heo]

On Tue, Nov 01, 2011 at 06:59:53PM +0100, Oleg Nesterov wrote:
> > #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, oh
well.  Any better ideas?

-- 
tejun

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

On 11/01, Tejun Heo wrote:
>
> 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().

> 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?

Oleg.

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Jeff Layton]

On Tue, 1 Nov 2011 11:06:01 -0700
Tejun Heo <tj@kernel.org> wrote:

> On Tue, Nov 01, 2011 at 06:59:53PM +0100, Oleg Nesterov wrote:
> > > #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, oh
> well.  Any better ideas?
> 

This is (obviously) not my area of expertise, but the TAKE_WAKEFREEZE
flag that you mentioned earlier might be the best way to solve this.

Tasks that want to be awoken on freezer events can set that flag and we
can have the freezer code wake them up. For the NFS and CIFS code, we'll
just ensure that we set that flag

This does mean a rather nasty proliferation of new wait_event_* macros,
and we'll need a new schedule_timeout_freezekillable() variant for the
new state. It looks fairly straightforward to implement though.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Tejun Heo]

Hello,

On Tue, Nov 01, 2011 at 07:13:29PM +0100, Oleg Nesterov wrote:
> On 11/01, Tejun Heo wrote:
> >
> > 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().

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.

> > 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?

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.

Thanks.

-- 
tejun

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

On 11/01, Tejun Heo wrote:
>
> Hello,
>
> On Tue, Nov 01, 2011 at 07:13:29PM +0100, Oleg Nesterov wrote:
> > On 11/01, Tejun Heo wrote:
> > >
> > > 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().
>
> 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.

OK, agreed, __wait_event_interruptible() was a bad example.

Yes, this is only needed for the code which doesn't check
signal_pending() "properly", or doesn't check it at all before
schedule(). OK, say, wait_for_completion_interruptible().
Or schedule_timeout_interruptible().

I suspect we have a lot more examples. Historically linux allows to
set TASK_INTERRUPTIBLE and schedule() without any checks.

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

Yes, I don't think we can remove it.

> > > 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?
>
> Yeah, probably.  I was hoping to remove count/do_not_count tho.

Or at least rename it ;)

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

Perhaps.

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().

Oleg.

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

On 11/01, Oleg Nesterov wrote:
>
> 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.

Oleg.

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Tejun Heo]

Hey, Oleg.

On Tue, Nov 01, 2011 at 08:46:01PM +0100, Oleg Nesterov wrote:
> On 11/01, Oleg Nesterov wrote:
> >
> > 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.

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.

Thanks.

-- 
tejun

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Jeff Layton]

On Tue, 1 Nov 2011 14:57:10 -0700
Tejun Heo <tj@kernel.org> wrote:

> Hey, Oleg.
> 
> On Tue, Nov 01, 2011 at 08:46:01PM +0100, Oleg Nesterov wrote:
> > On 11/01, Oleg Nesterov wrote:
> > >
> > > 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.
> 
> 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?
> 

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?

If so, guess I'll need to respin the NFS/RPC patches for this to do
something similar around the sleeps there since they don't use
wait_event_*.

> 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.
> 

wait_event_state() sounds like a wonderful idea regardless of what we
do here.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

On 11/02, Jeff Layton wrote:
>
> On Tue, 1 Nov 2011 14:57:10 -0700
> Tejun Heo <tj@kernel.org> wrote:
>
> > 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().

Yes. PF_FREEZER_SKIP means, "please count me as frozen". This task can
do nothing except refrigerator() after wakeup.

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

I think this should work... but this is the question to you ;)

Oleg.

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

Hi,

On 11/01, Tejun Heo wrote:
>
> 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.

Oleg.

[PATCH] wait_event_freezekillable: use freezer_do_not_count/freezer_count

[Oleg Nesterov]

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 <oleg@redhat.com>
---

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

--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -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));	\
-		if (__retval || (condition))				\
-			break;						\
-		try_to_freeze();					\
-	}								\
+	freezer_do_not_count();						\
+	__retval = wait_event_killable(wq, (condition));		\
+	freezer_count();						\
 	__retval;							\
 })
 

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Rafael J. Wysocki]

On Wednesday, November 02, 2011, Oleg Nesterov wrote:
> Hi,
> 
> On 11/01, Tejun Heo wrote:
> >
> > 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 ?

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.

> 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).

Thanks,
Rafael

Re: [PATCH] wait_event_freezekillable: use freezer_do_not_count/freezer_count

[Jeff Layton]

On Wed, 2 Nov 2011 18:53:27 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> 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 <oleg@redhat.com>
> ---
> 
>  include/linux/freezer.h |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -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));	\
> -		if (__retval || (condition))				\
> -			break;						\
> -		try_to_freeze();					\
> -	}								\
> +	freezer_do_not_count();						\
> +	__retval = wait_event_killable(wq, (condition));		\
> +	freezer_count();						\
>  	__retval;							\
>  })
>  
> 

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...

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Jeff Layton]

On Thu, 3 Nov 2011 00:11:23 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Wednesday, November 02, 2011, Oleg Nesterov wrote:
> > Hi,
> > 
> > On 11/01, Tejun Heo wrote:
> > >
> > > 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 ?
> 
> 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.
> 
> > 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).
> 

FWIW, wrapping wait_event_killable in freezer_do_not_count/freezer_count
seems to work. The machine suspends consistently with it. It sounds
like Rafael has concerns about this scheme however, so I'll let you
guys argue this out :)

Once you tell me the right scheme to use, I'll be happy to fix up cifs
and nfs to use it.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] wait_event_freezekillable: use freezer_do_not_count/freezer_count

[Tejun Heo]

Hello,

On Thu, Nov 03, 2011 at 06:42:15AM -0400, Jeff Layton wrote:
> 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...

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.

Thank you.

-- 
tejun

Re: [RFC PATCH] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

[Oleg Nesterov]

On 11/03, Rafael J. Wysocki wrote:
>
> On Wednesday, November 02, 2011, Oleg Nesterov wrote:
> >
> > Rafael, could you remind why freezer_do_not_count/freezer_count check
> > ->mm != NULL ?
>
> You're asking difficult questions. ;-)

I am trying ;)

> 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.

Actually CLONE_VFORK is used by call_usermodehelper() paths but this case
is fine. The caller is the PF_NOFREEZE workqueue thread.

> > 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).

Indeed! So why do we check ->mm != NULL?

We can remove this check, right now it doesn't matter. And we are trying
to avoid the new users of this interface.

Oleg.

Re: [PATCH] wait_event_freezekillable: use freezer_do_not_count/freezer_count

[Jeff Layton]

On Thu, 3 Nov 2011 07:13:54 -0700
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> On Thu, Nov 03, 2011 at 06:42:15AM -0400, Jeff Layton wrote:
> > 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...
> 
> 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...

[1] Here are the older patches that depended on the change to
fake_signal_wake_up. I'll need to convert these to use the new scheme
once you guys settle on what it should be:

https://git.samba.org/jlayton/?p=jlayton/linux.git;a=commitdiff;h=0f85cbb747a0f9f8a582ae9bf642e094168001be
https://git.samba.org/jlayton/?p=jlayton/linux.git;a=commitdiff;h=084535708bcb33dd2448e73be5a0f4cac69bea92

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] wait_event_freezekillable: use freezer_do_not_count/freezer_count

[Tejun Heo]

Hello,

On Thu, Nov 03, 2011 at 11:27:04AM -0400, Jeff Layton wrote:
> 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...

Yes, I still think it would be better to isolate implementation
details from users.  I really hate to see the 'count' functions
scattered around the kernel.

Thank you.

-- 
tejun