| Message ID | 20110113121458.4487.925.stgit@localhost6.localdomain6 |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj <arun@linux.vnet.ibm.com> wrote: > +static void threadlet_io_completion_signal_handler(int signum) > +{ > + qemu_service_io(); > +} > + > +static void threadlet_register_signal_handler(void) > +{ > + struct sigaction act; > + sigfillset(&act.sa_mask); > + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ > + act.sa_handler = threadlet_io_completion_signal_handler; > + sigaction(SIGUSR2, &act, NULL); > +} > + > +void threadlet_init(void) > +{ > + threadlet_register_signal_handler(); > +} This would be the right place to create qemu-threadlet.c, instead of adding the thread_init() prototype to qemu-thread.h and then including that in vl.c. Stefan
On 01/17/2011 10:56 AM, Stefan Hajnoczi wrote:
> This would be the right place to create qemu-threadlet.c,
You mean .h?
Paolo
On Mon, Jan 17, 2011 at 3:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 01/17/2011 10:56 AM, Stefan Hajnoczi wrote: >> >> This would be the right place to create qemu-threadlet.c, > > You mean .h? We need both qemu-threadlet.c and qemu-threadlet.h. Stefan
* Stefan Hajnoczi <stefanha@gmail.com> [2011-01-17 09:56:58]: > On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj > <arun@linux.vnet.ibm.com> wrote: > > +static void threadlet_io_completion_signal_handler(int signum) > > +{ > > + qemu_service_io(); > > +} > > + > > +static void threadlet_register_signal_handler(void) > > +{ > > + struct sigaction act; > > + sigfillset(&act.sa_mask); > > + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ > > + act.sa_handler = threadlet_io_completion_signal_handler; > > + sigaction(SIGUSR2, &act, NULL); > > +} > > + > > +void threadlet_init(void) > > +{ > > + threadlet_register_signal_handler(); > > +} > > This would be the right place to create qemu-threadlet.c, instead of > adding the thread_init() prototype to qemu-thread.h and then including > that in vl.c. > > Stefan I did not follow your comment here. How can we avoid including threadler_init() in vl.c? -arun
On Tue, Jan 18, 2011 at 4:43 AM, Arun R Bharadwaj <arun@linux.vnet.ibm.com> wrote: > * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-17 09:56:58]: > >> On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj >> <arun@linux.vnet.ibm.com> wrote: >> > +static void threadlet_io_completion_signal_handler(int signum) >> > +{ >> > + qemu_service_io(); >> > +} >> > + >> > +static void threadlet_register_signal_handler(void) >> > +{ >> > + struct sigaction act; >> > + sigfillset(&act.sa_mask); >> > + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ >> > + act.sa_handler = threadlet_io_completion_signal_handler; >> > + sigaction(SIGUSR2, &act, NULL); >> > +} >> > + >> > +void threadlet_init(void) >> > +{ >> > + threadlet_register_signal_handler(); >> > +} >> >> This would be the right place to create qemu-threadlet.c, instead of >> adding the thread_init() prototype to qemu-thread.h and then including >> that in vl.c. >> >> Stefan > > I did not follow your comment here. How can we avoid including > threadler_init() in vl.c? Instead of adding threadlet_init() and related functions to posix-aio-compat.c and adding the prototype to qemu-thread.h, why not just create qemu-threadlet.c/qemu-threadlet.h and put these functions there instead? Stefan
* Stefan Hajnoczi <stefanha@gmail.com> [2011-01-18 06:31:34]: > On Tue, Jan 18, 2011 at 4:43 AM, Arun R Bharadwaj > <arun@linux.vnet.ibm.com> wrote: > > * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-17 09:56:58]: > > > >> On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj > >> <arun@linux.vnet.ibm.com> wrote: > >> > +static void threadlet_io_completion_signal_handler(int signum) > >> > +{ > >> > + qemu_service_io(); > >> > +} > >> > + > >> > +static void threadlet_register_signal_handler(void) > >> > +{ > >> > + struct sigaction act; > >> > + sigfillset(&act.sa_mask); > >> > + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ > >> > + act.sa_handler = threadlet_io_completion_signal_handler; > >> > + sigaction(SIGUSR2, &act, NULL); > >> > +} > >> > + > >> > +void threadlet_init(void) > >> > +{ > >> > + threadlet_register_signal_handler(); > >> > +} > >> > >> This would be the right place to create qemu-threadlet.c, instead of > >> adding the thread_init() prototype to qemu-thread.h and then including > >> that in vl.c. > >> > >> Stefan > > > > I did not follow your comment here. How can we avoid including > > threadler_init() in vl.c? > > Instead of adding threadlet_init() and related functions to > posix-aio-compat.c and adding the prototype to qemu-thread.h, why not > just create qemu-threadlet.c/qemu-threadlet.h and put these functions > there instead? > > Stefan Got it. So you mean I merge patch 8 and patch 10 into a single patch. But wouldn't this mean we are moving code and adding new API in the same patch? Anthony did not want this from what I recall. But I can do it if you feel it makes things simple. -arun
On Tue, Jan 18, 2011 at 6:46 AM, Arun R Bharadwaj <arun@linux.vnet.ibm.com> wrote: > * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-18 06:31:34]: > >> On Tue, Jan 18, 2011 at 4:43 AM, Arun R Bharadwaj >> <arun@linux.vnet.ibm.com> wrote: >> > * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-17 09:56:58]: >> > >> >> On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj >> >> <arun@linux.vnet.ibm.com> wrote: >> >> > +static void threadlet_io_completion_signal_handler(int signum) >> >> > +{ >> >> > + qemu_service_io(); >> >> > +} >> >> > + >> >> > +static void threadlet_register_signal_handler(void) >> >> > +{ >> >> > + struct sigaction act; >> >> > + sigfillset(&act.sa_mask); >> >> > + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ >> >> > + act.sa_handler = threadlet_io_completion_signal_handler; >> >> > + sigaction(SIGUSR2, &act, NULL); >> >> > +} >> >> > + >> >> > +void threadlet_init(void) >> >> > +{ >> >> > + threadlet_register_signal_handler(); >> >> > +} >> >> >> >> This would be the right place to create qemu-threadlet.c, instead of >> >> adding the thread_init() prototype to qemu-thread.h and then including >> >> that in vl.c. >> >> >> >> Stefan >> > >> > I did not follow your comment here. How can we avoid including >> > threadler_init() in vl.c? >> >> Instead of adding threadlet_init() and related functions to >> posix-aio-compat.c and adding the prototype to qemu-thread.h, why not >> just create qemu-threadlet.c/qemu-threadlet.h and put these functions >> there instead? >> >> Stefan > > Got it. So you mean I merge patch 8 and patch 10 into a single patch. > But wouldn't this mean we are moving code and adding new API in the > same patch? Anthony did not want this from what I recall. But I can do > it if you feel it makes things simple. I don't think you need to merge the patches. It's odd to add functions to posix-aio-compat.c but put the prototype in qemu-thread.h (and then include and call from vl.c). So for these three functions (threadlet_init, threadlet_register_signal_handler, and threadlet_io_completion_signal_handler) only I think it makes sense to move them to qemu-threadlet.[ch] straight away. It's not the end of the world if you don't want to do that. It just jumped out at me when reading and I had to remember that it'll get fixed up later. Stefan
On 1/17/2011 11:14 PM, Stefan Hajnoczi wrote: > On Tue, Jan 18, 2011 at 6:46 AM, Arun R Bharadwaj > <arun@linux.vnet.ibm.com> wrote: >> * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-18 06:31:34]: >> >>> On Tue, Jan 18, 2011 at 4:43 AM, Arun R Bharadwaj >>> <arun@linux.vnet.ibm.com> wrote: >>>> * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-17 09:56:58]: >>>> >>>>> On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj >>>>> <arun@linux.vnet.ibm.com> wrote: >>>>>> +static void threadlet_io_completion_signal_handler(int signum) >>>>>> +{ >>>>>> + qemu_service_io(); >>>>>> +} >>>>>> + >>>>>> +static void threadlet_register_signal_handler(void) >>>>>> +{ >>>>>> + struct sigaction act; >>>>>> + sigfillset(&act.sa_mask); >>>>>> + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ >>>>>> + act.sa_handler = threadlet_io_completion_signal_handler; >>>>>> + sigaction(SIGUSR2, &act, NULL); >>>>>> +} >>>>>> + >>>>>> +void threadlet_init(void) >>>>>> +{ >>>>>> + threadlet_register_signal_handler(); >>>>>> +} >>>>> >>>>> This would be the right place to create qemu-threadlet.c, instead of >>>>> adding the thread_init() prototype to qemu-thread.h and then including >>>>> that in vl.c. >>>>> >>>>> Stefan >>>> >>>> I did not follow your comment here. How can we avoid including >>>> threadler_init() in vl.c? >>> >>> Instead of adding threadlet_init() and related functions to >>> posix-aio-compat.c and adding the prototype to qemu-thread.h, why not >>> just create qemu-threadlet.c/qemu-threadlet.h and put these functions >>> there instead? >>> >>> Stefan >> >> Got it. So you mean I merge patch 8 and patch 10 into a single patch. >> But wouldn't this mean we are moving code and adding new API in the >> same patch? Anthony did not want this from what I recall. But I can do >> it if you feel it makes things simple. > > I don't think you need to merge the patches. It's odd to add > functions to posix-aio-compat.c but put the prototype in qemu-thread.h > (and then include and call from vl.c). So for these three functions > (threadlet_init, threadlet_register_signal_handler, and > threadlet_io_completion_signal_handler) only I think it makes sense to > move them to qemu-threadlet.[ch] straight away. So basically create the new file qemu-threadlet.[ch] with only these functions and move the rest of the code in patch 10(as we do now). > > It's not the end of the world if you don't want to do that. It just > jumped out at me when reading and I had to remember that it'll get > fixed up later. Yes; as things are settling down towards the end of this series; I guess it is OK if you decide not to alter the sequence of the patches and avoid merges. Thanks, JV > > Stefan >
On Tue, Jan 18, 2011 at 6:10 PM, Venkateswararao Jujjuri (JV) <jvrao@linux.vnet.ibm.com> wrote: > On 1/17/2011 11:14 PM, Stefan Hajnoczi wrote: >> On Tue, Jan 18, 2011 at 6:46 AM, Arun R Bharadwaj >> <arun@linux.vnet.ibm.com> wrote: >>> * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-18 06:31:34]: >>> >>>> On Tue, Jan 18, 2011 at 4:43 AM, Arun R Bharadwaj >>>> <arun@linux.vnet.ibm.com> wrote: >>>>> * Stefan Hajnoczi <stefanha@gmail.com> [2011-01-17 09:56:58]: >>>>> >>>>>> On Thu, Jan 13, 2011 at 12:14 PM, Arun R Bharadwaj >>>>>> <arun@linux.vnet.ibm.com> wrote: >>>>>>> +static void threadlet_io_completion_signal_handler(int signum) >>>>>>> +{ >>>>>>> + qemu_service_io(); >>>>>>> +} >>>>>>> + >>>>>>> +static void threadlet_register_signal_handler(void) >>>>>>> +{ >>>>>>> + struct sigaction act; >>>>>>> + sigfillset(&act.sa_mask); >>>>>>> + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ >>>>>>> + act.sa_handler = threadlet_io_completion_signal_handler; >>>>>>> + sigaction(SIGUSR2, &act, NULL); >>>>>>> +} >>>>>>> + >>>>>>> +void threadlet_init(void) >>>>>>> +{ >>>>>>> + threadlet_register_signal_handler(); >>>>>>> +} >>>>>> >>>>>> This would be the right place to create qemu-threadlet.c, instead of >>>>>> adding the thread_init() prototype to qemu-thread.h and then including >>>>>> that in vl.c. >>>>>> >>>>>> Stefan >>>>> >>>>> I did not follow your comment here. How can we avoid including >>>>> threadler_init() in vl.c? >>>> >>>> Instead of adding threadlet_init() and related functions to >>>> posix-aio-compat.c and adding the prototype to qemu-thread.h, why not >>>> just create qemu-threadlet.c/qemu-threadlet.h and put these functions >>>> there instead? >>>> >>>> Stefan >>> >>> Got it. So you mean I merge patch 8 and patch 10 into a single patch. >>> But wouldn't this mean we are moving code and adding new API in the >>> same patch? Anthony did not want this from what I recall. But I can do >>> it if you feel it makes things simple. >> >> I don't think you need to merge the patches. It's odd to add >> functions to posix-aio-compat.c but put the prototype in qemu-thread.h >> (and then include and call from vl.c). So for these three functions >> (threadlet_init, threadlet_register_signal_handler, and >> threadlet_io_completion_signal_handler) only I think it makes sense to >> move them to qemu-threadlet.[ch] straight away. > > So basically create the new file qemu-threadlet.[ch] with only these functions > and move the rest of the code in patch 10(as we do now). Exactly. Stefan
diff --git a/posix-aio-compat.c b/posix-aio-compat.c index 8c5bb46..94dd007 100644 --- a/posix-aio-compat.c +++ b/posix-aio-compat.c @@ -327,11 +327,14 @@ static void *threadlet_worker(void *data) return NULL; } +static PosixAioState *posix_aio_state; + static void handle_work(ThreadletWork *work) { pid_t pid; ssize_t ret = 0; struct qemu_paiocb *aiocb; + char byte = 0; pid = getpid(); qemu_mutex_lock(&aiocb_mutex); @@ -360,11 +363,35 @@ static void handle_work(ThreadletWork *work) qemu_cond_broadcast(&aiocb_completion); qemu_mutex_unlock(&aiocb_mutex); + ret = write(posix_aio_state->wfd, &byte, sizeof(byte)); + if (ret < 0 && errno != EAGAIN) { + die("write()"); + } + if (kill(pid, aiocb->ev_signo)) { die("kill failed"); } } +static void threadlet_io_completion_signal_handler(int signum) +{ + qemu_service_io(); +} + +static void threadlet_register_signal_handler(void) +{ + struct sigaction act; + sigfillset(&act.sa_mask); + act.sa_flags = 0; /* do not restart syscalls to interrupt select() */ + act.sa_handler = threadlet_io_completion_signal_handler; + sigaction(SIGUSR2, &act, NULL); +} + +void threadlet_init(void) +{ + threadlet_register_signal_handler(); +} + static void spawn_threadlet(ThreadletQueue *queue) { QemuThread thread; @@ -520,22 +547,6 @@ static int posix_aio_flush(void *opaque) return !!s->first_aio; } -static PosixAioState *posix_aio_state; - -static void aio_signal_handler(int signum) -{ - 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()"); - } - - qemu_service_io(); -} - static void paio_remove(struct qemu_paiocb *acb) { struct qemu_paiocb **pacb; @@ -642,7 +653,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd, int paio_init(void) { - struct sigaction act; PosixAioState *s; int fds[2]; @@ -654,11 +664,6 @@ int paio_init(void) s = qemu_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"); diff --git a/qemu-thread.h b/qemu-thread.h index 19bb30c..c5b579c 100644 --- a/qemu-thread.h +++ b/qemu-thread.h @@ -40,5 +40,6 @@ void qemu_thread_signal(QemuThread *thread, int sig); void qemu_thread_self(QemuThread *thread); int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2); void qemu_thread_exit(void *retval); +void threadlet_init(void); #endif diff --git a/vl.c b/vl.c index df414ef..aba805f 100644 --- a/vl.c +++ b/vl.c @@ -148,6 +148,7 @@ int main(int argc, char **argv) #include "qemu-config.h" #include "qemu-objects.h" #include "qemu-options.h" +#include "qemu-thread.h" #ifdef CONFIG_VIRTFS #include "fsdev/qemu-fsdev.h" #endif @@ -2922,6 +2923,8 @@ int main(int argc, char **argv, char **envp) exit(1); } + threadlet_init(); + /* init generic devices */ if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 1) != 0) exit(1);
This patch adds aio_signal_handler threadlet API. Earler posix-aio-compat.c had its own signal handler code. Now abstract this, in the later patch it is moved to a generic code so that it can be used by other subsystems. Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com> --- posix-aio-compat.c | 49 +++++++++++++++++++++++++++---------------------- qemu-thread.h | 1 + vl.c | 3 +++ 3 files changed, 31 insertions(+), 22 deletions(-)