diff mbox

[GIT,PULL,pm-next] freezer: fix various bugs and simplify implementation

Message ID 201108212003.14722.rjw@sisk.pl (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Rafael Wysocki Aug. 21, 2011, 6:03 p.m. UTC
On Sunday, August 21, 2011, Tejun Heo wrote:
> Hello, Rafafel.
> 
> On Sat, Aug 20, 2011 at 06:33:38PM +0200, Rafael J. Wysocki wrote:
> > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/tj/misc.git freezer
> > 
> > Pulled and stored in the pm-freezer branch in my tree, and merged into
> > the linux-next branch.
> 
> Cool.
> 
> > > FYI, this patchset will cause a conflict with s390 TIF flag fix patch.
> > > The conflict is trivial and Stephen should be able to handle it
> > > without any problem.  Also, I'm planning on doing some further work on
> > > cgroup freezer and then will try to bridge it with job control.  If
> > > that plan fans out, I might ask Oleg to pull from the pm tree.
> > 
> > I'm not sure if Linus likes it.  He generally doesn't want the trees
> > that he pulls from to be entangled this way.
> 
> The job control portion has to go through Linus anyway, so let's see
> how that flies.
> 
> > > This shouldn't matter too much either way but it *might* be a good idea to
> > > keep this line of patches in a separate branch.
> > 
> > I'm going to keep it in the pm-freezer branch anyway (there may be patches
> > on top of it, though)
> 
> Yeah, I'm pretty sure it will need some fix too.

Speaking of which, the addition of might_sleep() to try_to_freeze()
causes a badly looking backtrace to appear during reboot on ARM,
so I'd prefer it to go into __refrigerator().

Please tell me what you think of the patch below.

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Freezer: Move might_sleep() from try_to_freeze()

There are some code paths that call try_to_freeze() from interrupt
context, but doing so they know that the current process cannot
possible be freezing (e.g. during reboot on ARM).  However, the
recently added might_sleep() annotation in try_to_freeze()
triggers in those cases, making it look like there were bugs in
those places, which really isn't the case.

Therefore move might_sleep() from try_to_freeze() to
__refrigerator() so that it doesn't produce false positives.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |    1 -
 kernel/freezer.c        |    2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Tejun Heo Aug. 22, 2011, 9:58 a.m. UTC | #1
Hello, Rafael.

On Sun, Aug 21, 2011 at 08:03:14PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Freezer: Move might_sleep() from try_to_freeze()
> 
> There are some code paths that call try_to_freeze() from interrupt
> context, but doing so they know that the current process cannot
> possible be freezing (e.g. during reboot on ARM).  However, the
> recently added might_sleep() annotation in try_to_freeze()
> triggers in those cases, making it look like there were bugs in
> those places, which really isn't the case.
> 
> Therefore move might_sleep() from try_to_freeze() to
> __refrigerator() so that it doesn't produce false positives.

Hmmm... I can't quite agree with this change.  Some invocations of
try_to_freeze() can be very difficult to trigger.  Freezing isn't a
frequent operation after some try_to_freeze() can be buried in weird
places.  might_sleep() is exactly to detect context bugs in these
situations.  If a code path is called from both sleepable and
unsleepable context and it knows that the latter wouldn't happen if
the system is freezing, that code path should conditionalize
invocation of try_to_freeze() based on its knowledge of context.  That
way, all other normal cases get the might_sleep() protection and the
peculiar logic in that code path is explicitly described - win win.

Can you please point me to where the problem was?

Thanks.
Rafael Wysocki Aug. 22, 2011, 6:50 p.m. UTC | #2
On Monday, August 22, 2011, Tejun Heo wrote:
> Hello, Rafael.
> 
> On Sun, Aug 21, 2011 at 08:03:14PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PM / Freezer: Move might_sleep() from try_to_freeze()
> > 
> > There are some code paths that call try_to_freeze() from interrupt
> > context, but doing so they know that the current process cannot
> > possible be freezing (e.g. during reboot on ARM).  However, the
> > recently added might_sleep() annotation in try_to_freeze()
> > triggers in those cases, making it look like there were bugs in
> > those places, which really isn't the case.
> > 
> > Therefore move might_sleep() from try_to_freeze() to
> > __refrigerator() so that it doesn't produce false positives.
> 
> Hmmm... I can't quite agree with this change.  Some invocations of
> try_to_freeze() can be very difficult to trigger.  Freezing isn't a
> frequent operation after some try_to_freeze() can be buried in weird
> places.  might_sleep() is exactly to detect context bugs in these
> situations.  If a code path is called from both sleepable and
> unsleepable context and it knows that the latter wouldn't happen if
> the system is freezing, that code path should conditionalize
> invocation of try_to_freeze() based on its knowledge of context.  That
> way, all other normal cases get the might_sleep() protection and the
> peculiar logic in that code path is explicitly described - win win.
> 
> Can you please point me to where the problem was?

Apparently, during reboot on ARM try_to_freeze() is called via
do_signal() with interrupts disabled.

Thanks,
Rafael
diff mbox

Patch

Index: linux/include/linux/freezer.h
===================================================================
--- linux.orig/include/linux/freezer.h
+++ linux/include/linux/freezer.h
@@ -41,7 +41,6 @@  extern void thaw_processes(void);
 
 static inline bool try_to_freeze(void)
 {
-	might_sleep();
 	if (likely(!freezing(current)))
 		return false;
 	return __refrigerator(false);
Index: linux/kernel/freezer.c
===================================================================
--- linux.orig/kernel/freezer.c
+++ linux/kernel/freezer.c
@@ -54,6 +54,8 @@  bool __refrigerator(bool check_kthr_stop
 	bool was_frozen = false;
 	long save;
 
+	might_sleep();
+
 	/*
 	 * No point in checking freezing() again - the caller already did.
 	 * Proceed to enter FROZEN.