Re: [Qemu-devel] [patch 04/10] qemu: introduce main_loop_break

[Marcelo Tosatti]

On Thu, Mar 26, 2009 at 12:27:17PM +0000, Jamie Lokier wrote:
> Marcelo Tosatti wrote:
> > Use a pipe to signal pending work for the iothread.
> 
> > +void main_loop_break(void)
> > +{
> > +    uint64_t value = 1;
> > +    char buffer[8];
> > +    size_t offset = 0;
> > +
> > +    if (io_thread_fd == -1)
> > +        return;
> > +
> > +    memcpy(buffer, &value, sizeof(value));
> > +
> > +    while (offset < 8) {
> > +        ssize_t len;
> > +
> > +        len = write(io_thread_fd, buffer + offset, 8 - offset);
> > +        if (len == -1 && errno == EINTR)
> > +            continue;
> > +
> > +        if (len <= 0)
> > +            break;
> > +
> > +        offset += len;
> > +    }
> > +
> > +    if (offset != 8)
> > +        fprintf(stderr, "failed to notify io thread\n");
> 
> 1: Why do you write 8 bytes instead of 1 byte?  If you're thinking of
>    passing a value at some point, you could change it later when it's
>    needed.  But beware that requiring the pipe to hold all values
>    written is what made Netscape 4 lock up on too-new Linux kernels
>    some 10 years ago :-)
> 
> 2: Do you know that writes <= PIPE_BUF are atomic so you don't need
>    the offset calculation?

I didnt know. Anthony wrote this code, so I'll let him comment.

> > +/* Used to break IO thread out of select */
> > +static void io_thread_wakeup(void *opaque)
> > +{
> > +    int fd = (unsigned long)opaque;
> > +    char buffer[8];
> > +    size_t offset = 0;
> > +
> > +    while (offset < 8) {
> > +        ssize_t len;
> > +
> > +        len = read(fd, buffer + offset, 8 - offset);
> > +        if (len == -1 && errno == EINTR)
> > +            continue;
> > +
> > +        if (len <= 0)
> > +            break;
> > +
> > +        offset += len;
> > +    }
> > +}
> 
> You should read until the pipe is empty, in case more than one signal
> was raised and called main_loop_break() between calls to
> io_thread_wait().
> 
> Since reads <= PIPE_BUF are atomic too, the easiest way to do that is
> try to read at least one more byte than you wrote per
> main_loop_break() call, and if you don't get that many, you've emptied
> the pipe.
> 
> > +static void setup_iothread_fd(void)
> > +{
> > +    int fds[2];
> > +
> > +    if (pipe(fds) == -1) {
> > +        fprintf(stderr, "failed to create iothread pipe");
> > +        exit(0);
> > +    }
> > +
> > +    qemu_set_fd_handler2(fds[0], NULL, io_thread_wakeup, NULL,
> > +                         (void *)(unsigned long)fds[0]);
> > +    io_thread_fd = fds[1];
> > +}
> 
> To avoid deadlock in perverse conditions where lots of signals call
> main_loop_break() enough to fill the pipe before it's read, set the
> write side non-blocking.

The pipe is set nonblocking by a later patch. Perhaps the hunk should be 
part of this patch.

> To use the trick of reading more bytes than you expect to detect EOF
> without an extra read, set the read side non-blocking too.