Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 18 Jan 2019 10:19:41 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Hugo Lefeuvre <hle@owl.eu.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Greg Hartman <ghartman@google.com>,
        Alistair Strachan <strachan@google.com>,
        Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Christian Brauner <christian@brauner.io>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sched/wait: introduce wait_event_freezable_hrtimeout
Message-ID: <20190118151941.GB187589@google.com>
References: <20190117224135.GC8100@hle-laptop.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190117224135.GC8100@hle-laptop.local>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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