Re: [Qemu-devel] QEMU question: is eventfd not thread safe?

[ronnie sahlberg]

Hi,

As Paolo said,
I hit this with block/iscsi.c a few months back.
Then about a month back I recall something that looks alkmost
identical to this hit the IDE driver on the KVM list.
( At least the symptoms looked just like my symptoms, and the KVM guys
managed to bisect it down to the exact same patch that I did that
would expose the bug in block.iscsi.c, namely the lack of calling
qemu_notify_event() )

If we have hit a problem 3 times recently due to developers not
realizing the importance of calling qemu_notify_event(), maybe the API
is suboptimal.

Would it be possible to change the set-event-handlers functions to
automatically call qemu_notify_event() when the descriptos change?
To eliminate the need to call this function at all ?


regards
ronnie sahlberg

On Mon, Jul 2, 2012 at 10:06 AM, Alexey Kardashevskiy <address@hidden> wrote:
> On 02/07/12 09:07, Benjamin Herrenschmidt wrote:
>>>>> diff --git a/iohandler.c b/iohandler.c
>>>>> index 3c74de6..54f4c48 100644
>>>>> --- a/iohandler.c
>>>>> +++ b/iohandler.c
>>>>> @@ -77,6 +77,7 @@ int qemu_set_fd_handler2(int fd,
>>>>>          ioh->fd_write = fd_write;
>>>>>          ioh->opaque = opaque;
>>>>>          ioh->deleted = 0;
>>>>> +        kill(getpid(), SIGUSR2);
>>>>>      }
>>>>>      return 0;
>>>>>  }
>>
>> That probably wants to be a pthread_kill targetted at the main loop.
>>
>>>>> +static void sigusr2_print(int signal)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static void sigusr2_init(void)
>>>>> +{
>>>>> +    struct sigaction action;
>>>>> +
>>>>> +    memset(&action, 0, sizeof(action));
>>>>> +    sigfillset(&action.sa_mask);
>>>>> +    action.sa_handler = sigusr2_print;
>>>>> +    action.sa_flags = 0;
>>>>> +    sigaction(SIGUSR2, &action, NULL);
>>>>> +}
>>>>> +
>>
>> Won't that conflict with the business in coroutine-sigaltstack.c ?
>
> The code which touches SIGUSR2 does not compile on power.
>
>> Hrm... looking at it, it looks like it will save/restore the handler,
>> so that should be good.
>>
>> Still, one might want to wrap that into something, like
>> qemu_wake_main_loop();
>
>
> I already posted another patch with qemu_notify_event() in this mail thread 
> later :)
>
>
>>
>> Cheers,
>> Ben.
>>
>>>>>  int main_loop_init(void)
>>>>>  {
>>>>>      int ret;
>>>>>
>>>>> +    sigusr2_init();
>>>>> +
>>>>>      qemu_mutex_lock_iothread();
>>>>>      ret = qemu_signal_init();
>>>>>      if (ret) {
>>>>> --
>>>>> 1.7.10
>>>
>>>
>>
>>
>
>
> --
> Alexey
>