[PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

Allow the wait_on_bit_killable sleeps in SUNRPC layer to respect the
freezer. This should allow suspend and hibernate events to occur, even
when there are RPC's pending on the wire.

Tested-by: John Hughes <john@Calva.COM>
Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/sched.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index d12ffa5..09bb64e 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -18,6 +18,7 @@
 #include <linux/smp.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 
 #include <linux/sunrpc/clnt.h>
 
@@ -231,7 +232,8 @@ static int rpc_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	if (!try_to_freeze())
+		schedule();
 	return 0;
 }
 
-- 
1.7.6.4

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

On Mon, 21 Nov 2011 12:46:20 -0800
Tejun Heo <tj@kernel.org> wrote:

> On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
> >  /*
> > + * Freezer-friendly macro around schedule() in the kernel.
> > + */
> > +#define freezable_schedule()						\
> > +({									\
> > +	freezer_do_not_count();						\
> > +	schedule();							\
> > +	freezer_count();						\
> > +})
> 
> So, yes, this seems correct to me but I'm not really sure about the
> naming.  Given what freezable means for other interfaces, this is a
> bit confusing to me but that could be because I know how each is
> implemented.  Anyone with a better name?
> 
> Also, this definitely can use a lot more detailed comment.  What it's
> supposed to do, how it's supposed to be used and so on.
> 
> Thank you.
> 

That makes sense. I'll plan to flesh out the comments in a respin.

Yeah, naming these things is rather difficult -- too many "*able"
adjectives. I'm quite open to better suggestions for the name (and for
wait_event_freezekillable too).

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
>  /*
> + * Freezer-friendly macro around schedule() in the kernel.
> + */
> +#define freezable_schedule()						\
> +({									\
> +	freezer_do_not_count();						\
> +	schedule();							\
> +	freezer_count();						\
> +})

So, yes, this seems correct to me but I'm not really sure about the
naming.  Given what freezable means for other interfaces, this is a
bit confusing to me but that could be because I know how each is
implemented.  Anyone with a better name?

Also, this definitely can use a lot more detailed comment.  What it's
supposed to do, how it's supposed to be used and so on.

Thank you.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

On Mon, Nov 21, 2011 at 12:41:41PM -0800, Tejun Heo wrote:
> Hello, Jeff.
> 
> On Mon, Nov 21, 2011 at 03:36:15PM -0500, Jeff Layton wrote:
> > I suppose you're suggesting something like this?
> > 
> > 	freezer_do_not_count();
> > 	if (!try_to_freeze())
> > 		schedule();
> > 	freezer_count();
> 
> No,
> 
> 	freezer_do_not_count();
> 	schedule();
> 	freezer_count();
> 	try_to_freeze();
> 
> freezer_count() may make %current eligible for freezing again, so it
> has to check whether freezing condition is pending afterwards.

Ooh, freezer_count() already has that.  Heh, call me an idiot.  The
proposed code seems correct then.

Thanks.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

Hello, Jeff.

On Mon, Nov 21, 2011 at 03:36:15PM -0500, Jeff Layton wrote:
> I suppose you're suggesting something like this?
> 
> 	freezer_do_not_count();
> 	if (!try_to_freeze())
> 		schedule();
> 	freezer_count();

No,

	freezer_do_not_count();
	schedule();
	freezer_count();
	try_to_freeze();

freezer_count() may make %current eligible for freezing again, so it
has to check whether freezing condition is pending afterwards.

> The freezer is not really my forte', but I have to say that the whole
> do_not_count scheme seems "sketchy". Would we not be better off with
> something closer to the original method that I proposed, along with a
> new TASK_WAKEFREEZE state bit? Processes that are sleeping in
> uninterruptible sleep that are able to deal with freezer events could
> set that bit and fake_signal_wake_up could be changed to also wake
> processes with that bit set.

AFAICS, the above should be race-free w/ the pending freezer update.

  http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=refs/heads/pm-freezer

Thank you.

-- 
tejun

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

On Mon, 21 Nov 2011 09:56:44 -0800
Tejun Heo <tj@kernel.org> wrote:

> On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
> > Allow the freezer to skip wait_on_bit_killable sleeps in the sunrpc
> > layer. This should allow suspend and hibernate events to proceed, even
> > when there are RPC's pending on the wire.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  include/linux/freezer.h |   12 ++++++++++++
> >  net/sunrpc/sched.c      |    3 ++-
> >  2 files changed, 14 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> > index a5386e3..8b60dd0 100644
> > --- a/include/linux/freezer.h
> > +++ b/include/linux/freezer.h
> > @@ -135,6 +135,16 @@ static inline void set_freezable_with_signal(void)
> >  }
> >  
> >  /*
> > + * Freezer-friendly macro around schedule() in the kernel.
> > + */
> > +#define freezable_schedule()						\
> > +({									\
> > +	freezer_do_not_count();						\
> > +	schedule();							\
> > +	freezer_count();						\
> 
> Don't we want try_to_freeze() here?  If the freezer thought the task
> was safe to skip, it better stay inside freezer until the freezing
> condition is lifted.
> 
> Thanks.
> 

I suppose you're suggesting something like this?

	freezer_do_not_count();
	if (!try_to_freeze())
		schedule();
	freezer_count();

Hmm, so I guess the concern is that we'd race in such a way that
freezer_count() ends up running after the freezer has already skipped
the task?

If so, that seems just as racy...nothing guarantees that the freeze
event occurs before the try_to_freeze call...

The freezer is not really my forte', but I have to say that the whole
do_not_count scheme seems "sketchy". Would we not be better off with
something closer to the original method that I proposed, along with a
new TASK_WAKEFREEZE state bit? Processes that are sleeping in
uninterruptible sleep that are able to deal with freezer events could
set that bit and fake_signal_wake_up could be changed to also wake
processes with that bit set.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Tejun Heo]

On Mon, Nov 21, 2011 at 12:40:20PM -0500, Jeff Layton wrote:
> Allow the freezer to skip wait_on_bit_killable sleeps in the sunrpc
> layer. This should allow suspend and hibernate events to proceed, even
> when there are RPC's pending on the wire.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  include/linux/freezer.h |   12 ++++++++++++
>  net/sunrpc/sched.c      |    3 ++-
>  2 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index a5386e3..8b60dd0 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -135,6 +135,16 @@ static inline void set_freezable_with_signal(void)
>  }
>  
>  /*
> + * Freezer-friendly macro around schedule() in the kernel.
> + */
> +#define freezable_schedule()						\
> +({									\
> +	freezer_do_not_count();						\
> +	schedule();							\
> +	freezer_count();						\

Don't we want try_to_freeze() here?  If the freezer thought the task
was safe to skip, it better stay inside freezer until the freezing
condition is lifted.

Thanks.

-- 
tejun

[PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

Allow the freezer to skip wait_on_bit_killable sleeps in the sunrpc
layer. This should allow suspend and hibernate events to proceed, even
when there are RPC's pending on the wire.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/freezer.h |   12 ++++++++++++
 net/sunrpc/sched.c      |    3 ++-
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index a5386e3..8b60dd0 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -135,6 +135,16 @@ static inline void set_freezable_with_signal(void)
 }
 
 /*
+ * Freezer-friendly macro around schedule() in the kernel.
+ */
+#define freezable_schedule()						\
+({									\
+	freezer_do_not_count();						\
+	schedule();							\
+	freezer_count();						\
+})
+
+/*
  * Freezer-friendly wrappers around wait_event_interruptible(),
  * wait_event_killable() and wait_event_interruptible_timeout(), originally
  * defined in <linux/wait.h>
@@ -194,6 +204,8 @@ static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
 static inline void set_freezable_with_signal(void) {}
 
+#define freezable_schedule()  schedule()
+
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
 
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index d12ffa5..5317b93 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -18,6 +18,7 @@
 #include <linux/smp.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 
 #include <linux/sunrpc/clnt.h>
 
@@ -231,7 +232,7 @@ static int rpc_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	freezable_schedule();
 	return 0;
 }
 
-- 
1.7.6.4

[PATCH 1/2] sunrpc: make rpc_wait_bit_killable handle freeze events

[Jeff Layton]

Allow the wait_on_bit_killable sleeps in SUNRPC layer to respect the
freezer. This should allow suspend and hibernate events to occur, even
when there are RPC's pending on the wire.

Tested-by: John Hughes <john@Calva.COM>
Acked-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/sched.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index d12ffa5..09bb64e 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -18,6 +18,7 @@
 #include <linux/smp.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
+#include <linux/freezer.h>
 
 #include <linux/sunrpc/clnt.h>
 
@@ -231,7 +232,8 @@ static int rpc_wait_bit_killable(void *word)
 {
 	if (fatal_signal_pending(current))
 		return -ERESTARTSYS;
-	schedule();
+	if (!try_to_freeze())
+		schedule();
 	return 0;
 }
 
-- 
1.7.6.4