| Message ID | 201108212003.14722.rjw@sisk.pl (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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
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.