qemu-kvm: Consolidate kvm_eat_signals

Having both kvm_eat_signal and kvm_eat_signals makes the code harder to read. Moreover, given the single caller of kvm_eat_signals, there is no real reason to keep it in a separate function.

Quoted 5 lines · Jan Kiszka

Having both kvm_eat_signal and kvm_eat_signals makes the code harder to read. Moreover, given the single caller of kvm_eat_signals, there is no real reason to keep it in a separate function.

Given that with the iothread we spend very little time processing signals in vcpu threads, maybe it's better to drop the loop completely. The common case is zero or one pending signals. The uncommon case of two or more pending signals will be handled by the KVM_RUN ioctl returning immediately with -EINTR (i.e. in the outer loop).

Quoted 7 lines

Given that with the iothread we spend very little time processing signals in vcpu threads, maybe it's better to drop the loop completely. The common case is zero or one pending signals. The uncommon case of two or more pending signals will be handled by the KVM_RUN ioctl returning immediately with -EINTR (i.e. in the outer loop).

You mean

static void kvm_main_loop_wait(CPUState *env, int timeout)

{
    pthread_mutex_unlock(&qemu_mutex);
    kvm_eat_signal(env, timeout);
    pthread_mutex_lock(&qemu_mutex);
    cpu_single_env = env;

    vcpu_info[env->cpu_index].signalled = 0;
}

?

Quoted 20 lines · Jan Kiszka

Quoted 8 lines

Given that with the iothread we spend very little time processing signals in vcpu threads, maybe it's better to drop the loop completely. The common case is zero or one pending signals. The uncommon case of two or more pending signals will be handled by the KVM_RUN ioctl returning immediately with -EINTR (i.e. in the outer loop).

You mean

static void kvm_main_loop_wait(CPUState *env, int timeout)

{
    pthread_mutex_unlock(&qemu_mutex);
    kvm_eat_signal(env, timeout);
    pthread_mutex_lock(&qemu_mutex);
    cpu_single_env = env;

    vcpu_info[env->cpu_index].signalled = 0;
}

Quoted 3 lines

?

Yes. The loop was a (perhaps premature) optimization that is now totally unnecessary, unless I'm missing something quite large.

Oh. There used to be a bug where we didn't check for a pending signal before the first guest entry, so this would add a lot of latency (effectively making the bug window much larger). That was only closed in 2.6.24 (by 7e66f350).

Quoted 4 lines

Yes. The loop was a (perhaps premature) optimization that is now totally unnecessary, unless I'm missing something quite large.

It used to be that kvm_eat_signal() selected after consuming as many signals as possible while only sleeping once. That's why there's a combination of sleeping and polling.

Now the VCPU threads never select so the whole loop can be simplified to a single sigtimedwait() that always blocks.

In reality, I don't think sigtimedwait() is really needed/useful for VCPUs anymore. We only use it to catch SIG_IPI and we only use SIG_IPI to break out of sleeping. I don't see any reason why we couldn't switch over to using a file descriptor for notification (or a pthread condition).

In the very least, we could just select() on nothing and allow SIG_IPI to break us out of the select.

Quoted 8 lines

Now the VCPU threads never select so the whole loop can be simplified to a single sigtimedwait() that always blocks.

In reality, I don't think sigtimedwait() is really needed/useful for VCPUs anymore. We only use it to catch SIG_IPI and we only use SIG_IPI to break out of sleeping. I don't see any reason why we couldn't switch over to using a file descriptor for notification (or a pthread condition).

How would you stop a vcpu running in guest mode?

Quoted 2 lines

In the very least, we could just select() on nothing and allow SIG_IPI to break us out of the select.

sigtimedwait() (or just sigwait, now) doesn't require the signal to be delivered, so it's faster.

If there's nothing to select, why call select()?