Re: [Qemu-devel] Re: [PATCH v3 14/22] kvm: Fix race between timer signals and vcpu entry under !IOTHREAD

[Stefan Hajnoczi]

On Mon, Jan 31, 2011 at 12:18 PM, Jan Kiszka <address@hidden> wrote:
> On 2011-01-31 13:13, Stefan Hajnoczi wrote:
>> On Mon, Jan 31, 2011 at 11:27 AM, Jan Kiszka <address@hidden> wrote:
>>> On 2011-01-31 11:03, Avi Kivity wrote:
>>>> On 01/27/2011 04:33 PM, Jan Kiszka wrote:
>>>>> Found by Stefan Hajnoczi: There is a race in kvm_cpu_exec between
>>>>> checking for exit_request on vcpu entry and timer signals arriving
>>>>> before KVM starts to catch them. Plug it by blocking both timer related
>>>>> signals also on !CONFIG_IOTHREAD and process those via signalfd.
>>>>>
>>>>> As this fix depends on real signalfd support (otherwise the timer
>>>>> signals only kick the compat helper thread, and the main thread hangs),
>>>>> we need to detect the invalid constellation and abort configure.
>>>>>
>>>>> Signed-off-by: Jan Kiszka<address@hidden>
>>>>> CC: Stefan Hajnoczi<address@hidden>
>>>>> ---
>>>>>
>>>>> I don't want to invest that much into !IOTHREAD anymore, so let's see if
>>>>> the proposed catch&abort is acceptable.
>>>>>
>>>>
>>>> I don't understand the dependency on signalfd.  The normal way of doing
>>>> things, either waiting for the signal in sigtimedwait() or in
>>>> ioctl(KVM_RUN), works with SIGALRM just fine.
>>>
>>> And how would you be kicked out of the select() call if it is waiting
>>> with a timeout? We only have a single thread here.
>>>
>>> The only alternative is Stefan's original proposal. But that required
>>> fiddling with the signal mask twice per KVM_RUN.
>>
>> I think my original patch messed with the sigmask in the wrong place,
>> as you mentioned doing it twice per KVM_RUN isn't a good idea.  I
>> wonder if we can enable SIGALRM only in blocking calls and guest code
>> execution but without signalfd.  It might be possible, I don't see an
>> immediate problem with doing that, we might have to use pselect(2) or
>> similar in a few places.
>
> My main concern about alternative approaches is that IOTHREAD is about
> to become the default, and hardly anyone (of the few upstream KVM users)
> will run without it in the foreseeable future. The next step will be the
> removal of any !CONFIG_IOTHREAD section. So, how much do we want to
> invest here (provided my proposal has not remaining issues)?

Yes, you're right.  I'm not volunteering to dig more into this, the
best case would be to switch to a non-I/O thread world that works for
everybody.

Stefan