[Xen-devel] Re: [RFC PATCH 27/33] Add the Xen virtual console driver.

[Arjan van de Ven <arjan@xxxxxxxxxxxxx>]

On Tue, 2006-07-18 at 00:00 -0700, Chris Wright wrote:
> +                     } else if (sysrq_requested) {
> +                             unsigned long sysrq_timeout =
> +                                     sysrq_requested + HZ*2;
> +                             sysrq_requested = 0;
> +                             if (time_before(jiffies, sysrq_timeout)) {
> +                                     spin_unlock_irqrestore(
> +                                             &xencons_lock, flags);
> +                                     handle_sysrq(
> +                                             buf[i], regs, xencons_tty);
> +                                     spin_lock_irqsave(
> +                                             &xencons_lock, flags);
> +                                     continue;
> +                             }

Lindent can be harmful...

> +static void xencons_wait_until_sent(struct tty_struct *tty, int timeout)
> +{
> +     unsigned long orig_jiffies = jiffies;
> +
> +     if (TTY_INDEX(tty) != 0)
> +             return;
> +
> +     while (DRV(tty->driver)->chars_in_buffer(tty)) {
> +             set_current_state(TASK_INTERRUPTIBLE);
> +             schedule_timeout(1);
> +             if (signal_pending(current))
> +                     break;
> +             if (timeout && time_after(jiffies, orig_jiffies + timeout))
> +                     break;
> +     }
> +
> +     set_current_state(TASK_RUNNING);
> +}

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..





_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel