[PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

The [un]lock_system_sleep() APIs were originally introduced to mutually
exclude memory hotplug and hibernation.

Directly using mutex_lock(&pm_mutex) to achieve mutual exclusion with
suspend or hibernation code can lead to freezing failures. However, the
APIs [un]lock_system_sleep() can be safely used to achieve the same, without
causing freezing failures.

So, since it would be beneficial to modify all the existing users of
mutex_lock(&pm_mutex) (in all parts of the kernel), so that they use these
safe APIs intead, make these APIs generic by removing the restriction that
they work only when CONFIG_HIBERNATE_CALLBACKS is set. Moreover, that
restriction didn't buy us anything anyway.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   42 ++++++++++++++++++------------------------
 1 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 1f7fff4..d109847 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -331,6 +331,8 @@ static inline bool system_entering_hibernation(void) { return false; }
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
 
+extern struct mutex pm_mutex;
+
 #ifdef CONFIG_PM_SLEEP
 void save_processor_state(void);
 void restore_processor_state(void);
@@ -351,6 +353,21 @@ extern bool events_check_enabled;
 extern bool pm_wakeup_pending(void);
 extern bool pm_get_wakeup_count(unsigned int *count);
 extern bool pm_save_wakeup_count(unsigned int count);
+
+static inline void lock_system_sleep(void)
+{
+	/* simplified freezer_do_not_count() */
+	current->flags |= PF_FREEZER_SKIP;
+	mutex_lock(&pm_mutex);
+}
+
+static inline void unlock_system_sleep(void)
+{
+	mutex_unlock(&pm_mutex);
+	/* simplified freezer_count() */
+	current->flags &= ~PF_FREEZER_SKIP;
+}
+
 #else /* !CONFIG_PM_SLEEP */
 
 static inline int register_pm_notifier(struct notifier_block *nb)
@@ -366,32 +383,9 @@ static inline int unregister_pm_notifier(struct notifier_block *nb)
 #define pm_notifier(fn, pri)	do { (void)(fn); } while (0)
 
 static inline bool pm_wakeup_pending(void) { return false; }
-#endif /* !CONFIG_PM_SLEEP */
-
-extern struct mutex pm_mutex;
-
-#ifndef CONFIG_HIBERNATE_CALLBACKS
 static inline void lock_system_sleep(void) {}
 static inline void unlock_system_sleep(void) {}
-
-#else
-
-/* Let some subsystems like memory hotadd exclude hibernation */
-
-static inline void lock_system_sleep(void)
-{
-	/* simplified freezer_do_not_count() */
-	current->flags |= PF_FREEZER_SKIP;
-	mutex_lock(&pm_mutex);
-}
-
-static inline void unlock_system_sleep(void)
-{
-	mutex_unlock(&pm_mutex);
-	/* simplified freezer_count() */
-	current->flags &= ~PF_FREEZER_SKIP;
-}
-#endif
+#endif /* !CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_ARCH_SAVE_PAGE_KEYS
 /*

[PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

The [un]lock_system_sleep() APIs were originally introduced to mutually
exclude memory hotplug and hibernation.

Directly using mutex_lock(&pm_mutex) to achieve mutual exclusion with
suspend or hibernation code can lead to freezing failures. However, the
APIs [un]lock_system_sleep() can be safely used to achieve the same, without
causing freezing failures.

So, since it would be beneficial to modify all the existing users of
mutex_lock(&pm_mutex) (in all parts of the kernel), so that they use these
safe APIs intead, make these APIs generic by removing the restriction that
they work only when CONFIG_HIBERNATE_CALLBACKS is set. Moreover, that
restriction didn't buy us anything anyway.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   42 ++++++++++++++++++------------------------
 1 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 1f7fff4..d109847 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -331,6 +331,8 @@ static inline bool system_entering_hibernation(void) { return false; }
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
 
+extern struct mutex pm_mutex;
+
 #ifdef CONFIG_PM_SLEEP
 void save_processor_state(void);
 void restore_processor_state(void);
@@ -351,6 +353,21 @@ extern bool events_check_enabled;
 extern bool pm_wakeup_pending(void);
 extern bool pm_get_wakeup_count(unsigned int *count);
 extern bool pm_save_wakeup_count(unsigned int count);
+
+static inline void lock_system_sleep(void)
+{
+	/* simplified freezer_do_not_count() */
+	current->flags |= PF_FREEZER_SKIP;
+	mutex_lock(&pm_mutex);
+}
+
+static inline void unlock_system_sleep(void)
+{
+	mutex_unlock(&pm_mutex);
+	/* simplified freezer_count() */
+	current->flags &= ~PF_FREEZER_SKIP;
+}
+
 #else /* !CONFIG_PM_SLEEP */
 
 static inline int register_pm_notifier(struct notifier_block *nb)
@@ -366,32 +383,9 @@ static inline int unregister_pm_notifier(struct notifier_block *nb)
 #define pm_notifier(fn, pri)	do { (void)(fn); } while (0)
 
 static inline bool pm_wakeup_pending(void) { return false; }
-#endif /* !CONFIG_PM_SLEEP */
-
-extern struct mutex pm_mutex;
-
-#ifndef CONFIG_HIBERNATE_CALLBACKS
 static inline void lock_system_sleep(void) {}
 static inline void unlock_system_sleep(void) {}
