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@xxxxxxxxxx>
> ---
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
