| Message ID | 1434885634-19895-2-git-send-email-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sunday, June 21, 2015 01:20:32 PM Pali Rohár wrote: > To prevent race conditions on userspace processes with I/O some taks must be > called after processes are freezed. This patch adds new events which are > delivered by pm_notifier_call_chain() after freezing processes when doing > suspend or hibernate action. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > --- > include/linux/suspend.h | 2 ++ > kernel/power/hibernate.c | 2 ++ > kernel/power/suspend.c | 4 +++- > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index 5efe743..bc743c8 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; } > #define PM_POST_SUSPEND 0x0004 /* Suspend finished */ > #define PM_RESTORE_PREPARE 0x0005 /* Going to restore a saved image */ > #define PM_POST_RESTORE 0x0006 /* Restore failed */ > +#define PM_HIBERNATION_AFTER_FREEZE 0x0007 /* After hibernation freeze */ > +#define PM_SUSPEND_AFTER_FREEZE 0x0008 /* After suspend freeze */ > > extern struct mutex pm_mutex; > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index 2329daa..184f7ee 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -671,6 +671,8 @@ int hibernate(void) > if (error) > goto Exit; > > + pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE); Don't we need to check errors from these? Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too for symmetry. > + > lock_device_hotplug(); > /* Allocate memory management structures */ > error = create_basic_memory_bitmaps(); > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > index 8d7a1ef..ba2a945 100644 > --- a/kernel/power/suspend.c > +++ b/kernel/power/suspend.c > @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state) > trace_suspend_resume(TPS("freeze_processes"), 0, true); > error = suspend_freeze_processes(); > trace_suspend_resume(TPS("freeze_processes"), 0, false); > - if (!error) > + if (!error) { > + pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE); > return 0; > + } > > suspend_stats.failed_freeze++; > dpm_save_failed_step(SUSPEND_FREEZE); >
On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: > On Sunday, June 21, 2015 01:20:32 PM Pali Rohár wrote: > > To prevent race conditions on userspace processes with I/O some taks must be > > called after processes are freezed. This patch adds new events which are > > delivered by pm_notifier_call_chain() after freezing processes when doing > > suspend or hibernate action. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > --- > > include/linux/suspend.h | 2 ++ > > kernel/power/hibernate.c | 2 ++ > > kernel/power/suspend.c | 4 +++- > > 3 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > > index 5efe743..bc743c8 100644 > > --- a/include/linux/suspend.h > > +++ b/include/linux/suspend.h > > @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; } > > #define PM_POST_SUSPEND 0x0004 /* Suspend finished */ > > #define PM_RESTORE_PREPARE 0x0005 /* Going to restore a saved image */ > > #define PM_POST_RESTORE 0x0006 /* Restore failed */ > > +#define PM_HIBERNATION_AFTER_FREEZE 0x0007 /* After hibernation freeze */ > > +#define PM_SUSPEND_AFTER_FREEZE 0x0008 /* After suspend freeze */ > > > > extern struct mutex pm_mutex; > > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > > index 2329daa..184f7ee 100644 > > --- a/kernel/power/hibernate.c > > +++ b/kernel/power/hibernate.c > > @@ -671,6 +671,8 @@ int hibernate(void) > > if (error) > > goto Exit; > > > > + pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE); > > Don't we need to check errors from these? > If yes, what to do in this case? Fail hibernation and goto Exit? > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too > for symmetry. > But there is no use case for BEFORE_THAW. At least it is not needed for now. > > + > > lock_device_hotplug(); > > /* Allocate memory management structures */ > > error = create_basic_memory_bitmaps(); > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > > index 8d7a1ef..ba2a945 100644 > > --- a/kernel/power/suspend.c > > +++ b/kernel/power/suspend.c > > @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state) > > trace_suspend_resume(TPS("freeze_processes"), 0, true); > > error = suspend_freeze_processes(); > > trace_suspend_resume(TPS("freeze_processes"), 0, false); > > - if (!error) > > + if (!error) { > > + pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE); > > return 0; > > + } > > > > suspend_stats.failed_freeze++; > > dpm_save_failed_step(SUSPEND_FREEZE); > > >
On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: > > On Sunday, June 21, 2015 01:20:32 PM Pali Rohár wrote: > > > To prevent race conditions on userspace processes with I/O some taks must be > > > called after processes are freezed. This patch adds new events which are > > > delivered by pm_notifier_call_chain() after freezing processes when doing > > > suspend or hibernate action. > > > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > > --- > > > include/linux/suspend.h | 2 ++ > > > kernel/power/hibernate.c | 2 ++ > > > kernel/power/suspend.c | 4 +++- > > > 3 files changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > > > index 5efe743..bc743c8 100644 > > > --- a/include/linux/suspend.h > > > +++ b/include/linux/suspend.h > > > @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; } > > > #define PM_POST_SUSPEND 0x0004 /* Suspend finished */ > > > #define PM_RESTORE_PREPARE 0x0005 /* Going to restore a saved image */ > > > #define PM_POST_RESTORE 0x0006 /* Restore failed */ > > > +#define PM_HIBERNATION_AFTER_FREEZE 0x0007 /* After hibernation freeze */ > > > +#define PM_SUSPEND_AFTER_FREEZE 0x0008 /* After suspend freeze */ > > > > > > extern struct mutex pm_mutex; > > > > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > > > index 2329daa..184f7ee 100644 > > > --- a/kernel/power/hibernate.c > > > +++ b/kernel/power/hibernate.c > > > @@ -671,6 +671,8 @@ int hibernate(void) > > > if (error) > > > goto Exit; > > > > > > + pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE); > > > > Don't we need to check errors from these? > > > > If yes, what to do in this case? Fail hibernation and goto Exit? Yes, fail the transition in progress. > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too > > for symmetry. > > > > But there is no use case for BEFORE_THAW. At least it is not needed for now. For your use case, a single function pointer would be sufficient too. > > > + > > > lock_device_hotplug(); > > > /* Allocate memory management structures */ > > > error = create_basic_memory_bitmaps(); > > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > > > index 8d7a1ef..ba2a945 100644 > > > --- a/kernel/power/suspend.c > > > +++ b/kernel/power/suspend.c > > > @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state) > > > trace_suspend_resume(TPS("freeze_processes"), 0, true); > > > error = suspend_freeze_processes(); > > > trace_suspend_resume(TPS("freeze_processes"), 0, false); > > > - if (!error) > > > + if (!error) { > > > + pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE); > > > return 0; > > > + } > > > > > > suspend_stats.failed_freeze++; > > > dpm_save_failed_step(SUSPEND_FREEZE); > > > > > > >
On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote: > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too > > > for symmetry. > > > > > > > But there is no use case for BEFORE_THAW. At least it is not needed for now. > > For your use case, a single function pointer would be sufficient too. > What do you mean by single function pointer? kernel/power is part of kernel image and dm-crypt is external kernel module.
On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote: > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote: > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too > > > > for symmetry. > > > > > > > > > > But there is no use case for BEFORE_THAW. At least it is not needed for now. > > > > For your use case, a single function pointer would be sufficient too. > > > > What do you mean by single function pointer? kernel/power is part of > kernel image and dm-crypt is external kernel module. Well, if there is a function pointer in the core suspend code initially set to NULL and exported to modules such that the dm-crypt code can set it to something else, that should be sufficient, shouldn't it? So if you're adding new PM notifier events, that's already more than *you* need. Anyway, I guess the "post freeze" new one should be enough for now, but please change the name to POST_FREEZE. Also I think we don't need separate "post freeze" events for suspend and hibernation. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote: > > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote: > > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: > > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: > > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too > > > > > for symmetry. > > > > > > > > > > > > > But there is no use case for BEFORE_THAW. At least it is not needed for now. > > > > > > For your use case, a single function pointer would be sufficient too. > > > > > > > What do you mean by single function pointer? kernel/power is part of > > kernel image and dm-crypt is external kernel module. > > Well, if there is a function pointer in the core suspend code initially set to > NULL and exported to modules such that the dm-crypt code can set it to > something else, that should be sufficient, shouldn't it? As long as the dm-crypt module is never unloaded. And as long as no other module could very possible want functionality like this. Ever. If a module wants to be notified - the providing a notifier chain really seems like the right thing to do... NeilBrown > > So if you're adding new PM notifier events, that's already more than *you* need. > > Anyway, I guess the "post freeze" new one should be enough for now, but please > change the name to POST_FREEZE. > > Also I think we don't need separate "post freeze" events for suspend and > hibernation. > > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Neil, On Wed, Jul 22, 2015 at 12:08 AM, NeilBrown <neilb@suse.com> wrote: > On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki" > <rjw@rjwysocki.net> wrote: > >> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote: >> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote: >> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: >> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: >> > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too >> > > > > for symmetry. >> > > > > >> > > > >> > > > But there is no use case for BEFORE_THAW. At least it is not needed for now. >> > > >> > > For your use case, a single function pointer would be sufficient too. >> > > >> > >> > What do you mean by single function pointer? kernel/power is part of >> > kernel image and dm-crypt is external kernel module. >> >> Well, if there is a function pointer in the core suspend code initially set to >> NULL and exported to modules such that the dm-crypt code can set it to >> something else, that should be sufficient, shouldn't it? > > As long as the dm-crypt module is never unloaded. OK, there is a race related to that. > And as long as no other module could very possible want functionality like this. Ever. The point was that there were no other users currently, so dm-crypt is going to be the only user for the time being. > If a module wants to be notified - the providing a notifier chain > really seems like the right thing to do... Well, so please see my last response in this thread. :-) Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 22, 2015 at 1:00 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > Hi Neil, > > On Wed, Jul 22, 2015 at 12:08 AM, NeilBrown <neilb@suse.com> wrote: >> On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki" >> <rjw@rjwysocki.net> wrote: >> >>> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote: >>> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote: >>> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: >>> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: >>> > > > > Also, if you're adding AFTER_FREEZE, it would be good to add BEFORE_THAW too >>> > > > > for symmetry. >>> > > > > >>> > > > >>> > > > But there is no use case for BEFORE_THAW. At least it is not needed for now. >>> > > >>> > > For your use case, a single function pointer would be sufficient too. >>> > > >>> > >>> > What do you mean by single function pointer? kernel/power is part of >>> > kernel image and dm-crypt is external kernel module. >>> >>> Well, if there is a function pointer in the core suspend code initially set to >>> NULL and exported to modules such that the dm-crypt code can set it to >>> something else, that should be sufficient, shouldn't it? >> >> As long as the dm-crypt module is never unloaded. > > OK, there is a race related to that. > >> And as long as no other module could very possible want functionality like this. Ever. > > The point was that there were no other users currently, so dm-crypt is > going to be the only user for the time being. > >> If a module wants to be notified - the providing a notifier chain >> really seems like the right thing to do... > > Well, so please see my last response in this thread. :-) So it was below: "Anyway, I guess the "post freeze" new one should be enough for now" which doesn't mean I'm really against the notifier. Or at least it is not supposed to mean so. If there's any confusion related to that, I'm sorry. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 22 July 2015 01:03:23 Rafael J. Wysocki wrote: > On Wed, Jul 22, 2015 at 1:00 AM, Rafael J. Wysocki <rafael@kernel.org> > wrote: > > Hi Neil, > > > > On Wed, Jul 22, 2015 at 12:08 AM, NeilBrown <neilb@suse.com> wrote: > >> On Mon, 20 Jul 2015 23:46:32 +0200 "Rafael J. Wysocki" > >> > >> <rjw@rjwysocki.net> wrote: > >>> On Monday, July 20, 2015 09:32:26 AM Pali Rohár wrote: > >>> > On Saturday 18 July 2015 01:27:15 Rafael J. Wysocki wrote: > >>> > > On Thursday, July 16, 2015 09:33:02 AM Pali Rohár wrote: > >>> > > > On Thursday 16 July 2015 03:02:03 Rafael J. Wysocki wrote: > >>> > > > > Also, if you're adding AFTER_FREEZE, it would be good to > >>> > > > > add BEFORE_THAW too for symmetry. > >>> > > > > >>> > > > But there is no use case for BEFORE_THAW. At least it is > >>> > > > not needed for now. > >>> > > > >>> > > For your use case, a single function pointer would be > >>> > > sufficient too. > >>> > > >>> > What do you mean by single function pointer? kernel/power is > >>> > part of kernel image and dm-crypt is external kernel module. > >>> > >>> Well, if there is a function pointer in the core suspend code > >>> initially set to NULL and exported to modules such that the > >>> dm-crypt code can set it to something else, that should be > >>> sufficient, shouldn't it? > >> > >> As long as the dm-crypt module is never unloaded. > > > > OK, there is a race related to that. > > > >> And as long as no other module could very possible want > >> functionality like this. Ever. > > > > The point was that there were no other users currently, so dm-crypt > > is going to be the only user for the time being. > > > >> If a module wants to be notified - the providing a notifier chain > >> really seems like the right thing to do... > > > > Well, so please see my last response in this thread. :-) > > So it was below: "Anyway, I guess the "post freeze" new one should be > enough for now" which doesn't mean I'm really against the notifier. > Or at least it is not supposed to mean so. If there's any confusion > related to that, I'm sorry. > > Thanks, > Rafael In that case we are not able to distinguish if computer is going to be hibernated or just suspended to RAM. If we have both notifier (one for suspend and for hibernate) then different actions can be configured for suspend and hibernate. And it makes sense to configure different behaviour for suspend and for hibernate. E.g. when you have encrypted partition where is stored hibernation image then you do not have to wipe keys before going to do hibernation. But for suspend to RAM you may want to wipe them.
diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 5efe743..bc743c8 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; } #define PM_POST_SUSPEND 0x0004 /* Suspend finished */ #define PM_RESTORE_PREPARE 0x0005 /* Going to restore a saved image */ #define PM_POST_RESTORE 0x0006 /* Restore failed */ +#define PM_HIBERNATION_AFTER_FREEZE 0x0007 /* After hibernation freeze */ +#define PM_SUSPEND_AFTER_FREEZE 0x0008 /* After suspend freeze */ extern struct mutex pm_mutex; diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index 2329daa..184f7ee 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -671,6 +671,8 @@ int hibernate(void) if (error) goto Exit; + pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE); + lock_device_hotplug(); /* Allocate memory management structures */ error = create_basic_memory_bitmaps(); diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 8d7a1ef..ba2a945 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -277,8 +277,10 @@ static int suspend_prepare(suspend_state_t state) trace_suspend_resume(TPS("freeze_processes"), 0, true); error = suspend_freeze_processes(); trace_suspend_resume(TPS("freeze_processes"), 0, false); - if (!error) + if (!error) { + pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE); return 0; + } suspend_stats.failed_freeze++; dpm_save_failed_step(SUSPEND_FREEZE);
To prevent race conditions on userspace processes with I/O some taks must be called after processes are freezed. This patch adds new events which are delivered by pm_notifier_call_chain() after freezing processes when doing suspend or hibernate action. Signed-off-by: Pali Rohár <pali.rohar@gmail.com> --- include/linux/suspend.h | 2 ++ kernel/power/hibernate.c | 2 ++ kernel/power/suspend.c | 4 +++- 3 files changed, 7 insertions(+), 1 deletion(-)