-
-#else
-
-/* Let some subsystems like memory hotadd exclude hibernation */
-
-static inline void lock_system_sleep(void)
-{
-	/* simplified freezer_do_not_count() */
-	current->flags |= PF_FREEZER_SKIP;
-	mutex_lock(&pm_mutex);
-}
-
-static inline void unlock_system_sleep(void)
-{
-	mutex_unlock(&pm_mutex);
-	/* simplified freezer_count() */
-	current->flags &= ~PF_FREEZER_SKIP;
-}
-#endif
+#endif /* !CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_ARCH_SAVE_PAGE_KEYS
 /*


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/3] PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with [un]lock_system_sleep()

[Srivatsa S. Bhat]

Using [un]lock_system_sleep() is safer than directly using mutex_[un]lock()
on 'pm_mutex', since the latter could lead to freezing failures. Hence convert
all the present users of mutex_[un]lock(&pm_mutex) to use these safe APIs
instead.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 kernel/kexec.c           |    4 ++--
 kernel/power/hibernate.c |   16 ++++++++--------
 kernel/power/main.c      |    4 ++--
 kernel/power/suspend.c   |    4 ++--
 kernel/power/user.c      |   16 ++++++++--------
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index dc7bc08..090ee10 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1523,7 +1523,7 @@ int kernel_kexec(void)
 
 #ifdef CONFIG_KEXEC_JUMP
 	if (kexec_image->preserve_context) {
-		mutex_lock(&pm_mutex);
+		lock_system_sleep();
 		pm_prepare_console();
 		error = freeze_processes();
 		if (error) {
@@ -1576,7 +1576,7 @@ int kernel_kexec(void)
 		thaw_processes();
  Restore_console:
 		pm_restore_console();
-		mutex_unlock(&pm_mutex);
+		unlock_system_sleep();
 	}
 #endif
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 605149a..6d6d288 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -69,14 +69,14 @@ void hibernation_set_ops(const struct platform_hibernation_ops *ops)
 		WARN_ON(1);
 		return;
 	}
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	hibernation_ops = ops;
 	if (ops)
 		hibernation_mode = HIBERNATION_PLATFORM;
 	else if (hibernation_mode == HIBERNATION_PLATFORM)
 		hibernation_mode = HIBERNATION_SHUTDOWN;
 
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 }
 
 static bool entering_platform_hibernation;
@@ -597,7 +597,7 @@ int hibernate(void)
 {
 	int error;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	/* The snapshot device should not be opened while we're running */
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
 		error = -EBUSY;
@@ -665,7 +665,7 @@ int hibernate(void)
 	pm_restore_console();
 	atomic_inc(&snapshot_device_available);
  Unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 	return error;
 }
 
@@ -893,7 +893,7 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 	p = memchr(buf, '\n', n);
 	len = p ? p - buf : n;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	for (i = HIBERNATION_FIRST; i <= HIBERNATION_MAX; i++) {
 		if (len == strlen(hibernation_modes[i])
 		    && !strncmp(buf, hibernation_modes[i], len)) {
@@ -919,7 +919,7 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (!error)
 		pr_debug("PM: Hibernation mode set to '%s'\n",
 			 hibernation_modes[mode]);
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 	return error ? error : n;
 }
 
@@ -946,9 +946,9 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (maj != MAJOR(res) || min != MINOR(res))
 		goto out;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	swsusp_resume_device = res;
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 	printk(KERN_INFO "PM: Starting manual resume from disk\n");
 	noresume = 0;
 	software_resume();
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7d36fb3..9824b41e 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -116,7 +116,7 @@ static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
 	p = memchr(buf, '\n', n);
 	len = p ? p - buf : n;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	level = TEST_FIRST;
 	for (s = &pm_tests[level]; level <= TEST_MAX; s++, level++)
@@ -126,7 +126,7 @@ static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
 			break;
 		}
 
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return error ? error : n;
 }
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index d336b27..4fd51be 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -42,9 +42,9 @@ static const struct platform_suspend_ops *suspend_ops;
  */
 void suspend_set_ops(const struct platform_suspend_ops *ops)
 {
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	suspend_ops = ops;
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 }
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 06ea33d..98ade21 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -71,7 +71,7 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	struct snapshot_data *data;
 	int error;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
 		error = -EBUSY;
@@ -123,7 +123,7 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	data->platform_support = 0;
 
  Unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return error;
 }
@@ -132,7 +132,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 {
 	struct snapshot_data *data;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	swsusp_free();
 	free_basic_memory_bitmaps();
@@ -146,7 +146,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 			PM_POST_HIBERNATION : PM_POST_RESTORE);
 	atomic_inc(&snapshot_device_available);
 
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return 0;
 }
@@ -158,7 +158,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	data = filp->private_data;
 	if (!data->ready) {
@@ -179,7 +179,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 		*offp += res;
 
  Unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return res;
 }
@@ -191,7 +191,7 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	data = filp->private_data;
 
@@ -208,7 +208,7 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
 	if (res > 0)
 		*offp += res;
 unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return res;
 }

[PATCH 2/3] PM / Sleep: Replace mutex_[un]lock(&pm_mutex) with [un]lock_system_sleep()

[Srivatsa S. Bhat]

Using [un]lock_system_sleep() is safer than directly using mutex_[un]lock()
on 'pm_mutex', since the latter could lead to freezing failures. Hence convert
all the present users of mutex_[un]lock(&pm_mutex) to use these safe APIs
instead.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 kernel/kexec.c           |    4 ++--
 kernel/power/hibernate.c |   16 ++++++++--------
 kernel/power/main.c      |    4 ++--
 kernel/power/suspend.c   |    4 ++--
 kernel/power/user.c      |   16 ++++++++--------
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index dc7bc08..090ee10 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1523,7 +1523,7 @@ int kernel_kexec(void)
 
 #ifdef CONFIG_KEXEC_JUMP
 	if (kexec_image->preserve_context) {
-		mutex_lock(&pm_mutex);
+		lock_system_sleep();
 		pm_prepare_console();
 		error = freeze_processes();
 		if (error) {
@@ -1576,7 +1576,7 @@ int kernel_kexec(void)
 		thaw_processes();
  Restore_console:
 		pm_restore_console();
-		mutex_unlock(&pm_mutex);
+		unlock_system_sleep();
 	}
 #endif
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 605149a..6d6d288 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -69,14 +69,14 @@ void hibernation_set_ops(const struct platform_hibernation_ops *ops)
 		WARN_ON(1);
 		return;
 	}
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	hibernation_ops = ops;
 	if (ops)
 		hibernation_mode = HIBERNATION_PLATFORM;
 	else if (hibernation_mode == HIBERNATION_PLATFORM)
 		hibernation_mode = HIBERNATION_SHUTDOWN;
 
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 }
 
 static bool entering_platform_hibernation;
@@ -597,7 +597,7 @@ int hibernate(void)
 {
 	int error;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	/* The snapshot device should not be opened while we're running */
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
 		error = -EBUSY;
@@ -665,7 +665,7 @@ int hibernate(void)
 	pm_restore_console();
 	atomic_inc(&snapshot_device_available);
  Unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 	return error;
 }
 
@@ -893,7 +893,7 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 	p = memchr(buf, '\n', n);
 	len = p ? p - buf : n;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	for (i = HIBERNATION_FIRST; i <= HIBERNATION_MAX; i++) {
 		if (len == strlen(hibernation_modes[i])
 		    && !strncmp(buf, hibernation_modes[i], len)) {
@@ -919,7 +919,7 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (!error)
 		pr_debug("PM: Hibernation mode set to '%s'\n",
 			 hibernation_modes[mode]);
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 	return error ? error : n;
 }
 
@@ -946,9 +946,9 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (maj != MAJOR(res) || min != MINOR(res))
 		goto out;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	swsusp_resume_device = res;
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 	printk(KERN_INFO "PM: Starting manual resume from disk\n");
 	noresume = 0;
 	software_resume();
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7d36fb3..9824b41e 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -116,7 +116,7 @@ static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
 	p = memchr(buf, '\n', n);
 	len = p ? p - buf : n;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	level = TEST_FIRST;
 	for (s = &pm_tests[level]; level <= TEST_MAX; s++, level++)
@@ -126,7 +126,7 @@ static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
 			break;
 		}
 
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return error ? error : n;
 }
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index d336b27..4fd51be 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -42,9 +42,9 @@ static const struct platform_suspend_ops *suspend_ops;
  */
 void suspend_set_ops(const struct platform_suspend_ops *ops)
 {
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 	suspend_ops = ops;
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 }
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 06ea33d..98ade21 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -71,7 +71,7 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	struct snapshot_data *data;
 	int error;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
 		error = -EBUSY;
@@ -123,7 +123,7 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	data->platform_support = 0;
 
  Unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return error;
 }
@@ -132,7 +132,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 {
 	struct snapshot_data *data;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	swsusp_free();
 	free_basic_memory_bitmaps();
@@ -146,7 +146,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 			PM_POST_HIBERNATION : PM_POST_RESTORE);
 	atomic_inc(&snapshot_device_available);
 
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return 0;
 }
@@ -158,7 +158,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	data = filp->private_data;
 	if (!data->ready) {
@@ -179,7 +179,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 		*offp += res;
 
  Unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return res;
 }
@@ -191,7 +191,7 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
 
-	mutex_lock(&pm_mutex);
+	lock_system_sleep();
 
 	data = filp->private_data;
 
@@ -208,7 +208,7 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
 	if (res > 0)
 		*offp += res;
 unlock:
-	mutex_unlock(&pm_mutex);
+	unlock_system_sleep();
 
 	return res;
 }


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Srivatsa S. Bhat]

Update the documentation to explain the perils of directly using
mutex_[un]lock(&pm_mutex) and recommend the usage of the safe
APIs [un]lock_system_sleep() instead.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 Documentation/power/freezing-of-tasks.txt |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/freezing-of-tasks.txt b/Documentation/power/freezing-of-tasks.txt
index 3ab9fbd..6ccb68f 100644
--- a/Documentation/power/freezing-of-tasks.txt
+++ b/Documentation/power/freezing-of-tasks.txt
@@ -176,3 +176,28 @@ tasks, since it generally exists anyway.
 A driver must have all firmwares it may need in RAM before suspend() is called.
 If keeping them is not practical, for example due to their size, they must be
 requested early enough using the suspend notifier API described in notifiers.txt.
+
+VI. Are there any precautions to be taken to prevent freezing failures?
+
+Yes, there are.
+
+First of all, grabbing the 'pm_mutex' lock to mutually exclude a piece of code
+from system-wide sleep such as suspend/hibernation is not encouraged.
+If possible, that piece of code must instead hook onto the suspend/hibernation
+notifiers to achieve mutual exclusion. Look at the CPU-Hotplug code
+(kernel/cpu.c) for an example.
+
+However, if that is not feasible, and grabbing 'pm_mutex' is deemed necessary,
+it is strongly discouraged to directly call mutex_[un]lock(&pm_mutex) since
+that could lead to freezing failures, because if the suspend/hibernate code
+successfully acquired the 'pm_mutex' lock, and hence that other entity failed
+to acquire the lock, then that task would get blocked in TASK_UNINTERRUPTIBLE
+state. As a consequence, the freezer would not be able to freeze that task,
+leading to freezing failure.
+
+However, the [un]lock_system_sleep() APIs are safe to use in this scenario,
+since they ask the freezer to skip freezing this task, since it is anyway
+"frozen enough" as it is blocked on 'pm_mutex', which will be released
+only after the entire suspend/hibernation sequence is complete.
+So, to summarize, use [un]lock_system_sleep() instead of directly using
+mutex_[un]lock(&pm_mutex). That would prevent freezing failures.

[PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Srivatsa S. Bhat]

Update the documentation to explain the perils of directly using
mutex_[un]lock(&pm_mutex) and recommend the usage of the safe
APIs [un]lock_system_sleep() instead.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 Documentation/power/freezing-of-tasks.txt |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/freezing-of-tasks.txt b/Documentation/power/freezing-of-tasks.txt
index 3ab9fbd..6ccb68f 100644
--- a/Documentation/power/freezing-of-tasks.txt
+++ b/Documentation/power/freezing-of-tasks.txt
@@ -176,3 +176,28 @@ tasks, since it generally exists anyway.
 A driver must have all firmwares it may need in RAM before suspend() is called.
 If keeping them is not practical, for example due to their size, they must be
 requested early enough using the suspend notifier API described in notifiers.txt.
+
+VI. Are there any precautions to be taken to prevent freezing failures?
+
+Yes, there are.
+
+First of all, grabbing the 'pm_mutex' lock to mutually exclude a piece of code
+from system-wide sleep such as suspend/hibernation is not encouraged.
+If possible, that piece of code must instead hook onto the suspend/hibernation
+notifiers to achieve mutual exclusion. Look at the CPU-Hotplug code
+(kernel/cpu.c) for an example.
+
+However, if that is not feasible, and grabbing 'pm_mutex' is deemed necessary,
+it is strongly discouraged to directly call mutex_[un]lock(&pm_mutex) since
+that could lead to freezing failures, because if the suspend/hibernate code
+successfully acquired the 'pm_mutex' lock, and hence that other entity failed
+to acquire the lock, then that task would get blocked in TASK_UNINTERRUPTIBLE
+state. As a consequence, the freezer would not be able to freeze that task,
+leading to freezing failure.
+
+However, the [un]lock_system_sleep() APIs are safe to use in this scenario,
+since they ask the freezer to skip freezing this task, since it is anyway
+"frozen enough" as it is blocked on 'pm_mutex', which will be released
+only after the entire suspend/hibernation sequence is complete.
+So, to summarize, use [un]lock_system_sleep() instead of directly using
+mutex_[un]lock(&pm_mutex). That would prevent freezing failures.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Tejun Heo]

Hello,

On Mon, Dec 05, 2011 at 01:32:38AM +0530, Srivatsa S. Bhat wrote:
> +static inline void lock_system_sleep(void)
> +{
> +	/* simplified freezer_do_not_count() */
> +	current->flags |= PF_FREEZER_SKIP;
> +	mutex_lock(&pm_mutex);
> +}
> +
> +static inline void unlock_system_sleep(void)
> +{
> +	mutex_unlock(&pm_mutex);
> +	/* simplified freezer_count() */
> +	current->flags &= ~PF_FREEZER_SKIP;
> +}

BTW, don't we want try_to_freeze() there?  What's the reason for not
using freezer_count()?

Thanks.

-- 
tejun

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Tejun Heo]

Hello,

On Mon, Dec 05, 2011 at 01:32:38AM +0530, Srivatsa S. Bhat wrote:
> +static inline void lock_system_sleep(void)
> +{
> +	/* simplified freezer_do_not_count() */
> +	current->flags |= PF_FREEZER_SKIP;
> +	mutex_lock(&pm_mutex);
> +}
> +
> +static inline void unlock_system_sleep(void)
> +{
> +	mutex_unlock(&pm_mutex);
> +	/* simplified freezer_count() */
> +	current->flags &= ~PF_FREEZER_SKIP;
> +}

BTW, don't we want try_to_freeze() there?  What's the reason for not
using freezer_count()?

Thanks.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Tejun Heo]

Hello,

On Mon, Dec 05, 2011 at 01:33:42AM +0530, Srivatsa S. Bhat wrote:
> Update the documentation to explain the perils of directly using
> mutex_[un]lock(&pm_mutex) and recommend the usage of the safe
> APIs [un]lock_system_sleep() instead.

Maybe just make it pm internal?

-- 
tejun

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Tejun Heo]

Hello,

On Mon, Dec 05, 2011 at 01:33:42AM +0530, Srivatsa S. Bhat wrote:
> Update the documentation to explain the perils of directly using
> mutex_[un]lock(&pm_mutex) and recommend the usage of the safe
> APIs [un]lock_system_sleep() instead.

Maybe just make it pm internal?

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

On 12/05/2011 10:44 PM, Tejun Heo wrote:

> Hello,
> 
> On Mon, Dec 05, 2011 at 01:32:38AM +0530, Srivatsa S. Bhat wrote:
>> +static inline void lock_system_sleep(void)
>> +{
>> +	/* simplified freezer_do_not_count() */
>> +	current->flags |= PF_FREEZER_SKIP;
>> +	mutex_lock(&pm_mutex);
>> +}
>> +
>> +static inline void unlock_system_sleep(void)
>> +{
>> +	mutex_unlock(&pm_mutex);
>> +	/* simplified freezer_count() */
>> +	current->flags &= ~PF_FREEZER_SKIP;
>> +}
> 
> BTW, don't we want try_to_freeze() there?  What's the reason for not
> using freezer_count()?
> 


I wanted these APIs to be generic, not restricted to work only for userspace
processes. Both freezer_do_not_count() and freezer_count() are effective only
when current->mm is non-NULL (ie., only for userspace ones).
I think I have documented this in the patch which added these things to the
2 APIs. See commit 6a76b7a in linux-pm/linux-next.

As for try_to_freeze(), we can surely add that. I think I missed it while
open-coding the relevant part of freezer_count(). I'll send it as a separate
patch. Thank you very much.

Regards,
Srivatsa S. Bhat

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

On 12/05/2011 10:44 PM, Tejun Heo wrote:

> Hello,
> 
> On Mon, Dec 05, 2011 at 01:32:38AM +0530, Srivatsa S. Bhat wrote:
>> +static inline void lock_system_sleep(void)
>> +{
>> +	/* simplified freezer_do_not_count() */
>> +	current->flags |= PF_FREEZER_SKIP;
>> +	mutex_lock(&pm_mutex);
>> +}
>> +
>> +static inline void unlock_system_sleep(void)
>> +{
>> +	mutex_unlock(&pm_mutex);
>> +	/* simplified freezer_count() */
>> +	current->flags &= ~PF_FREEZER_SKIP;
>> +}
> 
> BTW, don't we want try_to_freeze() there?  What's the reason for not
> using freezer_count()?
> 


I wanted these APIs to be generic, not restricted to work only for userspace
processes. Both freezer_do_not_count() and freezer_count() are effective only
when current->mm is non-NULL (ie., only for userspace ones).
I think I have documented this in the patch which added these things to the
2 APIs. See commit 6a76b7a in linux-pm/linux-next.

As for try_to_freeze(), we can surely add that. I think I missed it while
open-coding the relevant part of freezer_count(). I'll send it as a separate
patch. Thank you very much.

Regards,
Srivatsa S. Bhat


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Tejun Heo]

(cc'ing Oleg)

On Mon, Dec 05, 2011 at 10:55:51PM +0530, Srivatsa S. Bhat wrote:
> I wanted these APIs to be generic, not restricted to work only for userspace
> processes. Both freezer_do_not_count() and freezer_count() are effective only
> when current->mm is non-NULL (ie., only for userspace ones).
> I think I have documented this in the patch which added these things to the
> 2 APIs. See commit 6a76b7a in linux-pm/linux-next.

I see.  Oleg was curious about the ->mm condition too and IIRC there's
no reason for that restriction.  Maybe removing that in another patch
and using the count functions is better?

Thanks.

-- 
tejun

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Tejun Heo]

(cc'ing Oleg)

On Mon, Dec 05, 2011 at 10:55:51PM +0530, Srivatsa S. Bhat wrote:
> I wanted these APIs to be generic, not restricted to work only for userspace
> processes. Both freezer_do_not_count() and freezer_count() are effective only
> when current->mm is non-NULL (ie., only for userspace ones).
> I think I have documented this in the patch which added these things to the
> 2 APIs. See commit 6a76b7a in linux-pm/linux-next.

I see.  Oleg was curious about the ->mm condition too and IIRC there's
no reason for that restriction.  Maybe removing that in another patch
and using the count functions is better?

Thanks.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Srivatsa S. Bhat]

On 12/05/2011 10:45 PM, Tejun Heo wrote:

> Hello,
> 
> On Mon, Dec 05, 2011 at 01:33:42AM +0530, Srivatsa S. Bhat wrote:
>> Update the documentation to explain the perils of directly using
>> mutex_[un]lock(&pm_mutex) and recommend the usage of the safe
>> APIs [un]lock_system_sleep() instead.
> 
> Maybe just make it pm internal?
> 


Sorry, I didn't get what you meant here. Are you talking about what
needs to be added/changed in the documentation or, are you referring
to the code itself and are saying that we must make these APIs
internal to the PM alone?

If it is the latter, please note that memory hotplug is one of the
outside-of-PM user of these APIs, and hence we really can't make
these APIs internal to the PM alone without substantially needing to
change the memory hotplug code to mutually exclude itself from PM
somehow without using these APIs.
And moreover, since these APIs avoid task freezing failures when
some out-of-PM user like memory hotplug code uses them, I don't see
much benefit by making them internal to the PM. IOW, I don't see
much use left of them, if we do that.

IMHO, we should leave these APIs as they are, until we have a better
solution for out-of-PM users to mutex themselves from PM. (For one,
currently it is not always possible to hook onto PM notifiers to
achieve that, in which case these APIs will come in handy, and these
aren't that ugly anyway, so no harm using them when necessary.)

Regards,
Srivatsa S. Bhat

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Srivatsa S. Bhat]

On 12/05/2011 10:45 PM, Tejun Heo wrote:

> Hello,
> 
> On Mon, Dec 05, 2011 at 01:33:42AM +0530, Srivatsa S. Bhat wrote:
>> Update the documentation to explain the perils of directly using
>> mutex_[un]lock(&pm_mutex) and recommend the usage of the safe
>> APIs [un]lock_system_sleep() instead.
> 
> Maybe just make it pm internal?
> 


Sorry, I didn't get what you meant here. Are you talking about what
needs to be added/changed in the documentation or, are you referring
to the code itself and are saying that we must make these APIs
internal to the PM alone?

If it is the latter, please note that memory hotplug is one of the
outside-of-PM user of these APIs, and hence we really can't make
these APIs internal to the PM alone without substantially needing to
change the memory hotplug code to mutually exclude itself from PM
somehow without using these APIs.
And moreover, since these APIs avoid task freezing failures when
some out-of-PM user like memory hotplug code uses them, I don't see
much benefit by making them internal to the PM. IOW, I don't see
much use left of them, if we do that.

IMHO, we should leave these APIs as they are, until we have a better
solution for out-of-PM users to mutex themselves from PM. (For one,
currently it is not always possible to hook onto PM notifiers to
achieve that, in which case these APIs will come in handy, and these
aren't that ugly anyway, so no harm using them when necessary.)

Regards,
Srivatsa S. Bhat


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

On 12/05/2011 11:00 PM, Tejun Heo wrote:

> (cc'ing Oleg)
> 
> On Mon, Dec 05, 2011 at 10:55:51PM +0530, Srivatsa S. Bhat wrote:
>> I wanted these APIs to be generic, not restricted to work only for userspace
>> processes. Both freezer_do_not_count() and freezer_count() are effective only
>> when current->mm is non-NULL (ie., only for userspace ones).
>> I think I have documented this in the patch which added these things to the
>> 2 APIs. See commit 6a76b7a in linux-pm/linux-next.
> 
> I see.  Oleg was curious about the ->mm condition too and IIRC there's
> no reason for that restriction.  Maybe removing that in another patch
> and using the count functions is better?
> 


Oh well, then yes, that sounds like a better idea. Will send patches for
that. Thank you.

Regards,
Srivatsa S. Bhat

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

On 12/05/2011 11:00 PM, Tejun Heo wrote:

> (cc'ing Oleg)
> 
> On Mon, Dec 05, 2011 at 10:55:51PM +0530, Srivatsa S. Bhat wrote:
>> I wanted these APIs to be generic, not restricted to work only for userspace
>> processes. Both freezer_do_not_count() and freezer_count() are effective only
>> when current->mm is non-NULL (ie., only for userspace ones).
>> I think I have documented this in the patch which added these things to the
>> 2 APIs. See commit 6a76b7a in linux-pm/linux-next.
> 
> I see.  Oleg was curious about the ->mm condition too and IIRC there's
> no reason for that restriction.  Maybe removing that in another patch
> and using the count functions is better?
> 


Oh well, then yes, that sounds like a better idea. Will send patches for
that. Thank you.

Regards,
Srivatsa S. Bhat


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Tejun Heo]

Hello,

On Mon, Dec 05, 2011 at 11:08:38PM +0530, Srivatsa S. Bhat wrote:
> Sorry, I didn't get what you meant here. Are you talking about what
> needs to be added/changed in the documentation or, are you referring
> to the code itself and are saying that we must make these APIs
> internal to the PM alone?

Ooh, sorry about not being clear.  I meant pm_mutex itself.  There's
no reason to expose that outside of pm, right?  And in the
documentation, we can just require use of the APIs instead of pm_mutex
itself.

Thank you.

-- 
tejun

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Tejun Heo]

Hello,

On Mon, Dec 05, 2011 at 11:08:38PM +0530, Srivatsa S. Bhat wrote:
> Sorry, I didn't get what you meant here. Are you talking about what
> needs to be added/changed in the documentation or, are you referring
> to the code itself and are saying that we must make these APIs
> internal to the PM alone?

Ooh, sorry about not being clear.  I meant pm_mutex itself.  There's
no reason to expose that outside of pm, right?  And in the
documentation, we can just require use of the APIs instead of pm_mutex
itself.

Thank you.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Srivatsa S. Bhat]

On 12/05/2011 11:13 PM, Tejun Heo wrote:

