Re: [PATCH][RFC] Use pipe() to simulate signalfd()

[Anthony Liguori <aliguori@us.ibm.com>]

Marcelo Tosatti wrote:
> Hi Anthony,
>
> How is -no-kvm-irqchip working with the patch?
>   

Seems to work fine.  What is your expectation?

> On Tue, Apr 29, 2008 at 09:28:14AM -0500, Anthony Liguori wrote:
>   
>> This patch eliminates the use of sigtimedwait() in the IO thread.  To avoid the
>> signal/select race condition, we use a pipe that we write to in the signal
>> handlers.  This was suggested by Rusty and seems to work well.
>>
>> +static int kvm_eat_signal(CPUState *env, int timeout)
>>  {
>>      struct timespec ts;
>>      int r, e, ret = 0;
>>      siginfo_t siginfo;
>> +    sigset_t waitset;
>>  
>> +    sigemptyset(&waitset);
>> +    sigaddset(&waitset, SIG_IPI);
>>      ts.tv_sec = timeout / 1000;
>>      ts.tv_nsec = (timeout % 1000) * 1000000;
>> -    r = sigtimedwait(&waitset->sigset, &siginfo, &ts);
>> +    qemu_kvm_unlock();
>> +    r = sigtimedwait(&waitset, &siginfo, &ts);
>> +    qemu_kvm_lock(env);
>> +    cpu_single_env = env;
>>     
>
> This assignment seems redundant now.
>   

Yeah, I have a bigger patch which eliminates all of the explicit 
assignments to cpu_single_env.

>>  
>> @@ -263,12 +238,8 @@ static void pause_all_threads(void)
>>  	vcpu_info[i].stop = 1;
>>  	pthread_kill(vcpu_info[i].thread, SIG_IPI);
>>     
>
> Make sure the IO thread has SIG_IPI blocked (those are for APIC vcpu
> initialization only).
>   

Just so I'm clear, there's really no harm in not blocking SIG_IPI 
because it would just be ignored by the IO thread (since the SIG_IPI 
handler is a nop).  But yeah, we should explicitly block it.

>> +static void sig_aio_fd_read(void *opaque)
>> +{
>> +    int signum;
>> +    ssize_t len;
>> +
>> +    do { 
>> +	len = read(kvm_sigfd[0], &signum, sizeof(signum));
>> +    } while (len == -1 && errno == EINTR);
>>     
>
> What is the reason for this loop instead of a straight read? 
>
> Its alright to be interrupted by a signal.
>   

Just general habit with QEMU.

>> +    signal(SIGUSR1, sig_aio_handler);
>> +    signal(SIGUSR2, sig_aio_handler);
>> +    signal(SIGALRM, sig_aio_handler);
>> +    signal(SIGIO, sig_aio_handler);
>> +
>> +    if (pipe(kvm_sigfd) == -1)
>> +	abort();
>>     
>
> perror() would be nice.
>   

Yeah, everything needs proper error handling.

>> -        kvm_eat_signal(&io_signal_table, NULL, 1000);
>>          pthread_mutex_lock(&qemu_mutex);
>> -        cpu_single_env = NULL;
>> -        main_loop_wait(0);
>> +	main_loop_wait(10);
>>     
>
> Increase that 1000 or something. Will make it easier to spot bugs.
>   

I have actually and it does introduce some bugs.  I'm not entirely clear 
what is causing them though.

Regards,

Anthony Liguori

> Similarly in qemu_kvm_aio_wait().
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone