linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] sched/wait, staging/android: simplification and optimization of freeze related code
@ 2019-02-01  5:37 Hugo Lefeuvre
  2019-02-01  5:38 ` [PATCH 1/3] sched/wait: use freezable_schedule when possible Hugo Lefeuvre
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-01  5:37 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Greg Hartman, Alistair Strachan, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Christian Brauner, Ingo Molnar,
	Peter Zijlstra, devel, linux-kernel, Joel Fernandes

This patchset changes the wait api to use freezable_schedule when
possible and adds a new wait_event_freezable_hrtimeout method.

wait_event_freezable_hrtimeout is then used to greatly simplify
handle_vsoc_cond_wait in the android vsoc driver.

This reduces the size of the vsoc driver and allows for potential
performance gain during freeze in the wait api.

This is a follow up of my previous patch "sched/wait: introduce
wait_event_freezable_hrtimeout"[0]. More information related to the
performance gain by using freezable_schedule can be found in the
previous discussion[1].

[0] https://lkml.org/lkml/2019/1/17/877
[1] https://lkml.org/lkml/2019/1/19/58

Hugo Lefeuvre (3):
  sched/wait: use freezable_schedule when possible
  sched/wait: introduce wait_event_freezable_hrtimeout
  staging/android: simplify handle_vsoc_cond_wait

 drivers/staging/android/vsoc.c | 69 +++++-----------------------------
 include/linux/wait.h           | 31 +++++++++++----
 2 files changed, 34 insertions(+), 66 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-01  5:37 [PATCH 0/3] sched/wait, staging/android: simplification and optimization of freeze related code Hugo Lefeuvre
@ 2019-02-01  5:38 ` Hugo Lefeuvre
  2019-02-02 18:37   ` Joel Fernandes
  2019-02-05 16:37   ` Joel Fernandes
  2019-02-01  5:38 ` [PATCH 2/3] sched/wait: introduce wait_event_freezable_hrtimeout Hugo Lefeuvre
  2019-02-01  5:39 ` [PATCH 3/3] staging/android: simplify handle_vsoc_cond_wait Hugo Lefeuvre
  2 siblings, 2 replies; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-01  5:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Greg Hartman, Alistair Strachan, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Christian Brauner, Ingo Molnar,
	Peter Zijlstra, devel, linux-kernel, Joel Fernandes

Replace schedule(); try_to_freeze() by freezable_schedule().

Tasks calling freezable_schedule() set the PF_FREEZER_SKIP flag
before calling schedule(). Unlike tasks calling schedule();
try_to_freeze() tasks calling freezable_schedule() are not awaken by
try_to_freeze_tasks(). Instead they call try_to_freeze() when they
wake up if the freeze is still underway.

It is not a problem since sleeping tasks can't do anything which isn't
allowed for a frozen task while sleeping.

The result is a potential performance gain during freeze, since less
tasks have to be awaken.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 include/linux/wait.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index ed7c122cb31f..5f3efabc36f4 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -308,7 +308,7 @@ do {										\
 
 #define __wait_event_freezable(wq_head, condition)				\
 	___wait_event(wq_head, condition, TASK_INTERRUPTIBLE, 0, 0,		\
-			    schedule(); try_to_freeze())
+			    freezable_schedule())
 
 /**
  * wait_event_freezable - sleep (or freeze) until a condition gets true
@@ -367,7 +367,7 @@ do {										\
 #define __wait_event_freezable_timeout(wq_head, condition, timeout)		\
 	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
 		      TASK_INTERRUPTIBLE, 0, timeout,				\
-		      __ret = schedule_timeout(__ret); try_to_freeze())
+		      __ret = freezable_schedule_timeout(__ret))
 
 /*
  * like wait_event_timeout() -- except it uses TASK_INTERRUPTIBLE to avoid
@@ -588,7 +588,7 @@ do {										\
 
 #define __wait_event_freezable_exclusive(wq, condition)				\
 	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 1, 0,			\
-			schedule(); try_to_freeze())
+			freezable_schedule())
 
 #define wait_event_freezable_exclusive(wq, condition)				\
 ({										\
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] sched/wait: introduce wait_event_freezable_hrtimeout
  2019-02-01  5:37 [PATCH 0/3] sched/wait, staging/android: simplification and optimization of freeze related code Hugo Lefeuvre
  2019-02-01  5:38 ` [PATCH 1/3] sched/wait: use freezable_schedule when possible Hugo Lefeuvre
@ 2019-02-01  5:38 ` Hugo Lefeuvre
  2019-02-05 16:42   ` Joel Fernandes
  2019-02-01  5:39 ` [PATCH 3/3] staging/android: simplify handle_vsoc_cond_wait Hugo Lefeuvre
  2 siblings, 1 reply; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-01  5:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Greg Hartman, Alistair Strachan, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Christian Brauner, Ingo Molnar,
	Peter Zijlstra, devel, linux-kernel, Joel Fernandes

introduce wait_event_freezable_hrtimeout, an interruptible and freezable
version of wait_event_hrtimeout.

Among others this helper will allow for simplifications in
staging/android/vsoc.c.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 include/linux/wait.h | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 5f3efabc36f4..c4cf5113f58a 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -483,7 +483,7 @@ do {										\
 	__ret;									\
 })
 
-#define __wait_event_hrtimeout(wq_head, condition, timeout, state)		\
+#define __wait_event_hrtimeout(wq_head, condition, timeout, state, cmd)		\
 ({										\
 	int __ret = 0;								\
 	struct hrtimer_sleeper __t;						\
@@ -500,7 +500,7 @@ do {										\
 			__ret = -ETIME;						\
 			break;							\
 		}								\
-		schedule());							\
+		cmd);							        \
 										\
 	hrtimer_cancel(&__t.timer);						\
 	destroy_hrtimer_on_stack(&__t.timer);					\
@@ -529,7 +529,23 @@ do {										\
 	might_sleep();								\
 	if (!(condition))							\
 		__ret = __wait_event_hrtimeout(wq_head, condition, timeout,	\
-					       TASK_UNINTERRUPTIBLE);		\
+					       TASK_UNINTERRUPTIBLE,		\
+					       schedule());			\
+	__ret;									\
+})
+
+/*
+ * like wait_event_hrtimeout() -- except it uses TASK_INTERRUPTIBLE to avoid
+ * increasing load and is freezable.
+ */
+#define wait_event_freezable_hrtimeout(wq_head, condition, timeout)		\
+({										\
+	int __ret = 0;								\
+	might_sleep();								\
+	if (!(condition))							\
+		__ret = __wait_event_hrtimeout(wq_head, condition, timeout,	\
+					       TASK_INTERRUPTIBLE,		\
+					       freezable_schedule());		\
 	__ret;									\
 })
 
@@ -555,7 +571,8 @@ do {										\
 	might_sleep();								\
 	if (!(condition))							\
 		__ret = __wait_event_hrtimeout(wq, condition, timeout,		\
-					       TASK_INTERRUPTIBLE);		\
+					       TASK_INTERRUPTIBLE,		\
+					       schedule());			\
 	__ret;									\
 })
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] staging/android: simplify handle_vsoc_cond_wait
  2019-02-01  5:37 [PATCH 0/3] sched/wait, staging/android: simplification and optimization of freeze related code Hugo Lefeuvre
  2019-02-01  5:38 ` [PATCH 1/3] sched/wait: use freezable_schedule when possible Hugo Lefeuvre
  2019-02-01  5:38 ` [PATCH 2/3] sched/wait: introduce wait_event_freezable_hrtimeout Hugo Lefeuvre
@ 2019-02-01  5:39 ` Hugo Lefeuvre
  2019-02-04 15:52   ` Joel Fernandes
  2 siblings, 1 reply; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-01  5:39 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Greg Hartman, Alistair Strachan, Arve Hjønnevåg,
	Todd Kjos, Martijn Coenen, Christian Brauner, Ingo Molnar,
	Peter Zijlstra, devel, linux-kernel, Joel Fernandes

simplify handle_vsoc_cond_wait (drivers/staging/android/vsoc.c) using newly
added wait_event_freezable_hrtimeout helper and remove useless includes.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 drivers/staging/android/vsoc.c | 69 +++++-----------------------------
 1 file changed, 10 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/android/vsoc.c b/drivers/staging/android/vsoc.c
index 22571abcaa4e..7e620e69f39d 100644
--- a/drivers/staging/android/vsoc.c
+++ b/drivers/staging/android/vsoc.c
@@ -17,7 +17,6 @@
  */
 
 #include <linux/dma-mapping.h>
-#include <linux/freezer.h>
 #include <linux/futex.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -29,7 +28,6 @@
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
 #include <linux/interrupt.h>
-#include <linux/mutex.h>
 #include <linux/cdev.h>
 #include <linux/file.h>
 #include "uapi/vsoc_shm.h"
@@ -401,7 +399,6 @@ static int handle_vsoc_cond_wait(struct file *filp, struct vsoc_cond_wait *arg)
 	DEFINE_WAIT(wait);
 	u32 region_number = iminor(file_inode(filp));
 	struct vsoc_region_data *data = vsoc_dev.regions_data + region_number;
-	struct hrtimer_sleeper timeout, *to = NULL;
 	int ret = 0;
 	struct vsoc_device_region *region_p = vsoc_region_from_filep(filp);
 	atomic_t *address = NULL;
@@ -420,69 +417,23 @@ static int handle_vsoc_cond_wait(struct file *filp, struct vsoc_cond_wait *arg)
 	/* Ensure that the type of wait is valid */
 	switch (arg->wait_type) {
 	case VSOC_WAIT_IF_EQUAL:
+		ret = wait_event_freezable(data->futex_wait_queue,
+					   arg->wakes++ &&
+					   atomic_read(address) != arg->value);
 		break;
 	case VSOC_WAIT_IF_EQUAL_TIMEOUT:
-		to = &timeout;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	if (to) {
-		/* Copy the user-supplied timesec into the kernel structure.
-		 * We do things this way to flatten differences between 32 bit
-		 * and 64 bit timespecs.
-		 */
 		if (arg->wake_time_nsec >= NSEC_PER_SEC)
 			return -EINVAL;
 		wake_time = ktime_set(arg->wake_time_sec, arg->wake_time_nsec);
-
-		hrtimer_init_on_stack(&to->timer, CLOCK_MONOTONIC,
-				      HRTIMER_MODE_ABS);
-		hrtimer_set_expires_range_ns(&to->timer, wake_time,
-					     current->timer_slack_ns);
-
-		hrtimer_init_sleeper(to, current);
+		ret = wait_event_freezable_hrtimeout(data->futex_wait_queue,
+						     arg->wakes++ &&
+						     atomic_read(address) != arg->value,
+						     wake_time);
+		break;
+	default:
+		return -EINVAL;
 	}
 
-	while (1) {
-		prepare_to_wait(&data->futex_wait_queue, &wait,
-				TASK_INTERRUPTIBLE);
-		/*
-		 * Check the sentinel value after prepare_to_wait. If the value
-		 * changes after this check the writer will call signal,
-		 * changing the task state from INTERRUPTIBLE to RUNNING. That
-		 * will ensure that schedule() will eventually schedule this
-		 * task.
-		 */
-		if (atomic_read(address) != arg->value) {
-			ret = 0;
-			break;
-		}
-		if (to) {
-			hrtimer_start_expires(&to->timer, HRTIMER_MODE_ABS);
-			if (likely(to->task))
-				freezable_schedule();
-			hrtimer_cancel(&to->timer);
-			if (!to->task) {
-				ret = -ETIMEDOUT;
-				break;
-			}
-		} else {
-			freezable_schedule();
-		}
-		/* Count the number of times that we woke up. This is useful
-		 * for unit testing.
-		 */
-		++arg->wakes;
-		if (signal_pending(current)) {
-			ret = -EINTR;
-			break;
-		}
-	}
-	finish_wait(&data->futex_wait_queue, &wait);
-	if (to)
-		destroy_hrtimer_on_stack(&to->timer);
 	return ret;
 }
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-01  5:38 ` [PATCH 1/3] sched/wait: use freezable_schedule when possible Hugo Lefeuvre
@ 2019-02-02 18:37   ` Joel Fernandes
  2019-02-06 23:30     ` Hugo Lefeuvre
  2019-02-07 13:28     ` Hugo Lefeuvre
  2019-02-05 16:37   ` Joel Fernandes
  1 sibling, 2 replies; 12+ messages in thread
From: Joel Fernandes @ 2019-02-02 18:37 UTC (permalink / raw)
  To: Hugo Lefeuvre
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

On Fri, Feb 01, 2019 at 06:38:05AM +0100, Hugo Lefeuvre wrote:
> Replace schedule(); try_to_freeze() by freezable_schedule().
> 
> Tasks calling freezable_schedule() set the PF_FREEZER_SKIP flag
> before calling schedule(). Unlike tasks calling schedule();
> try_to_freeze() tasks calling freezable_schedule() are not awaken by
> try_to_freeze_tasks(). Instead they call try_to_freeze() when they
> wake up if the freeze is still underway.
> 
> It is not a problem since sleeping tasks can't do anything which isn't
> allowed for a frozen task while sleeping.
> 
> The result is a potential performance gain during freeze, since less
> tasks have to be awaken.

I'm curious did you try the freezing process and see if pointless wakeups are
reduced?  That would be an added bonus if you did.

Otherwise seems like a nice change. Peter and Rafael, what do you think?

thanks,

 - Joel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] staging/android: simplify handle_vsoc_cond_wait
  2019-02-01  5:39 ` [PATCH 3/3] staging/android: simplify handle_vsoc_cond_wait Hugo Lefeuvre
@ 2019-02-04 15:52   ` Joel Fernandes
  0 siblings, 0 replies; 12+ messages in thread
From: Joel Fernandes @ 2019-02-04 15:52 UTC (permalink / raw)
  To: Hugo Lefeuvre
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

I think you sent to wrong address of Alistair on these and other patches. Corrected address.

On Fri, Feb 01, 2019 at 06:39:03AM +0100, Hugo Lefeuvre wrote:
> simplify handle_vsoc_cond_wait (drivers/staging/android/vsoc.c) using newly
> added wait_event_freezable_hrtimeout helper and remove useless includes.
> 
> Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> ---
>  drivers/staging/android/vsoc.c | 69 +++++-----------------------------
>  1 file changed, 10 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/staging/android/vsoc.c b/drivers/staging/android/vsoc.c
> index 22571abcaa4e..7e620e69f39d 100644
> --- a/drivers/staging/android/vsoc.c
> +++ b/drivers/staging/android/vsoc.c
> @@ -17,7 +17,6 @@
>   */
>  
>  #include <linux/dma-mapping.h>
> -#include <linux/freezer.h>
>  #include <linux/futex.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -29,7 +28,6 @@
>  #include <linux/syscalls.h>
>  #include <linux/uaccess.h>
>  #include <linux/interrupt.h>
> -#include <linux/mutex.h>
>  #include <linux/cdev.h>
>  #include <linux/file.h>
>  #include "uapi/vsoc_shm.h"
> @@ -401,7 +399,6 @@ static int handle_vsoc_cond_wait(struct file *filp, struct vsoc_cond_wait *arg)
>  	DEFINE_WAIT(wait);
>  	u32 region_number = iminor(file_inode(filp));
>  	struct vsoc_region_data *data = vsoc_dev.regions_data + region_number;
> -	struct hrtimer_sleeper timeout, *to = NULL;
>  	int ret = 0;
>  	struct vsoc_device_region *region_p = vsoc_region_from_filep(filp);
>  	atomic_t *address = NULL;
> @@ -420,69 +417,23 @@ static int handle_vsoc_cond_wait(struct file *filp, struct vsoc_cond_wait *arg)
>  	/* Ensure that the type of wait is valid */
>  	switch (arg->wait_type) {
>  	case VSOC_WAIT_IF_EQUAL:
> +		ret = wait_event_freezable(data->futex_wait_queue,
> +					   arg->wakes++ &&
> +					   atomic_read(address) != arg->value);
>  		break;
>  	case VSOC_WAIT_IF_EQUAL_TIMEOUT:
> -		to = &timeout;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> -
> -	if (to) {
> -		/* Copy the user-supplied timesec into the kernel structure.
> -		 * We do things this way to flatten differences between 32 bit
> -		 * and 64 bit timespecs.
> -		 */
>  		if (arg->wake_time_nsec >= NSEC_PER_SEC)
>  			return -EINVAL;
>  		wake_time = ktime_set(arg->wake_time_sec, arg->wake_time_nsec);
> -
> -		hrtimer_init_on_stack(&to->timer, CLOCK_MONOTONIC,
> -				      HRTIMER_MODE_ABS);
> -		hrtimer_set_expires_range_ns(&to->timer, wake_time,
> -					     current->timer_slack_ns);
> -
> -		hrtimer_init_sleeper(to, current);
> +		ret = wait_event_freezable_hrtimeout(data->futex_wait_queue,
> +						     arg->wakes++ &&
> +						     atomic_read(address) != arg->value,
> +						     wake_time);
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> -	while (1) {
> -		prepare_to_wait(&data->futex_wait_queue, &wait,
> -				TASK_INTERRUPTIBLE);
> -		/*
> -		 * Check the sentinel value after prepare_to_wait. If the value
> -		 * changes after this check the writer will call signal,
> -		 * changing the task state from INTERRUPTIBLE to RUNNING. That
> -		 * will ensure that schedule() will eventually schedule this
> -		 * task.
> -		 */
> -		if (atomic_read(address) != arg->value) {
> -			ret = 0;
> -			break;
> -		}
> -		if (to) {
> -			hrtimer_start_expires(&to->timer, HRTIMER_MODE_ABS);
> -			if (likely(to->task))
> -				freezable_schedule();
> -			hrtimer_cancel(&to->timer);
> -			if (!to->task) {
> -				ret = -ETIMEDOUT;
> -				break;
> -			}
> -		} else {
> -			freezable_schedule();
> -		}
> -		/* Count the number of times that we woke up. This is useful
> -		 * for unit testing.
> -		 */
> -		++arg->wakes;
> -		if (signal_pending(current)) {
> -			ret = -EINTR;
> -			break;
> -		}
> -	}
> -	finish_wait(&data->futex_wait_queue, &wait);
> -	if (to)
> -		destroy_hrtimer_on_stack(&to->timer);
>  	return ret;
>  }
>  
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-01  5:38 ` [PATCH 1/3] sched/wait: use freezable_schedule when possible Hugo Lefeuvre
  2019-02-02 18:37   ` Joel Fernandes
@ 2019-02-05 16:37   ` Joel Fernandes
  1 sibling, 0 replies; 12+ messages in thread
From: Joel Fernandes @ 2019-02-05 16:37 UTC (permalink / raw)
  To: Hugo Lefeuvre
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

On Fri, Feb 01, 2019 at 06:38:05AM +0100, Hugo Lefeuvre wrote:
> Replace schedule(); try_to_freeze() by freezable_schedule().
> 
> Tasks calling freezable_schedule() set the PF_FREEZER_SKIP flag
> before calling schedule(). Unlike tasks calling schedule();
> try_to_freeze() tasks calling freezable_schedule() are not awaken by
> try_to_freeze_tasks(). Instead they call try_to_freeze() when they
> wake up if the freeze is still underway.
> 
> It is not a problem since sleeping tasks can't do anything which isn't
> allowed for a frozen task while sleeping.
> 
> The result is a potential performance gain during freeze, since less
> tasks have to be awaken.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

> 
> Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> ---
>  include/linux/wait.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index ed7c122cb31f..5f3efabc36f4 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -308,7 +308,7 @@ do {										\
>  
>  #define __wait_event_freezable(wq_head, condition)				\
>  	___wait_event(wq_head, condition, TASK_INTERRUPTIBLE, 0, 0,		\
> -			    schedule(); try_to_freeze())
> +			    freezable_schedule())
>  
>  /**
>   * wait_event_freezable - sleep (or freeze) until a condition gets true
> @@ -367,7 +367,7 @@ do {										\
>  #define __wait_event_freezable_timeout(wq_head, condition, timeout)		\
>  	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
>  		      TASK_INTERRUPTIBLE, 0, timeout,				\
> -		      __ret = schedule_timeout(__ret); try_to_freeze())
> +		      __ret = freezable_schedule_timeout(__ret))
>  
>  /*
>   * like wait_event_timeout() -- except it uses TASK_INTERRUPTIBLE to avoid
> @@ -588,7 +588,7 @@ do {										\
>  
>  #define __wait_event_freezable_exclusive(wq, condition)				\
>  	___wait_event(wq, condition, TASK_INTERRUPTIBLE, 1, 0,			\
> -			schedule(); try_to_freeze())
> +			freezable_schedule())
>  
>  #define wait_event_freezable_exclusive(wq, condition)				\
>  ({										\
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] sched/wait: introduce wait_event_freezable_hrtimeout
  2019-02-01  5:38 ` [PATCH 2/3] sched/wait: introduce wait_event_freezable_hrtimeout Hugo Lefeuvre
@ 2019-02-05 16:42   ` Joel Fernandes
  0 siblings, 0 replies; 12+ messages in thread
From: Joel Fernandes @ 2019-02-05 16:42 UTC (permalink / raw)
  To: Hugo Lefeuvre
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

On Fri, Feb 01, 2019 at 06:38:35AM +0100, Hugo Lefeuvre wrote:
> introduce wait_event_freezable_hrtimeout, an interruptible and freezable
> version of wait_event_hrtimeout.
> 
> Among others this helper will allow for simplifications in
> staging/android/vsoc.c.
> 
> Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> ---

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

>  include/linux/wait.h | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 5f3efabc36f4..c4cf5113f58a 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -483,7 +483,7 @@ do {										\
>  	__ret;									\
>  })
>  
> -#define __wait_event_hrtimeout(wq_head, condition, timeout, state)		\
> +#define __wait_event_hrtimeout(wq_head, condition, timeout, state, cmd)		\
>  ({										\
>  	int __ret = 0;								\
>  	struct hrtimer_sleeper __t;						\
> @@ -500,7 +500,7 @@ do {										\
>  			__ret = -ETIME;						\
>  			break;							\
>  		}								\
> -		schedule());							\
> +		cmd);							        \
>  										\
>  	hrtimer_cancel(&__t.timer);						\
>  	destroy_hrtimer_on_stack(&__t.timer);					\
> @@ -529,7 +529,23 @@ do {										\
>  	might_sleep();								\
>  	if (!(condition))							\
>  		__ret = __wait_event_hrtimeout(wq_head, condition, timeout,	\
> -					       TASK_UNINTERRUPTIBLE);		\
> +					       TASK_UNINTERRUPTIBLE,		\
> +					       schedule());			\
> +	__ret;									\
> +})
> +
> +/*
> + * like wait_event_hrtimeout() -- except it uses TASK_INTERRUPTIBLE to avoid
> + * increasing load and is freezable.
> + */
> +#define wait_event_freezable_hrtimeout(wq_head, condition, timeout)		\
> +({										\
> +	int __ret = 0;								\
> +	might_sleep();								\
> +	if (!(condition))							\
> +		__ret = __wait_event_hrtimeout(wq_head, condition, timeout,	\
> +					       TASK_INTERRUPTIBLE,		\
> +					       freezable_schedule());		\
>  	__ret;									\
>  })
>  
> @@ -555,7 +571,8 @@ do {										\
>  	might_sleep();								\
>  	if (!(condition))							\
>  		__ret = __wait_event_hrtimeout(wq, condition, timeout,		\
> -					       TASK_INTERRUPTIBLE);		\
> +					       TASK_INTERRUPTIBLE,		\
> +					       schedule());			\
>  	__ret;									\
>  })
>  
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-02 18:37   ` Joel Fernandes
@ 2019-02-06 23:30     ` Hugo Lefeuvre
  2019-02-07 15:05       ` Joel Fernandes
  2019-02-07 13:28     ` Hugo Lefeuvre
  1 sibling, 1 reply; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-06 23:30 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

Hi Joel,

> I'm curious did you try the freezing process and see if pointless wakeups are
> reduced?  That would be an added bonus if you did.

I'm currently testing these changes. I hope to be able to come back with
more concrete results soon.

Also, I just noticed that the third patch removes a necessary #include
<linux/freezer.h>. I will submit an updated version tomorrow.

Thanks for the review!

regards,
 Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-02 18:37   ` Joel Fernandes
  2019-02-06 23:30     ` Hugo Lefeuvre
@ 2019-02-07 13:28     ` Hugo Lefeuvre
  1 sibling, 0 replies; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-07 13:28 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

Hi,

> > The result is a potential performance gain during freeze, since less
> > tasks have to be awaken.
> 
> I'm curious did you try the freezing process and see if pointless wakeups are
> reduced?  That would be an added bonus if you did.

Test env: fresh Debian QEMU vm with 4.19 stable kernel.

Test process:

- Added two debug logs to freeze_task:

    bool freeze_task(struct task_struct *p)
    {
        unsigned long flags;
    [snip]
        pr_info("freezing a task");
    [snip]
        if (freezer_should_skip(p)) {
            pr_info("skeeping a task");
            return false;
        }
    [snip]
    }

- Triggered manual freeze:

# echo freezer > /sys/power/pm_test
# echo test_resume > /sys/power/disk
# echo disk > /sys/power/state

- grep -c to get the number of "freezing a task" and "skeeping a task"
lines in kern.log.

Results:

Without my patch: 448 calls freeze_task, 12 skipped.
With my patch: 448 calls, 32 skipped.

2.6x more tasks skipped.

Not sure this is the best way to test this patch, though. Any advice?

regards,
 Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-06 23:30     ` Hugo Lefeuvre