> Hello,
> 
> On Mon, Dec 05, 2011 at 11:08:38PM +0530, Srivatsa S. Bhat wrote:
>> Sorry, I didn't get what you meant here. Are you talking about what
>> needs to be added/changed in the documentation or, are you referring
>> to the code itself and are saying that we must make these APIs
>> internal to the PM alone?
> 
> Ooh, sorry about not being clear.  I meant pm_mutex itself.  There's
> no reason to expose that outside of pm, right?  And in the
> documentation, we can just require use of the APIs instead of pm_mutex
> itself.
> 


Yes, that sounds good. No need for giving unnecessary choices :-)
But I had worded the documentation that way with the intention of
explaining why calling mutex_lock() over pm_mutex can be disastrous (which
I mentioned in the commit message as one of the goals of the patch).
I didn't mean it to give the user 2 choices and say please use
[un]lock_system_sleep() preferably.

Although, we have to notice that unless somebody is acquainted with
these APIs, the first instinct would probably be to directly use
mutex_lock(), until they look up the documentation (hopefully).
So, IMHO, it would do good to keep the explanation in the docs as
it is, in this patch. What do you think?

Regards,
Srivatsa S. Bhat

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Srivatsa S. Bhat]

On 12/05/2011 11:13 PM, Tejun Heo wrote:

> Hello,
> 
> On Mon, Dec 05, 2011 at 11:08:38PM +0530, Srivatsa S. Bhat wrote:
>> Sorry, I didn't get what you meant here. Are you talking about what
>> needs to be added/changed in the documentation or, are you referring
>> to the code itself and are saying that we must make these APIs
>> internal to the PM alone?
> 
> Ooh, sorry about not being clear.  I meant pm_mutex itself.  There's
> no reason to expose that outside of pm, right?  And in the
> documentation, we can just require use of the APIs instead of pm_mutex
> itself.
> 


Yes, that sounds good. No need for giving unnecessary choices :-)
But I had worded the documentation that way with the intention of
explaining why calling mutex_lock() over pm_mutex can be disastrous (which
I mentioned in the commit message as one of the goals of the patch).
I didn't mean it to give the user 2 choices and say please use
[un]lock_system_sleep() preferably.

Although, we have to notice that unless somebody is acquainted with
these APIs, the first instinct would probably be to directly use
mutex_lock(), until they look up the documentation (hopefully).
So, IMHO, it would do good to keep the explanation in the docs as
it is, in this patch. What do you think?

Regards,
Srivatsa S. Bhat


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Tejun Heo]

On Mon, Dec 05, 2011 at 11:26:22PM +0530, Srivatsa S. Bhat wrote:
> Yes, that sounds good. No need for giving unnecessary choices :-)
> But I had worded the documentation that way with the intention of
> explaining why calling mutex_lock() over pm_mutex can be disastrous (which
> I mentioned in the commit message as one of the goals of the patch).
> I didn't mean it to give the user 2 choices and say please use
> [un]lock_system_sleep() preferably.
> 
> Although, we have to notice that unless somebody is acquainted with
> these APIs, the first instinct would probably be to directly use
> mutex_lock(), until they look up the documentation (hopefully).
> So, IMHO, it would do good to keep the explanation in the docs as
> it is, in this patch. What do you think?

Yeah, sounds good to me.

Thanks.

-- 
tejun

Re: [PATCH 3/3] PM / Docs: Recommend the use of [un]lock_system_sleep() over mutex_[un]lock(&pm_mutex)

[Tejun Heo]

On Mon, Dec 05, 2011 at 11:26:22PM +0530, Srivatsa S. Bhat wrote:
> Yes, that sounds good. No need for giving unnecessary choices :-)
> But I had worded the documentation that way with the intention of
> explaining why calling mutex_lock() over pm_mutex can be disastrous (which
> I mentioned in the commit message as one of the goals of the patch).
> I didn't mean it to give the user 2 choices and say please use
> [un]lock_system_sleep() preferably.
> 
> Although, we have to notice that unless somebody is acquainted with
> these APIs, the first instinct would probably be to directly use
> mutex_lock(), until they look up the documentation (hopefully).
> So, IMHO, it would do good to keep the explanation in the docs as
> it is, in this patch. What do you think?

Yeah, sounds good to me.

Thanks.

-- 
tejun

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

On 12/05/2011 11:11 PM, Srivatsa S. Bhat wrote:

> On 12/05/2011 11:00 PM, Tejun Heo wrote:
> 
>> (cc'ing Oleg)
>>
>> On Mon, Dec 05, 2011 at 10:55:51PM +0530, Srivatsa S. Bhat wrote:
>>> I wanted these APIs to be generic, not restricted to work only for userspace
>>> processes. Both freezer_do_not_count() and freezer_count() are effective only
>>> when current->mm is non-NULL (ie., only for userspace ones).
>>> I think I have documented this in the patch which added these things to the
>>> 2 APIs. See commit 6a76b7a in linux-pm/linux-next.
>>
>> I see.  Oleg was curious about the ->mm condition too and IIRC there's
>> no reason for that restriction.  Maybe removing that in another patch
>> and using the count functions is better?
>>
> 
> 
> Oh well, then yes, that sounds like a better idea. Will send patches for
> that. Thank you.
> 


I looked up in git and found that commit ba96a0c by Rafael introduced the
count functions, to handle the vfork case. But now, we seem to have more
uses than that. So I think we can remove that userspace restriction in the
count functions, and in kernel/fork.c, do something like:

if (current->mm)
	freezer_do_not_count();
...
if (current->mm)
	freezer_count();

This way, it wouldn't break anything, since functionality-wise it is equivalent
to the existing code; And we get what we want: ability to directly use the
count functions elsewhere.

Regards,
Srivatsa S. Bhat

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

On 12/05/2011 11:11 PM, Srivatsa S. Bhat wrote:

> On 12/05/2011 11:00 PM, Tejun Heo wrote:
> 
>> (cc'ing Oleg)
>>
>> On Mon, Dec 05, 2011 at 10:55:51PM +0530, Srivatsa S. Bhat wrote:
>>> I wanted these APIs to be generic, not restricted to work only for userspace
>>> processes. Both freezer_do_not_count() and freezer_count() are effective only
>>> when current->mm is non-NULL (ie., only for userspace ones).
>>> I think I have documented this in the patch which added these things to the
>>> 2 APIs. See commit 6a76b7a in linux-pm/linux-next.
>>
>> I see.  Oleg was curious about the ->mm condition too and IIRC there's
>> no reason for that restriction.  Maybe removing that in another patch
>> and using the count functions is better?
>>
> 
> 
> Oh well, then yes, that sounds like a better idea. Will send patches for
> that. Thank you.
> 


I looked up in git and found that commit ba96a0c by Rafael introduced the
count functions, to handle the vfork case. But now, we seem to have more
uses than that. So I think we can remove that userspace restriction in the
count functions, and in kernel/fork.c, do something like:

if (current->mm)
	freezer_do_not_count();
...
if (current->mm)
	freezer_count();

This way, it wouldn't break anything, since functionality-wise it is equivalent
to the existing code; And we get what we want: ability to directly use the
count functions elsewhere.

Regards,
Srivatsa S. Bhat


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Oleg Nesterov]

On 12/05, Srivatsa S. Bhat wrote:
>
> I looked up in git and found that commit ba96a0c by Rafael introduced the
> count functions, to handle the vfork case. But now, we seem to have more
> uses than that. So I think we can remove that userspace restriction in the
> count functions,

Agreed.

> and in kernel/fork.c, do something like:
>
> if (current->mm)
> 	freezer_do_not_count();
> ...
> if (current->mm)
> 	freezer_count();

see http://marc.info/?l=linux-kernel&m=132033335507261

I think this is not needed, we can just remove the ->mm check.
CLONE_VFORK is not used by a freezable kthread.

Oleg.

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Oleg Nesterov]

On 12/05, Srivatsa S. Bhat wrote:
>
> I looked up in git and found that commit ba96a0c by Rafael introduced the
> count functions, to handle the vfork case. But now, we seem to have more
> uses than that. So I think we can remove that userspace restriction in the
> count functions,

Agreed.

> and in kernel/fork.c, do something like:
>
> if (current->mm)
> 	freezer_do_not_count();
> ...
> if (current->mm)
> 	freezer_count();

see http://marc.info/?l=linux-kernel&m=132033335507261

I think this is not needed, we can just remove the ->mm check.
CLONE_VFORK is not used by a freezable kthread.

Oleg.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

Hi Oleg,

On 12/06/2011 12:05 AM, Oleg Nesterov wrote:

> On 12/05, Srivatsa S. Bhat wrote:
>>
>> I looked up in git and found that commit ba96a0c by Rafael introduced the
>> count functions, to handle the vfork case. But now, we seem to have more
>> uses than that. So I think we can remove that userspace restriction in the
>> count functions,
> 
> Agreed.
> 
>> and in kernel/fork.c, do something like:
>>
>> if (current->mm)
>> 	freezer_do_not_count();
>> ...
>> if (current->mm)
>> 	freezer_count();
> 
> see http://marc.info/?l=linux-kernel&m=132033335507261
> 
> I think this is not needed, we can just remove the ->mm check.
> CLONE_VFORK is not used by a freezable kthread.
> 


Great! Thanks for the pointer. I'll send a patch to remove that check.

Regards,
Srivatsa S. Bhat

Re: [PATCH 1/3] PM / Sleep: Make [un]lock_system_sleep() generic

[Srivatsa S. Bhat]

Hi Oleg,

On 12/06/2011 12:05 AM, Oleg Nesterov wrote:

> On 12/05, Srivatsa S. Bhat wrote:
>>
>> I looked up in git and found that commit ba96a0c by Rafael introduced the
>> count functions, to handle the vfork case. But now, we seem to have more
>> uses than that. So I think we can remove that userspace restriction in the
>> count functions,
> 
> Agreed.
> 
>> and in kernel/fork.c, do something like:
>>
>> if (current->mm)
>> 	freezer_do_not_count();
>> ...
>> if (current->mm)
>> 	freezer_count();
> 
> see http://marc.info/?l=linux-kernel&m=132033335507261
> 
> I think this is not needed, we can just remove the ->mm check.
> CLONE_VFORK is not used by a freezable kthread.
> 


Great! Thanks for the pointer. I'll send a patch to remove that check.

Regards,
Srivatsa S. Bhat


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec