[PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Hugo Lefeuvre]

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

introduce wait_event_freezable_hrtimeout, an interruptible and freezable
version of wait_event_hrtimeout.

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

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 drivers/staging/android/vsoc.c | 69 +++++-----------------------------
 include/linux/wait.h           | 25 ++++++++++--
 2 files changed, 31 insertions(+), 63 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;
 }
 
diff --git a/include/linux/wait.h b/include/linux/wait.h
index ed7c122cb31f..13a454884f8b 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

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

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Greg Kroah-Hartman]

On Thu, Jan 17, 2019 at 11:41:35PM +0100, Hugo Lefeuvre wrote:
> introduce wait_event_freezable_hrtimeout, an interruptible and freezable
> version of wait_event_hrtimeout.
> 
> simplify handle_vsoc_cond_wait (drivers/staging/android/vsoc.c) using this
> newly added helper and remove useless includes.
> 
> Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> ---
>  drivers/staging/android/vsoc.c | 69 +++++-----------------------------
>  include/linux/wait.h           | 25 ++++++++++--

code in drivers/staging/ should be self-contained, and not, if at all
possible, ever force additional changes on "core" kernel code.

Are you sure that the vsoc code can't use one of the current wait
macros?  Why is it so special and unique that no one else in the kernel
has ever needed this before it came along?

thanks,

greg k-h

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Hugo Lefeuvre]

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

Hi Greg,

> > introduce wait_event_freezable_hrtimeout, an interruptible and freezable
> > version of wait_event_hrtimeout.
> > 
> > simplify handle_vsoc_cond_wait (drivers/staging/android/vsoc.c) using this
> > newly added helper and remove useless includes.
> > 
> > Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> > ---
> >  drivers/staging/android/vsoc.c | 69 +++++-----------------------------
> >  include/linux/wait.h           | 25 ++++++++++--
> 
> code in drivers/staging/ should be self-contained, and not, if at all
> possible, ever force additional changes on "core" kernel code.
> 
> Are you sure that the vsoc code can't use one of the current wait
> macros?

As far as I know there is no macro implementing freezable wait with high
resolution timeout.

> Why is it so special and unique that no one else in the kernel
> has ever needed this before it came along?

many wait_event_X() (_exclusive, _interruptible, _timeout) functions have a
freezable counterpart. wait_event_hrtimeout() doesn't, probably because it
is relatively new (and seemingly quite unused).

If there is a wait_event_hrtimeout() function, it makes sense to me to have
wait_event_freezable_hrtimeout().

-- 
                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 --]

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Joel Fernandes]

Hi Hugo,

On Thu, Jan 17, 2019 at 11:41:35PM +0100, Hugo Lefeuvre wrote:
> introduce wait_event_freezable_hrtimeout, an interruptible and freezable
> version of wait_event_hrtimeout.
> 
> simplify handle_vsoc_cond_wait (drivers/staging/android/vsoc.c) using this
> newly added helper and remove useless includes.

I believe these should be 2 patches. In the first patch you introduce the
new API, in the second one you would simplify the VSOC driver.

In fact, in one part of the patch you are using wait_event_freezable which
already exists so that's unrelated to the new API you are adding.

More comments below:

> Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> ---
>  drivers/staging/android/vsoc.c | 69 +++++-----------------------------
>  include/linux/wait.h           | 25 ++++++++++--
>  2 files changed, 31 insertions(+), 63 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;
>  }
>  
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index ed7c122cb31f..13a454884f8b 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)		\

You should document the variable names in the header comments.

Also, this new API appears to conflict with definition of 'freezable' in
wait_event_freezable I think,

wait_event_freezable - sleep or freeze until condition is true.

wait_event_freezable_hrtimeout - sleep but make sure freezer is not blocked.
(your API)

It seems to me these are 2 different definitions of 'freezing' (or I could be
completely confused). But in the first case it calls try_to_freeze after
schedule(). In the second case (your new API), it calls freezable_schedule().

So I am wondering why is there this difference in the 'meaning of freezable'.
In the very least, any such subtle differences should be documented in the
header comments IMO.

thanks!

 - Joel

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Peter Zijlstra]

On Fri, Jan 18, 2019 at 10:19:41AM -0500, Joel Fernandes wrote:
> You should document the variable names in the header comments.
> 
> Also, this new API appears to conflict with definition of 'freezable' in
> wait_event_freezable I think,
> 
> wait_event_freezable - sleep or freeze until condition is true.
> 
> wait_event_freezable_hrtimeout - sleep but make sure freezer is not blocked.
> (your API)
> 
> It seems to me these are 2 different definitions of 'freezing' (or I could be
> completely confused). But in the first case it calls try_to_freeze after
> schedule(). In the second case (your new API), it calls freezable_schedule().
> 
> So I am wondering why is there this difference in the 'meaning of freezable'.
> In the very least, any such subtle differences should be documented in the
> header comments IMO.

Also; someone was recently hating on the whole freezing thing (again,
and rightfully). I just cannot remember who; Rafael you remember?

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Hugo Lefeuvre]

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

Hi Joel,

Thanks for your review.

> I believe these should be 2 patches. In the first patch you introduce the
> new API, in the second one you would simplify the VSOC driver.
> 
> In fact, in one part of the patch you are using wait_event_freezable which
> already exists so that's unrelated to the new API you are adding.

Agree, I will split the patch for the v2.

> > +/*
> > + * 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)		\
> 
> You should document the variable names in the header comments.

Agree. This comment was copy/pasted from wait_event_freezable_timeout,
should I fix it there as well?

> Also, this new API appears to conflict with definition of 'freezable' in
> wait_event_freezable I think,
> 
> wait_event_freezable - sleep or freeze until condition is true.
> 
> wait_event_freezable_hrtimeout - sleep but make sure freezer is not blocked.
> (your API)
> 
> It seems to me these are 2 different definitions of 'freezing' (or I could be
> completely confused). But in the first case it calls try_to_freeze after
> schedule(). In the second case (your new API), it calls freezable_schedule().
> 
> So I am wondering why is there this difference in the 'meaning of freezable'.
> In the very least, any such subtle differences should be documented in the
> header comments IMO.

It appears that freezable_schedule() and schedule(); try_to_freeze() are
almost identical:

    static inline void freezable_schedule(void)
    {
        freezer_do_not_count();
        schedule();
        freezer_count();
    }

and

    static inline void freezer_do_not_count(void)
    {
        current->flags |= PF_FREEZER_SKIP;
    }

    static inline void freezer_count(void)
    {
        current->flags &= ~PF_FREEZER_SKIP;
        /*
         * If freezing is in progress, the following paired with smp_mb()
         * in freezer_should_skip() ensures that either we see %true
         * freezing() or freezer_should_skip() sees !PF_FREEZER_SKIP.
         */
        smp_mb();
        try_to_freeze();
    }

as far as I understand this code, freezable_schedule() avoids blocking the
freezer during the schedule() call, but in the end try_to_freeze() is still
called so the result is the same, right?

I wonder why wait_event_freezable is not calling freezable_schedule().

That being said, I am not sure that the try_to_freeze() call does anything
in the vsoc case because there is no call to set_freezable() so the thread
still has PF_NOFREEZE...

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 --]

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Joel Fernandes]

On Fri, Jan 18, 2019 at 06:08:01PM +0100, Hugo Lefeuvre wrote:
[...] 
> > > +/*
> > > + * 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)		\
> > 
> > You should document the variable names in the header comments.
> 
> Agree. This comment was copy/pasted from wait_event_freezable_timeout,
> should I fix it there as well?
> 
> > Also, this new API appears to conflict with definition of 'freezable' in
> > wait_event_freezable I think,
> > 
> > wait_event_freezable - sleep or freeze until condition is true.
> > 
> > wait_event_freezable_hrtimeout - sleep but make sure freezer is not blocked.
> > (your API)
> > 
> > It seems to me these are 2 different definitions of 'freezing' (or I could be
> > completely confused). But in the first case it calls try_to_freeze after
> > schedule(). In the second case (your new API), it calls freezable_schedule().
> > 
> > So I am wondering why is there this difference in the 'meaning of freezable'.
> > In the very least, any such subtle differences should be documented in the
> > header comments IMO.
> 
> It appears that freezable_schedule() and schedule(); try_to_freeze() are
> almost identical:
> 
>     static inline void freezable_schedule(void)
>     {
>         freezer_do_not_count();
>         schedule();
>         freezer_count();
>     }
> 
> and
> 
>     static inline void freezer_do_not_count(void)
>     {
>         current->flags |= PF_FREEZER_SKIP;
>     }
> 
>     static inline void freezer_count(void)
>     {
>         current->flags &= ~PF_FREEZER_SKIP;
>         /*
>          * If freezing is in progress, the following paired with smp_mb()
>          * in freezer_should_skip() ensures that either we see %true
>          * freezing() or freezer_should_skip() sees !PF_FREEZER_SKIP.
>          */
>         smp_mb();
>         try_to_freeze();
>     }
> 
> as far as I understand this code, freezable_schedule() avoids blocking the
> freezer during the schedule() call, but in the end try_to_freeze() is still
> called so the result is the same, right?
> I wonder why wait_event_freezable is not calling freezable_schedule().

It could be something subtle in my view. freezable_schedule() actually makes
the freezer code not send a wake up to the sleeping task if a freeze happens,
because the PF_FREEZER_SKIP flag is set, as you pointed.

Whereas wait_event_freezable() which uses try_to_freeze() does not seem to have
this behavior and the task will enter the freezer. So I'm a bit skeptical if
your API will behave as expected (or at least consistently with other wait
APIs).

> That being said, I am not sure that the try_to_freeze() call does anything
> in the vsoc case because there is no call to set_freezable() so the thread
> still has PF_NOFREEZE...

I traced this, and PF_NOFREEZE is not set by default for tasks.

thanks,

 - Joel

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Hugo Lefeuvre]

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

> > as far as I understand this code, freezable_schedule() avoids blocking the
> > freezer during the schedule() call, but in the end try_to_freeze() is still
> > called so the result is the same, right?
> > I wonder why wait_event_freezable is not calling freezable_schedule().
> 
> It could be something subtle in my view. freezable_schedule() actually makes
> the freezer code not send a wake up to the sleeping task if a freeze happens,
> because the PF_FREEZER_SKIP flag is set, as you pointed.
> 
> Whereas wait_event_freezable() which uses try_to_freeze() does not seem to have
> this behavior and the task will enter the freezer. So I'm a bit skeptical if
> your API will behave as expected (or at least consistently with other wait
> APIs).

oh right, now it is clear to me:

- schedule(); try_to_freeze()

schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is
not set, the task wakes up as soon as try_to_freeze_tasks() is called.
Right after waking up the task calls try_to_freeze() which freezes it.

- freezable_schedule() 

schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is
set, the task does not wake up when try_to_freeze_tasks() is called. This
is not a problem, the task can't "do anything which isn't allowed for a
frozen task" while sleeping[0]. 

When the task wakes up (timeout, or whatever other reason) it calls
try_to_freeze() which freezes it if the freeze is still underway.

So if a freeze is triggered while the task is sleeping, a task executing
freezable_schedule() might or might not notice the freeze depending on how
long it sleeps. A task executing schedule(); try_to_freeze() will always
notice it.

I might be wrong on that, but freezable_schedule() just seems like a
performance improvement to me.

Now I fully agree with you that there should be a uniform definition of
"freezable" between wait_event_freezable and wait_event_freezable_hrtimeout.
This leaves me to the question: should I modify my definition of
wait_event_freezable_hrtimeout, or prepare a patch for wait_event_freezable ?

If I am right with the performance thing, the latter might be worth
considering?

Either way, this will be fixed in the v2.

> > That being said, I am not sure that the try_to_freeze() call does anything
> > in the vsoc case because there is no call to set_freezable() so the thread
> > still has PF_NOFREEZE...
> 
> I traced this, and PF_NOFREEZE is not set by default for tasks.

Well, I did not check this in practice and might be confused somewhere but
the documentation[1] says "kernel threads are not freezable by default.
However, a kernel thread may clear PF_NOFREEZE for itself by calling
set_freezable()".

Looking at the kthreadd() definition it seems like new tasks have
PF_NOFREEZE set by default[2].

I'll take some time to check this in practice in the next days.

Anyways, thanks for your time !

regards,
 Hugo

[0] https://elixir.bootlin.com/linux/latest/source/include/linux/freezer.h#L103
[1] https://elixir.bootlin.com/linux/latest/source/Documentation/power/freezing-of-tasks.txt#L90
[2] https://elixir.bootlin.com/linux/latest/source/kernel/kthread.c#L569

-- 
                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 --]

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Rafael J. Wysocki]

On Friday, January 18, 2019 5:04:50 PM CET Peter Zijlstra wrote:
> On Fri, Jan 18, 2019 at 10:19:41AM -0500, Joel Fernandes wrote:
> > You should document the variable names in the header comments.
> > 
> > Also, this new API appears to conflict with definition of 'freezable' in
> > wait_event_freezable I think,
> > 
> > wait_event_freezable - sleep or freeze until condition is true.
> > 
> > wait_event_freezable_hrtimeout - sleep but make sure freezer is not blocked.
> > (your API)
> > 
> > It seems to me these are 2 different definitions of 'freezing' (or I could be
> > completely confused). But in the first case it calls try_to_freeze after
> > schedule(). In the second case (your new API), it calls freezable_schedule().
> > 
> > So I am wondering why is there this difference in the 'meaning of freezable'.
> > In the very least, any such subtle differences should be documented in the
> > header comments IMO.
> 
> Also; someone was recently hating on the whole freezing thing (again,
> and rightfully). I just cannot remember who; Rafael you remember?

Nope.

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Joel Fernandes]

On Sat, Jan 19, 2019 at 11:29:12AM +0100, Hugo Lefeuvre wrote:
> > > as far as I understand this code, freezable_schedule() avoids blocking the
> > > freezer during the schedule() call, but in the end try_to_freeze() is still
> > > called so the result is the same, right?
> > > I wonder why wait_event_freezable is not calling freezable_schedule().
> > 
> > It could be something subtle in my view. freezable_schedule() actually makes
> > the freezer code not send a wake up to the sleeping task if a freeze happens,
> > because the PF_FREEZER_SKIP flag is set, as you pointed.
> > 
> > Whereas wait_event_freezable() which uses try_to_freeze() does not seem to have
> > this behavior and the task will enter the freezer. So I'm a bit skeptical if
> > your API will behave as expected (or at least consistently with other wait
> > APIs).
> 
> oh right, now it is clear to me:
> 
> - schedule(); try_to_freeze()
> 
> schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is
> not set, the task wakes up as soon as try_to_freeze_tasks() is called.
> Right after waking up the task calls try_to_freeze() which freezes it.
> 
> - freezable_schedule() 
> 
> schedule() is called and the task enters sleep. Since PF_FREEZER_SKIP is
> set, the task does not wake up when try_to_freeze_tasks() is called. This
> is not a problem, the task can't "do anything which isn't allowed for a
> frozen task" while sleeping[0]. 
> 
> When the task wakes up (timeout, or whatever other reason) it calls
> try_to_freeze() which freezes it if the freeze is still underway.
> 
> So if a freeze is triggered while the task is sleeping, a task executing
> freezable_schedule() might or might not notice the freeze depending on how
> long it sleeps. A task executing schedule(); try_to_freeze() will always
> notice it.
> 
> I might be wrong on that, but freezable_schedule() just seems like a
> performance improvement to me.
> 
> Now I fully agree with you that there should be a uniform definition of
> "freezable" between wait_event_freezable and wait_event_freezable_hrtimeout.
> This leaves me to the question: should I modify my definition of
> wait_event_freezable_hrtimeout, or prepare a patch for wait_event_freezable ?
> 
> If I am right with the performance thing, the latter might be worth
> considering?
> 
> Either way, this will be fixed in the v2.

I agree, it is probably better to use freezable_schedule() for all freeze
related wait APIs, and keep it consistent. Your analysis is convincing.

Peter, what do you think?

> > > That being said, I am not sure that the try_to_freeze() call does anything
> > > in the vsoc case because there is no call to set_freezable() so the thread
> > > still has PF_NOFREEZE...
> > 
> > I traced this, and PF_NOFREEZE is not set by default for tasks.
> 
> Well, I did not check this in practice and might be confused somewhere but
> the documentation[1] says "kernel threads are not freezable by default.
> However, a kernel thread may clear PF_NOFREEZE for itself by calling
> set_freezable()".
> 
> Looking at the kthreadd() definition it seems like new tasks have
> PF_NOFREEZE set by default[2].
> 
> I'll take some time to check this in practice in the next days.
> 
> Anyways, thanks for your time !

Yeah, no problem.

thanks,

 - Joel

Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout

[Hugo Lefeuvre]

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

Hi,

> I agree, it is probably better to use freezable_schedule() for all freeze
> related wait APIs, and keep it consistent. Your analysis is convincing.

I have submitted a new patchset which migrates the wait api to
freezable_schedule() and splits the changes from the previous patch.

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 --]