Re: [Qemu-devel] [PATCH 1/1] KVM: Retry KVM_CREATE_VM on EINTR or EAGAIN

[Andrea Arcangeli <aarcange@redhat.com>]

On Mon, Jan 13, 2014 at 12:16:11PM +0100, Paolo Bonzini wrote:
> Il 10/01/2014 23:15, Tom Knych ha scritto:
> > I'll flip the conditional check
> > 
> > So I traced thru the code and the one path I saw returning EINTR was:
> > 
> > kvm_dev_ioctl_create_vm -> kvm_create_vm -> kvm_init_mmu_notifier ->
> > mmu_notifier_register ->  do_mmu_notifier_register -> mm_take_all_locks
> > 
> > Which checks if any signals have been raised while it was attaining
> > locks and returns EINTR.
> > 
> > Going thru my logs - all of my errors actually are EINTRs I'll remove
> > the EAGAIN
> 
> Andrea, what do you think here?  Is it intended that
> kvm_init_mmu_notifier return an EINTR that percolates up to userspace?

It is intended yes. The reason being that mm_take_all_locks is
potentially a CPU intensive operation so if we don't return -EINTR and
break it immediately if a signal is pending, we'd be potentially
hanging the process for too long, if you press C^c or the task wouldn't
go away immediately, or if you kill -9 same problem.

All CPU intensive syscalls in the kernel should check for pending
signals and return -EINTR immediately to allow userland to remain
interactive.

EAGAIN shouldn't originate anywhere in those paths so yes they're all
EINTR for interactivity.

Why don't you mask the signals instead of looping? That would be more
efficient, what's the point of interrupting the syscall and restarting
it when you can just avoid being interrupted during it? If the signal
is blocked signal_pending won't be raised and EINTR won't be
generated. I think you only need a sigprocmask or equivalent around
the call.

It's important to check the retval and fail the startup of qemu if you
still get a error, but you shouldn't loop if you just mask the signal
instead of looping.