Messages in this thread | | | Date | Thu, 27 Nov 2008 18:18:49 +0900 | From | Tejun Heo <> | Subject | Re: [PATCH] poll: allow f_op->poll to sleep, take#5 |
| |
Hello,
Davide Libenzi wrote: > 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).
Yeah, that's exactly what the ->triggered condition replaces.
> 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.
Yeah, it seems we'll need something which is equivalent to smp_wmb() in try_to_wake_up(). So, the original set_mb() should have stayed there while just adding the latter one. Will prep yet another take of the patch. Thanks for the detailed analysis.
-- tejun
| |