@ 2019-02-07 15:05       ` Joel Fernandes
  2019-02-07 21:40         ` Hugo Lefeuvre
  0 siblings, 1 reply; 12+ messages in thread
From: Joel Fernandes @ 2019-02-07 15:05 UTC (permalink / raw)
  To: Hugo Lefeuvre
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

On Thu, Feb 07, 2019 at 12:30:50AM +0100, Hugo Lefeuvre wrote:
> Hi Joel,
> 
> > I'm curious did you try the freezing process and see if pointless wakeups are
> > reduced?  That would be an added bonus if you did.
> 
> I'm currently testing these changes. I hope to be able to come back with
> more concrete results soon.
> 
> Also, I just noticed that the third patch removes a necessary #include
> <linux/freezer.h>. I will submit an updated version tomorrow.
> 
> Thanks for the review!

Sure, add these test results to the patch as well showing reduced wakeups.

I would say submit the freezable_schedule as a single separate patch
independent of the vsoc series since it can go in separately, and also
benefits other things than vsoc.

Also CC Rafael (power maintainer) on it.

Thank you!

 - Joel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] sched/wait: use freezable_schedule when possible
  2019-02-07 15:05       ` Joel Fernandes
@ 2019-02-07 21:40         ` Hugo Lefeuvre
  0 siblings, 0 replies; 12+ messages in thread
From: Hugo Lefeuvre @ 2019-02-07 21:40 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Greg Kroah-Hartman, Greg Hartman, Alistair Strachan,
	Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
	Christian Brauner, Ingo Molnar, Peter Zijlstra, devel,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

> Sure, add these test results to the patch as well showing reduced wakeups.
> 
> I would say submit the freezable_schedule as a single separate patch
> independent of the vsoc series since it can go in separately, and also
> benefits other things than vsoc.
> 
> Also CC Rafael (power maintainer) on it.

Thanks, I have splitted the patch set[0][1] and submitted the
freezable_schedule patch separately (only cc-ing people responsible
for the wait api + Rafael).

regards,
 Hugo

[0] https://lkml.org/lkml/2019/2/7/802
[1] https://lkml.org/lkml/2019/2/7/870

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-02-07 21:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-01  5:37 [PATCH 0/3] sched/wait, staging/android: simplification and optimization of freeze related code Hugo Lefeuvre
2019-02-01  5:38 ` [PATCH 1/3] sched/wait: use freezable_schedule when possible Hugo Lefeuvre
2019-02-02 18:37   ` Joel Fernandes
2019-02-06 23:30     ` Hugo Lefeuvre
2019-02-07 15:05       ` Joel Fernandes
2019-02-07 21:40         ` Hugo Lefeuvre
2019-02-07 13:28     ` Hugo Lefeuvre
2019-02-05 16:37   ` Joel Fernandes
2019-02-01  5:38 ` [PATCH 2/3] sched/wait: introduce wait_event_freezable_hrtimeout Hugo Lefeuvre
2019-02-05 16:42   ` Joel Fernandes
2019-02-01  5:39 ` [PATCH 3/3] staging/android: simplify handle_vsoc_cond_wait Hugo Lefeuvre
2019-02-04 15:52   ` Joel Fernandes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).