| From | Keir Fraser <> | Subject | Re: [RFC PATCH 27/33] Add the Xen virtual console driver. | Date | Tue, 18 Jul 2006 11:31:42 +0100 |
| |
On 18 Jul 2006, at 11:24, Arjan van de Ven wrote:
> hmm somehow I find this code scary; we had similar code recently > elsewhere where this turned out to be a real issue; you now sleep for > "1" time, so you sleep for a fixed time if you aren't getting wakeups, > but if you are getting wakeups your code is upside down, I would expect > it to look like > > + set_current_state(TASK_INTERRUPTIBLE); > + while (DRV(tty->driver)->chars_in_buffer(tty)) > + schedule_timeout(1); > + if (signal_pending(current)) > + break; > + if (timeout && time_after(jiffies, orig_jiffies + timeout)) > + break; > + set_current_state(TASK_INTERRUPTIBLE); > + } > > instead, so that you don't have the wakeup race..
There's no wakeup signal, so no possibility of a wakeup race. That's why we schedule_timeout() instead of wait_event() or similar. This code is only used to flush the console when the kernel crashes, so we can get the full oops, so waiting a little bit too long is acceptable.
Your suggested change is perhaps more idiomatic though, and less jarring for reviewers. :-)
Thanks for your comments by the way. Reviewing lots of patches isn't much fun.
-- Keir
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|