From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1LpWSz-0003VO-35
	for qemu-devel@nongnu.org; Thu, 02 Apr 2009 19:37:01 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1LpWSs-0003RM-KA
	for qemu-devel@nongnu.org; Thu, 02 Apr 2009 19:36:59 -0400
Received: from [199.232.76.173] (port=40039 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1LpWSs-0003RH-D8
	for qemu-devel@nongnu.org; Thu, 02 Apr 2009 19:36:54 -0400
Received: from mx2.redhat.com ([66.187.237.31]:45421)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <mtosatti@redhat.com>) id 1LpWSr-0007ZC-Uk
	for qemu-devel@nongnu.org; Thu, 02 Apr 2009 19:36:54 -0400
Date: Thu, 2 Apr 2009 20:36:21 -0300
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [Qemu-devel] [patch 04/10] qemu: introduce main_loop_break
Message-ID: <20090402233621.GA4599@amt.cnet>
References: <20090325224714.853788328@amt.cnet>
	<20090325225438.990466524@amt.cnet>
	<20090326122717.GB20866@shareable.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20090326122717.GB20866@shareable.org>
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Jamie Lokier <jamie@shareable.org>
Cc: qemu-devel@nongnu.org

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.