[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] kvm: Prevent dynticks race condition for !CONFI
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [PATCH] kvm: Prevent dynticks race condition for !CONFIG_IOTHREAD |
Date: |
Wed, 26 Jan 2011 14:56:14 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
On 2011-01-26 14:12, Jan Kiszka wrote:
> On 2011-01-26 10:39, Stefan Hajnoczi wrote:
>> The dynticks timer arranges for SIGALRM to be raised when the next
>> pending timer expires. When building with !CONFIG_IOTHREAD, we need to
>> check whether a request to exit the vcpu is pending before re-entering
>> the guest.
>>
>> Unfortunately there is a race condition here because SIGALRM may be
>> raised after we check for an exit request but before re-entering the
>> guest. In that case the guest is re-entered without the dynticks timer
>> being rearmed.
>>
>> This results in temporary loss of timers until some other event forces a
>> vmexit. In the case of a CPU-bound guest it can cause softlockups.
>>
>> This patch blocks SIGALRM before checking for an exit request and uses
>> KVM's sigmask support to atomically unblock it when entering the guest,
>> thereby making the exit request check safe.
>>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>> cpus.c | 17 ++++++++++++++++-
>> kvm-all.c | 16 ++++++++++++++++
>> 2 files changed, 32 insertions(+), 1 deletions(-)
>>
>> Does not affect qemu-kvm.git. Still worth having in qemu.git so we don't get
>> odd behavior when building without --enable-io-thread.
>>
>> diff --git a/cpus.c b/cpus.c
>> index 0309189..59dbfab 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -262,14 +262,29 @@ void qemu_main_loop_start(void)
>> {
>> }
>>
>> +static void kvm_init_sigmask(CPUState *env)
>> +{
>> + int r;
>> + sigset_t set;
>> +
>> + pthread_sigmask(SIG_SETMASK, NULL, &set);
>> + r = kvm_set_signal_mask(env, &set);
>> + if (r) {
>> + fprintf(stderr, "kvm_set_signal_mask: %s\n", strerror(r));
>> + exit(1);
>> + }
>> +}
>> +
>> void qemu_init_vcpu(void *_env)
>> {
>> CPUState *env = _env;
>>
>> env->nr_cores = smp_cores;
>> env->nr_threads = smp_threads;
>> - if (kvm_enabled())
>> + if (kvm_enabled()) {
>> kvm_init_vcpu(env);
>> + kvm_init_sigmask(env);
>> + }
>> return;
>> }
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 255b6fa..9cc2553 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -890,19 +890,31 @@ int kvm_cpu_exec(CPUState *env)
>> {
>> struct kvm_run *run = env->kvm_run;
>> int ret;
>> +#ifndef CONFIG_IOTHREAD
>> + sigset_t set, old_set;
>> +
>> + sigemptyset(&set);
>> + sigaddset(&set, SIGALRM);
>> +#endif
>>
>> DPRINTF("kvm_cpu_exec()\n");
>>
>> do {
>> #ifndef CONFIG_IOTHREAD
>> + pthread_sigmask(SIG_BLOCK, &set, &old_set);
>> +
>> if (env->exit_request) {
>> DPRINTF("interrupt exit requested\n");
>> + pthread_sigmask(SIG_SETMASK, &old_set, NULL);
>> ret = 0;
>> break;
>> }
>> #endif
>>
>> if (kvm_arch_process_irqchip_events(env)) {
>> +#ifndef CONFIG_IOTHREAD
>> + pthread_sigmask(SIG_SETMASK, &old_set, NULL);
>> +#endif
>> ret = 0;
>> break;
>> }
>> @@ -920,6 +932,10 @@ int kvm_cpu_exec(CPUState *env)
>> cpu_single_env = env;
>> kvm_arch_post_run(env, run);
>>
>> +#ifndef CONFIG_IOTHREAD
>> + pthread_sigmask(SIG_SETMASK, &old_set, NULL);
>> +#endif
>> +
>> if (ret == -EINTR || ret == -EAGAIN) {
>> cpu_exit(env);
>> DPRINTF("io window exit\n");
>
> Good catch, but the code should be organized differently, probably with
> the help of signalfd which we need for SIGBUS anyway. I'm currently
> reworking signaling bits. I will pick this up and integrate a
> corresponding solution in my queue. Will keep you posted!
>
Looks like this should also include SIGIO in case hpet is selected as
host timer.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux