| Message ID | 1316443033-6489-1-git-send-email-freddy77@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 09/19/2011 04:37 PM, Frediano Ziglio wrote: > Now that iothread is always compiled sending a signal seems only an > additional step. This patch also avoid writing to two pipe (one from signal > and one in qemu_service_io). > > Work with kvm enabled or disabled. strace output is more readable (less syscalls). > > Signed-off-by: Frediano Ziglio<freddy77@gmail.com> > --- > cpus.c | 5 ----- > posix-aio-compat.c | 29 +++++++++-------------------- > 2 files changed, 9 insertions(+), 25 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 54c188c..d0cfe91 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -380,11 +380,6 @@ static int qemu_signal_init(void) > int sigfd; > sigset_t set; > > - /* SIGUSR2 used by posix-aio-compat.c */ > - sigemptyset(&set); > - sigaddset(&set, SIGUSR2); > - pthread_sigmask(SIG_UNBLOCK,&set, NULL); > - > /* > * SIG_IPI must be blocked in the main thread and must not be caught > * by sigwait() in the signal thread. Otherwise, the cpu thread will > diff --git a/posix-aio-compat.c b/posix-aio-compat.c > index 3193dbf..185d5b2 100644 > --- a/posix-aio-compat.c > +++ b/posix-aio-compat.c > @@ -42,7 +42,6 @@ struct qemu_paiocb { > int aio_niov; > size_t aio_nbytes; > #define aio_ioctl_cmd aio_nbytes /* for QEMU_AIO_IOCTL */ > - int ev_signo; > off_t aio_offset; > > QTAILQ_ENTRY(qemu_paiocb) node; > @@ -309,6 +308,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb) > return nbytes; > } > > +static void posix_aio_notify_event(void); > + > static void *aio_thread(void *unused) > { > pid_t pid; > @@ -381,7 +382,7 @@ static void *aio_thread(void *unused) > aiocb->ret = ret; > mutex_unlock(&lock); > > - if (kill(pid, aiocb->ev_signo)) die("kill failed"); > + posix_aio_notify_event(); > } > > cur_threads--; > @@ -548,18 +549,14 @@ static int posix_aio_flush(void *opaque) > > static PosixAioState *posix_aio_state; > > -static void aio_signal_handler(int signum) > +static void posix_aio_notify_event(void) > { > - if (posix_aio_state) { > - char byte = 0; > - ssize_t ret; > - > - ret = write(posix_aio_state->wfd,&byte, sizeof(byte)); > - if (ret< 0&& errno != EAGAIN) > - die("write()"); > - } > + char byte = 0; > + ssize_t ret; > > - qemu_service_io(); > + ret = write(posix_aio_state->wfd,&byte, sizeof(byte)); > + if (ret< 0&& errno != EAGAIN) > + die("write()"); > } > > static void paio_remove(struct qemu_paiocb *acb) > @@ -623,7 +620,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd, > return NULL; > acb->aio_type = type; > acb->aio_fildes = fd; > - acb->ev_signo = SIGUSR2; > > if (qiov) { > acb->aio_iov = qiov->iov; > @@ -651,7 +647,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, > return NULL; > acb->aio_type = QEMU_AIO_IOCTL; > acb->aio_fildes = fd; > - acb->ev_signo = SIGUSR2; > acb->aio_offset = 0; > acb->aio_ioctl_buf = buf; > acb->aio_ioctl_cmd = req; > @@ -665,7 +660,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, > > int paio_init(void) > { > - struct sigaction act; > PosixAioState *s; > int fds[2]; > int ret; > @@ -675,11 +669,6 @@ int paio_init(void) > > s = g_malloc(sizeof(PosixAioState)); > > - sigfillset(&act.sa_mask); > - act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ > - act.sa_handler = aio_signal_handler; > - sigaction(SIGUSR2,&act, NULL); > - > s->first_aio = NULL; > if (qemu_pipe(fds) == -1) { > fprintf(stderr, "failed to create pipe\n"); I think it is possible to go a step further, turn posix_aio_process_queue into a bottom half and get rid of the pipe altogether. This in turn would remove the only real user of io_process_queue in qemu_aio_set_fd_handler. However, this is already a nice improvement. Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Am 19.09.2011 17:02, schrieb Paolo Bonzini: > On 09/19/2011 04:37 PM, Frediano Ziglio wrote: >> Now that iothread is always compiled sending a signal seems only an >> additional step. This patch also avoid writing to two pipe (one from signal >> and one in qemu_service_io). >> >> Work with kvm enabled or disabled. strace output is more readable (less syscalls). >> >> Signed-off-by: Frediano Ziglio<freddy77@gmail.com> >> --- >> cpus.c | 5 ----- >> posix-aio-compat.c | 29 +++++++++-------------------- >> 2 files changed, 9 insertions(+), 25 deletions(-) >> >> diff --git a/cpus.c b/cpus.c >> index 54c188c..d0cfe91 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -380,11 +380,6 @@ static int qemu_signal_init(void) >> int sigfd; >> sigset_t set; >> >> - /* SIGUSR2 used by posix-aio-compat.c */ >> - sigemptyset(&set); >> - sigaddset(&set, SIGUSR2); >> - pthread_sigmask(SIG_UNBLOCK,&set, NULL); >> - >> /* >> * SIG_IPI must be blocked in the main thread and must not be caught >> * by sigwait() in the signal thread. Otherwise, the cpu thread will >> diff --git a/posix-aio-compat.c b/posix-aio-compat.c >> index 3193dbf..185d5b2 100644 >> --- a/posix-aio-compat.c >> +++ b/posix-aio-compat.c >> @@ -42,7 +42,6 @@ struct qemu_paiocb { >> int aio_niov; >> size_t aio_nbytes; >> #define aio_ioctl_cmd aio_nbytes /* for QEMU_AIO_IOCTL */ >> - int ev_signo; >> off_t aio_offset; >> >> QTAILQ_ENTRY(qemu_paiocb) node; >> @@ -309,6 +308,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb) >> return nbytes; >> } >> >> +static void posix_aio_notify_event(void); >> + >> static void *aio_thread(void *unused) >> { >> pid_t pid; >> @@ -381,7 +382,7 @@ static void *aio_thread(void *unused) >> aiocb->ret = ret; >> mutex_unlock(&lock); >> >> - if (kill(pid, aiocb->ev_signo)) die("kill failed"); >> + posix_aio_notify_event(); >> } >> >> cur_threads--; >> @@ -548,18 +549,14 @@ static int posix_aio_flush(void *opaque) >> >> static PosixAioState *posix_aio_state; >> >> -static void aio_signal_handler(int signum) >> +static void posix_aio_notify_event(void) >> { >> - if (posix_aio_state) { >> - char byte = 0; >> - ssize_t ret; >> - >> - ret = write(posix_aio_state->wfd,&byte, sizeof(byte)); >> - if (ret< 0&& errno != EAGAIN) >> - die("write()"); >> - } >> + char byte = 0; >> + ssize_t ret; >> >> - qemu_service_io(); >> + ret = write(posix_aio_state->wfd,&byte, sizeof(byte)); >> + if (ret< 0&& errno != EAGAIN) >> + die("write()"); >> } >> >> static void paio_remove(struct qemu_paiocb *acb) >> @@ -623,7 +620,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd, >> return NULL; >> acb->aio_type = type; >> acb->aio_fildes = fd; >> - acb->ev_signo = SIGUSR2; >> >> if (qiov) { >> acb->aio_iov = qiov->iov; >> @@ -651,7 +647,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, >> return NULL; >> acb->aio_type = QEMU_AIO_IOCTL; >> acb->aio_fildes = fd; >> - acb->ev_signo = SIGUSR2; >> acb->aio_offset = 0; >> acb->aio_ioctl_buf = buf; >> acb->aio_ioctl_cmd = req; >> @@ -665,7 +660,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, >> >> int paio_init(void) >> { >> - struct sigaction act; >> PosixAioState *s; >> int fds[2]; >> int ret; >> @@ -675,11 +669,6 @@ int paio_init(void) >> >> s = g_malloc(sizeof(PosixAioState)); >> >> - sigfillset(&act.sa_mask); >> - act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ >> - act.sa_handler = aio_signal_handler; >> - sigaction(SIGUSR2,&act, NULL); >> - >> s->first_aio = NULL; >> if (qemu_pipe(fds) == -1) { >> fprintf(stderr, "failed to create pipe\n"); > > I think it is possible to go a step further, turn > posix_aio_process_queue into a bottom half and get rid of the pipe > altogether. This in turn would remove the only real user of > io_process_queue in qemu_aio_set_fd_handler. However, this is already a > nice improvement. But without the fd, wouldn't the I/O thread possibly wait for much longer until its select() times out and it starts processing BHs? Kevin
Am 19.09.2011 16:37, schrieb Frediano Ziglio: > Now that iothread is always compiled sending a signal seems only an > additional step. This patch also avoid writing to two pipe (one from signal > and one in qemu_service_io). > > Work with kvm enabled or disabled. strace output is more readable (less syscalls). > > Signed-off-by: Frediano Ziglio <freddy77@gmail.com> Thanks applied to the block branch. Kevin
On 09/19/2011 05:11 PM, Kevin Wolf wrote: >> > I think it is possible to go a step further, turn >> > posix_aio_process_queue into a bottom half and get rid of the pipe >> > altogether. This in turn would remove the only real user of >> > io_process_queue in qemu_aio_set_fd_handler. However, this is already a >> > nice improvement. > But without the fd, wouldn't the I/O thread possibly wait for much > longer until its select() times out and it starts processing BHs? Hmm, in qemu_aio_wait yes... In the normal qemu event loop, however, bottom halves exit the select loop with qemu_notify_event(). qemu currently has a 1-second timeout for the select, but it should work just as well with an infinite timeout. If it doesn't, it's a bug. It should be possible to turn posix_aio_process_queue into a bottom half, but the pipe is still necessary in order to exit the qemu_aio_wait select loop and schedule the bottom half. Paolo
Am 19.09.2011 16:37, schrieb Frediano Ziglio: > Now that iothread is always compiled sending a signal seems only an > additional step. This patch also avoid writing to two pipe (one from signal > and one in qemu_service_io). > > Work with kvm enabled or disabled. strace output is more readable (less syscalls). > > Signed-off-by: Frediano Ziglio <freddy77@gmail.com> Something in this change has bad effects, in the sense that it seems to break bdrv_read_em. To be precise, what I'm testing is booting from a DOS installation floppy (interestingly, on my laptop it just works, but on my other test box it fails). The first attempt of git bisect pointed at the commit where we converted bdrv_read/write to coroutines. However, it turned out that the conversion commit only caused problems because instead of using a synchronous read() it now goes through posix-aio-compat.c. The problem is reproducible in pre-coroutine versions by just commenting out .bdrv_read/.bdrv_write in raw-posix. Going back a bit more showed that this did work fine a while ago, and the removal of SIGUSR2 is the first commit in which bdrv_read_em didn't provide the same behaviour as bdrv_read any more. I have no idea yet what's really going wrong, but maybe it rings a bell for one of you? Kevin > --- > cpus.c | 5 ----- > posix-aio-compat.c | 29 +++++++++-------------------- > 2 files changed, 9 insertions(+), 25 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 54c188c..d0cfe91 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -380,11 +380,6 @@ static int qemu_signal_init(void) > int sigfd; > sigset_t set; > > - /* SIGUSR2 used by posix-aio-compat.c */ > - sigemptyset(&set); > - sigaddset(&set, SIGUSR2); > - pthread_sigmask(SIG_UNBLOCK, &set, NULL); > - > /* > * SIG_IPI must be blocked in the main thread and must not be caught > * by sigwait() in the signal thread. Otherwise, the cpu thread will > diff --git a/posix-aio-compat.c b/posix-aio-compat.c > index 3193dbf..185d5b2 100644 > --- a/posix-aio-compat.c > +++ b/posix-aio-compat.c > @@ -42,7 +42,6 @@ struct qemu_paiocb { > int aio_niov; > size_t aio_nbytes; > #define aio_ioctl_cmd aio_nbytes /* for QEMU_AIO_IOCTL */ > - int ev_signo; > off_t aio_offset; > > QTAILQ_ENTRY(qemu_paiocb) node; > @@ -309,6 +308,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb) > return nbytes; > } > > +static void posix_aio_notify_event(void); > + > static void *aio_thread(void *unused) > { > pid_t pid; > @@ -381,7 +382,7 @@ static void *aio_thread(void *unused) > aiocb->ret = ret; > mutex_unlock(&lock); > > - if (kill(pid, aiocb->ev_signo)) die("kill failed"); > + posix_aio_notify_event(); > } > > cur_threads--; > @@ -548,18 +549,14 @@ static int posix_aio_flush(void *opaque) > > static PosixAioState *posix_aio_state; > > -static void aio_signal_handler(int signum) > +static void posix_aio_notify_event(void) > { > - if (posix_aio_state) { > - char byte = 0; > - ssize_t ret; > - > - ret = write(posix_aio_state->wfd, &byte, sizeof(byte)); > - if (ret < 0 && errno != EAGAIN) > - die("write()"); > - } > + char byte = 0; > + ssize_t ret; > > - qemu_service_io(); > + ret = write(posix_aio_state->wfd, &byte, sizeof(byte)); > + if (ret < 0 && errno != EAGAIN) > + die("write()"); > } > > static void paio_remove(struct qemu_paiocb *acb) > @@ -623,7 +620,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd, > return NULL; > acb->aio_type = type; > acb->aio_fildes = fd; > - acb->ev_signo = SIGUSR2; > > if (qiov) { > acb->aio_iov = qiov->iov; > @@ -651,7 +647,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, > return NULL; > acb->aio_type = QEMU_AIO_IOCTL; > acb->aio_fildes = fd; > - acb->ev_signo = SIGUSR2; > acb->aio_offset = 0; > acb->aio_ioctl_buf = buf; > acb->aio_ioctl_cmd = req; > @@ -665,7 +660,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, > > int paio_init(void) > { > - struct sigaction act; > PosixAioState *s; > int fds[2]; > int ret; > @@ -675,11 +669,6 @@ int paio_init(void) > > s = g_malloc(sizeof(PosixAioState)); > > - sigfillset(&act.sa_mask); > - act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ > - act.sa_handler = aio_signal_handler; > - sigaction(SIGUSR2, &act, NULL); > - > s->first_aio = NULL; > if (qemu_pipe(fds) == -1) { > fprintf(stderr, "failed to create pipe\n");
On Thu, Oct 27, 2011 at 03:26:23PM +0200, Kevin Wolf wrote: > Am 19.09.2011 16:37, schrieb Frediano Ziglio: > > Now that iothread is always compiled sending a signal seems only an > > additional step. This patch also avoid writing to two pipe (one from signal > > and one in qemu_service_io). > > > > Work with kvm enabled or disabled. strace output is more readable (less syscalls). > > > > Signed-off-by: Frediano Ziglio <freddy77@gmail.com> > > Something in this change has bad effects, in the sense that it seems to > break bdrv_read_em. How does it break bdrv_read_em? Are you seeing QEMU hung with 100% CPU utilization or deadlocked? One interesting thing is that qemu_aio_wait() does not release the QEMU mutex, so we cannot write to a pipe with the mutex held and then spin waiting for the iothread to do work for us. Exactly how kill and qemu_notify_event() were different I'm not sure right now but it could be a factor. Stefan
Am 27.10.2011 15:57, schrieb Stefan Hajnoczi: > On Thu, Oct 27, 2011 at 03:26:23PM +0200, Kevin Wolf wrote: >> Am 19.09.2011 16:37, schrieb Frediano Ziglio: >>> Now that iothread is always compiled sending a signal seems only an >>> additional step. This patch also avoid writing to two pipe (one from signal >>> and one in qemu_service_io). >>> >>> Work with kvm enabled or disabled. strace output is more readable (less syscalls). >>> >>> Signed-off-by: Frediano Ziglio <freddy77@gmail.com> >> >> Something in this change has bad effects, in the sense that it seems to >> break bdrv_read_em. > > How does it break bdrv_read_em? Are you seeing QEMU hung with 100% CPU > utilization or deadlocked? Sorry, I should have been more detailed here. No, it's nothing obvious, it must be some subtle side effect. The result of bdrv_read_em itself seems to be correct (return value and checksum of the read buffer). However instead of booting into the DOS setup I only get an error message "Kein System oder Laufwerksfehler" (don't know how it reads in English DOS versions), which seems to be produced by the boot sector. I excluded all of the minor changes, so I'm sure that it's caused by the switch from kill() to a direct call of the function that writes into the pipe. > One interesting thing is that qemu_aio_wait() does not release the QEMU > mutex, so we cannot write to a pipe with the mutex held and then spin > waiting for the iothread to do work for us. > > Exactly how kill and qemu_notify_event() were different I'm not sure > right now but it could be a factor. This would cause a hang, right? Then it isn't what I'm seeing. Kevin
Am 27.10.2011 16:15, schrieb Kevin Wolf: > Am 27.10.2011 15:57, schrieb Stefan Hajnoczi: >> On Thu, Oct 27, 2011 at 03:26:23PM +0200, Kevin Wolf wrote: >>> Am 19.09.2011 16:37, schrieb Frediano Ziglio: >>>> Now that iothread is always compiled sending a signal seems only an >>>> additional step. This patch also avoid writing to two pipe (one from signal >>>> and one in qemu_service_io). >>>> >>>> Work with kvm enabled or disabled. strace output is more readable (less syscalls). >>>> >>>> Signed-off-by: Frediano Ziglio <freddy77@gmail.com> >>> >>> Something in this change has bad effects, in the sense that it seems to >>> break bdrv_read_em. >> >> How does it break bdrv_read_em? Are you seeing QEMU hung with 100% CPU >> utilization or deadlocked? > > Sorry, I should have been more detailed here. > > No, it's nothing obvious, it must be some subtle side effect. The result > of bdrv_read_em itself seems to be correct (return value and checksum of > the read buffer). > > However instead of booting into the DOS setup I only get an error > message "Kein System oder Laufwerksfehler" (don't know how it reads in > English DOS versions), which seems to be produced by the boot sector. > > I excluded all of the minor changes, so I'm sure that it's caused by the > switch from kill() to a direct call of the function that writes into the > pipe. > >> One interesting thing is that qemu_aio_wait() does not release the QEMU >> mutex, so we cannot write to a pipe with the mutex held and then spin >> waiting for the iothread to do work for us. >> >> Exactly how kill and qemu_notify_event() were different I'm not sure >> right now but it could be a factor. > > This would cause a hang, right? Then it isn't what I'm seeing. While trying out some more things, I added some fprintfs to posix_aio_process_queue() and suddenly it also fails with the kill() version. So what has changed might really just be the timing, and it could be a race somewhere that has always (?) existed. Kevin
diff --git a/cpus.c b/cpus.c index 54c188c..d0cfe91 100644 --- a/cpus.c +++ b/cpus.c @@ -380,11 +380,6 @@ static int qemu_signal_init(void) int sigfd; sigset_t set; - /* SIGUSR2 used by posix-aio-compat.c */ - sigemptyset(&set); - sigaddset(&set, SIGUSR2); - pthread_sigmask(SIG_UNBLOCK, &set, NULL); - /* * SIG_IPI must be blocked in the main thread and must not be caught * by sigwait() in the signal thread. Otherwise, the cpu thread will diff --git a/posix-aio-compat.c b/posix-aio-compat.c index 3193dbf..185d5b2 100644 --- a/posix-aio-compat.c +++ b/posix-aio-compat.c @@ -42,7 +42,6 @@ struct qemu_paiocb { int aio_niov; size_t aio_nbytes; #define aio_ioctl_cmd aio_nbytes /* for QEMU_AIO_IOCTL */ - int ev_signo; off_t aio_offset; QTAILQ_ENTRY(qemu_paiocb) node; @@ -309,6 +308,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb) return nbytes; } +static void posix_aio_notify_event(void); + static void *aio_thread(void *unused) { pid_t pid; @@ -381,7 +382,7 @@ static void *aio_thread(void *unused) aiocb->ret = ret; mutex_unlock(&lock); - if (kill(pid, aiocb->ev_signo)) die("kill failed"); + posix_aio_notify_event(); } cur_threads--; @@ -548,18 +549,14 @@ static int posix_aio_flush(void *opaque) static PosixAioState *posix_aio_state; -static void aio_signal_handler(int signum) +static void posix_aio_notify_event(void) { - if (posix_aio_state) { - char byte = 0; - ssize_t ret; - - ret = write(posix_aio_state->wfd, &byte, sizeof(byte)); - if (ret < 0 && errno != EAGAIN) - die("write()"); - } + char byte = 0; + ssize_t ret; - qemu_service_io(); + ret = write(posix_aio_state->wfd, &byte, sizeof(byte)); + if (ret < 0 && errno != EAGAIN) + die("write()"); } static void paio_remove(struct qemu_paiocb *acb) @@ -623,7 +620,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd, return NULL; acb->aio_type = type; acb->aio_fildes = fd; - acb->ev_signo = SIGUSR2; if (qiov) { acb->aio_iov = qiov->iov; @@ -651,7 +647,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, return NULL; acb->aio_type = QEMU_AIO_IOCTL; acb->aio_fildes = fd; - acb->ev_signo = SIGUSR2; acb->aio_offset = 0; acb->aio_ioctl_buf = buf; acb->aio_ioctl_cmd = req; @@ -665,7 +660,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, int paio_init(void) { - struct sigaction act; PosixAioState *s; int fds[2]; int ret; @@ -675,11 +669,6 @@ int paio_init(void) s = g_malloc(sizeof(PosixAioState)); - sigfillset(&act.sa_mask); - act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ - act.sa_handler = aio_signal_handler; - sigaction(SIGUSR2, &act, NULL); - s->first_aio = NULL; if (qemu_pipe(fds) == -1) { fprintf(stderr, "failed to create pipe\n");
Now that iothread is always compiled sending a signal seems only an additional step. This patch also avoid writing to two pipe (one from signal and one in qemu_service_io). Work with kvm enabled or disabled. strace output is more readable (less syscalls). Signed-off-by: Frediano Ziglio <freddy77@gmail.com> --- cpus.c | 5 ----- posix-aio-compat.c | 29 +++++++++-------------------- 2 files changed, 9 insertions(+), 25 deletions(-)