+ poll-allow-f_op-poll-to-sleep-take-4.patch added to -mm tree

(quoted diff) +17 -0

+static int pollwake(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct poll_wqueues *pwq = wait->private;
+	DECLARE_WAITQUEUE(dummy_wait, pwq->polling_task);
+
+	set_mb(pwq->triggered, 1);
+
+	/*
+	 * Perform the default wake up operation using a dummy
+	 * waitqueue.
+	 *
+	 * TODO: This is hacky but there currently is no interface to
+	 * pass in @sync.  @sync is scheduled to be removed and once
+	 * that happens, wake_up_process() can be used directly.
+	 */
+	return default_wake_function(&dummy_wait, mode, sync, key);
+}

Quoted 17 lines

(quoted diff) +17 -0

+static int pollwake(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct poll_wqueues *pwq = wait->private;
+	DECLARE_WAITQUEUE(dummy_wait, pwq->polling_task);
+
+	set_mb(pwq->triggered, 1);
+
+	/*
+	 * Perform the default wake up operation using a dummy
+	 * waitqueue.
+	 *
+	 * TODO: This is hacky but there currently is no interface to
+	 * pass in @sync.  @sync is scheduled to be removed and once
+	 * that happens, wake_up_process() can be used directly.
+	 */
+	return default_wake_function(&dummy_wait, mode, sync, key);
+}

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
+			  ktime_t *expires, unsigned long slack)
+{
+	int rc = -EINTR;
+
+	set_current_state(state);
+	if (!pwq->triggered)
+		rc = schedule_hrtimeout_range(expires, slack, HRTIMER_MODE_ABS);
+	__set_current_state(TASK_RUNNING);

So, why do we need this mb() in pollwake() ?

try_to_wake_up() has a full barrier semantics, note the wmb() before task_rq_lock(). Since spin_lock() itself is STORE, the setting of pwq->triggered can't be further re-ordered with the reading of p->state.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/* clear triggered for the next iteration */
+	pwq->triggered = 0;

And don't we (in theory) actually need the mb() here instead?

Let's suppose do_poll() starts the next iteration, so we are doing

	pwq->triggered = 0;

	->poll(file)
		if (!check_file(file))
			return 0;

		return POLLXXX;

We don't have any barriers in between (unless fget_light bumps ->f_count), so this can be reordered as

	->poll(file)
		if (!check_file(file))
			return 0;

		pwq->triggered = 0;

And, if pollwake() happens in between we can miss the event, no?

So, why do we need this mb() in pollwake() ?

try_to_wake_up() has a full barrier semantics, note the wmb() before task_rq_lock(). Since spin_lock() itself is STORE, the setting of pwq->triggered can't be further re-ordered with the reading of p->state.

Or any other reason ?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/* clear triggered for the next iteration */
+	pwq->triggered = 0;

And don't we (in theory) actually need the mb() here instead?

Let's suppose do_poll() starts the next iteration, so we are doing

	pwq->triggered = 0;

	->poll(file)
		if (!check_file(file))
			return 0;

		return POLLXXX;

We don't have any barriers in between (unless fget_light bumps ->f_count), so this can be reordered as

	->poll(file)
		if (!check_file(file))
			return 0;

		pwq->triggered = 0;

And, if pollwake() happens in between we can miss the event, no?

fs/select.c +31 -0

+static int pollwake(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct poll_wqueues *pwq = wait->private;
+	DECLARE_WAITQUEUE(dummy_wait, pwq->polling_task);
+
+	/*
+	 * Wake up functions have full barrier semantics, no need for
+	 * barrier here.
+	 */
+	pwq->triggered = 1;
+
+	/*
+	 * Perform the default wake up operation using a dummy
+	 * waitqueue.
+	 *
+	 * TODO: This is hacky but there currently is no interface to
+	 * pass in @sync.  @sync is scheduled to be removed and once
+	 * that happens, wake_up_process() can be used directly.
+	 */
+	return default_wake_function(&dummy_wait, mode, sync, key);
+}
+int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
+			  ktime_t *expires, unsigned long slack)
+{
+	int rc = -EINTR;
+
+	set_current_state(state);
+	if (!pwq->triggered)
+		rc = schedule_hrtimeout_range(expires, slack, HRTIMER_MODE_ABS);
+	__set_current_state(TASK_RUNNING);
+

Look, pollwake() does:

w1) WR triggered (1)
w2) WMB
w3) WR task->state (RUNNING)

While poll_schedule_timeout() does:

s1) WR task->state (TASK_INTERRUPTIBLE)
s2) MB
s3) RD triggered
s4) IF0 => RD task->state (if !RUNNING -> sleep)

The only risk is that w3 preceed s1, so that we go to sleep even though a wakeup has been issued. But if w3 is visible, w1 is visible too, that means that 'triggered' is visible in s3 (there's a MB in s2). So we skip the schedule_hrtimeout_range(). So IMO you need no barriers on 'triggered'. If you feel you need barriers, do you mind explaning a sequence of events that makes a barrier-free version break?

Hello,

Quoted 6 lines · Davide Libenzi

Look, pollwake() does:

w1) WR triggered (1)
w2) WMB
w3) WR task->state (RUNNING)

Quoted 6 lines

While poll_schedule_timeout() does:

s1) WR task->state (TASK_INTERRUPTIBLE)
s2) MB
s3) RD triggered
s4) IF0 => RD task->state (if !RUNNING -> sleep)

s5) after waking up, WR triggered to zero

Quoted 6 lines

The only risk is that w3 preceed s1, so that we go to sleep even though a wakeup has been issued. But if w3 is visible, w1 is visible too, that means that 'triggered' is visible in s3 (there's a MB in s2). So we skip the schedule_hrtimeout_range(). So IMO you need no barriers on 'triggered'. If you feel you need barriers, do you mind explaning a sequence of events that makes a barrier-free version break?

s5 from the previous iteration could happen after w1 during the next iteration and the test in s4 of the next iteration will miss the event, so the event could get lost on the iterations which is not the first one, no?

Hmmm, I just noticed that the set_current_state(TASK_INTERRUPTIBLE) at the beginning of the ->poll() loop has been dropped (and it makes sense since now ->poll() can sleep).

w1) WR dev->events
w2) MB
w3) WR triggered (1)
w4) WMB
w5) WR task->state (RUNNING)

Poller side:

s1) WR task->state (TASK_INTERRUPTIBLE)
s2) MB
s3) RD triggered
s4) IF0 => RD task->state (if !RUNNING -> sleep)
s5) WR triggered (0)
s6) MB
s7) RD dev->events

That is, an MB before w3 (triggered=1) and a set_mb(triggered,0) at s5+s6. The spinlock on the queue taken before entering pollwake() is not enough to guarantee the required ordering, since a LOCK is no guarantee that operations before it are visible after the LOCK. Without the MB at w2, it could happen [w3, s5, s7, w1] that will make us miss the event *and* sleep.

f_op->poll is the only vfs operation which is not allowed to sleep. It's because poll and select implementation used task state to synchronize against wake ups, which doesn't have to be the case anymore as wait/wake interface can now use custom wake up functions. The non-sleep restriction can be a bit tricky because ->poll is not called from an atomic context and the result of accidentally sleeping in ->poll only shows up as temporary busy looping when the timing is right or rather wrong.

This patch converts poll/select to use custom wake up function and use separate triggered variable to synchronize against wake up events. The only added overhead is an extra function call during wake up and negligible.

This patch removes the one non-sleep exception from vfs locking rules and is beneficial to userland filesystem implementations like FUSE, 9p or peculiar fs like spufs as it's very difficult for those to implement non-sleeping poll method.

Looks OK to me, although it'd be better if some more folks eye it, in order to avoid painful mistakes.
Did you test it al all in a live system?

Quoted 13 lines · Tejun Heo

f_op->poll is the only vfs operation which is not allowed to sleep. It's because poll and select implementation used task state to synchronize against wake ups, which doesn't have to be the case anymore as wait/wake interface can now use custom wake up functions. The non-sleep restriction can be a bit tricky because ->poll is not called from an atomic context and the result of accidentally sleeping in ->poll only shows up as temporary busy looping when the timing is right or rather wrong.

This patch converts poll/select to use custom wake up function and use separate triggered variable to synchronize against wake up events. The only added overhead is an extra function call during wake up and negligible.

Quoted 4 lines

This patch removes the one non-sleep exception from vfs locking rules and is beneficial to userland filesystem implementations like FUSE, 9p or peculiar fs like spufs as it's very difficult for those to implement non-sleeping poll method.

Looks OK to me, although it'd be better if some more folks eye it, in order to avoid painful mistakes.

w1) WR dev->events
w2) WR triggered (1)
w3) WMB
w4) WR task->state (RUNNING)
...
That is, an MB before w3 (triggered=1) and a set_mb(triggered,0) at s5+s6. The spinlock on the queue taken before entering pollwake() is not enough to guarantee the required ordering, since a LOCK is no guarantee that operations before it are visible after the LOCK. Without the MB at w2, it could happen [w3, s5, s7, w1] that will make us miss the event *and* sleep.

Quoted 19 lines

And don't we (in theory) actually need the mb() here instead?

Let's suppose do_poll() starts the next iteration, so we are doing

	pwq->triggered = 0;

	->poll(file)
		if (!check_file(file))
			return 0;

		return POLLXXX;

We don't have any barriers in between (unless fget_light bumps ->f_count), so this can be reordered as

	->poll(file)
		if (!check_file(file))
			return 0;

Quoted 3 lines

		pwq->triggered = 0;

And, if pollwake() happens in between we can miss the event, no?

Hmmmm... yes, from the second run, ->poll doesn't grab the waitqueue lock, so it doesn't necessary have the required barriers. Heh... set_mb() should be here not in pollwake(). Thanks for spotting it.