Comments

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(-)

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

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